Run tests using testcontainers #198
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
remcowesterhoud
force-pushed
the
192-testcontainer-extension
branch
17 times, most recently
from
c4ee775 to
44d8d31
Compare
|
@korthout Would you mind reviewing this one? It seems like a huge change at first glance. While it certainly isn't a small change, the bulk of the changes are because of commit 0c553a6 where we duplicated the tests. I recommend reviewing this PR by commit. I've tried to split it up in small reviewable chunks. |
remcowesterhoud
changed the title
remcowesterhoud
changed the title
|
Thanks @remcowesterhoud, I'll make sure to check it out. Please note that there is a conflict. |
This module will be used to run the tests in a testcontainer as opposed to the regular extension module.
remcowesterhoud
force-pushed
the
192-testcontainer-extension
branch
from
26729d5 to
c2e9cd6
Compare
korthout
approved these changes
Feb 23, 2022
There was a problem hiding this comment.
Nice work @remcowesterhoud I have some comments and questions. Most are simply suggestions and I don't think any of them is blocking. So, LGTM 🎸 But, please do have a look at my comments
...src/main/java/io.camunda.zeebe.process.test.extension.testcontainer/ContainerizedEngine.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/io.camunda.zeebe.process.test.extension.testcontainer/ZeebeProcessTest.java
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
| final RecordResponse response = RecordResponse.newBuilder() | ||
| .setRecordJson(record.toJson()) |
There was a problem hiding this comment.
@npepinpe We talked about deprecating .toJson(), but it currently isn't. Perhaps we decided against it. 🤷 Still protocol-jackson describes the use of ObjectMapper.writeValueAsString instead of the toJson. Do you feel strongly towards a specific approach?
...gent/src/main/java/io/camunda/zeebe/process/test/engine/agent/RecordStreamSourceWrapper.java
Show resolved
Hide resolved
engine-agent/src/main/java/io/camunda/zeebe/process/test/engine/agent/AgentProperties.java
Outdated
Show resolved
Hide resolved
engine-agent/src/main/java/io/camunda/zeebe/process/test/engine/agent/AgentProperties.java
Outdated
Show resolved
Hide resolved
This extension makes it possible to run tests against a testcontainer. The extension is responsible for managing the testcontainer lifecycle. It will start the container once for the entire testsuite. This is done in the beforeAll() method. The EngineContainer singleton will make sure it only gets created once. Before each test the engine gets reset. This is done in the beforeEach() method, instead of in the afterEach(). The reasoning behind this is that the jUnit lifecycle calls afterEach() before testFailed(). We want to access the record stream upon test failure. If we reset the engine in the afterEach() method we will lose our access to the record stream in the testFailed() method.
Records have their own toJson() method which does the same thing as manually mapping it using the ObjectMapper. Using this toJson() method simplifies the code.
The toJson() method on Records is not threadsafe at the moment. This is because some Records contain an ArrayProperty. During serializationa new Record will be created (eg. a DeploymentRecord). This is an UnpackedObject which contains an ArrayProperty. The ArrayProperty has an ArrayValue. Serializing an ArrayValue modifies the object. This means that multiple threads trying to serialize the same DeploymentRecord at a time will modify the ArrayValue at the same time. This results in exceptions. Making it thread-safe this way a quick-fix. It would be nice if we can map and store the records as they come in. This way the requests themselves only have to return a list of mapped records, instead of mapping all records.
We want to verify both our extensions. Therefore, it makes sense to run all our QA tests against both extensions. For now all the tests are duplicated to achieve this. This, of course, is not ideal in maintaining the tests. In #195 we will get rid of this and find a more generic solution.
The image name, tag and ports used were hardcoded. In our workflow we'll have to modify at least the tag so we can run QA tests against the correct versions. This is easier to do if we store these options in a properties file. This commit also removed the BindableService vararg. This was never used.
We have 2 kinds of tests in our QA module. The regular ones, and the ones that will run using testcontainers. The regular ones will be ran against 3 OS (ubuntu, windows and mac OS). This hard to achieve in a generic way with testcontainers. This is because with tetcontainers we should build the docker image first, so we ensure we run the tests against the latest code. There is multiple issues with this: 1. Windows uses the wrong daemon and is unable to build the image. 2. GitHub's macos-latest container does not have docker installed. There is probably ways to switch the daemon and install docker. However, there is not much value in running all the tests 2 times on different OSs. Besides this, the execution will increase dramatically. Because of this we will run the testcontainer tests on 1 OS (ubuntu).
When a new commit gets pushed to master, or a new release is created we should update the configuration files so that the future QA tests will be using the correct image.
We used to run the maven and docker release jobs simultaneously. Now that the docker job has been changed to update the configuration files we have to make sure the maven release job runs after this is finished. If we don't we introduce a race-condition. If the maven release job completes before the docker job updated the configuration files, it results in the maven release running tests on the wrong container.
remcowesterhoud
force-pushed
the
192-testcontainer-extension
branch
from
c2e9cd6 to
18820ce
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a new module,
extension-testcontainer. This module provides users with an annotation that will run their tests in a testcontainer. Achieving this required a couple of steps:@ZeebeProcessTestannotation to the one fromextension-testcontainerwe can run the same QA tests with the testcontainer. This introduces a lot of duplication. This will be solved in Remove QA test duplication #195Note: Documentation is not updated yet. In #176 I will revisit the documentation and write a finalised version. This will also include the usage of this new extension.
Related issues
closes #192
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
Testing:
Documentation: