Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove test duplication #255

Merged
merged 7 commits into from
Mar 15, 2022
Merged

Remove test duplication #255

merged 7 commits into from
Mar 15, 2022

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Mar 11, 2022

Description

This PR introduces abstract test classes. Abstract test classes should contain all the test implementations. The other classes that extend this only need to provide the extension. This way we don't have to duplicate all of our test implementations.

One thing to note is that injection of the ZeebeClient, ZeebeTestEngine and RecordStream. These fields get injected in what jUnit defines as the test class:
final Class<?> requiredTestClass = extensionContext.getRequiredTestClass();

We are dealing with 2 different scenarios:

  1. We have the assertion tests. Here the abstract classes contain 2 nested classes, HappyPathTests and UnhappyPathTests. In this scenario the nested class is the test class. These fields get injected just fine.
  2. In other tests we do not have any nested classes. Here the class that extends the abstract class will be the test class (e.g. the WorkterTest class). For these classes the fields must be declared on the extending class. We can access these in the abstract class by adding abstract getters.

Related issues

closes #195

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually

Documentation:

  • Javadoc has been written
  • The documentation is updated

@github-actions
Copy link

github-actions bot commented Mar 11, 2022

Unit Test Results

132 files  132 suites   6m 24s ⏱️
225 tests 225 ✔️ 0 💤 0
868 runs  868 ✔️ 0 💤 0

Results for commit 632b64e.

♻️ This comment has been updated with latest results.

@remcowesterhoud remcowesterhoud requested a review from pihme March 11, 2022 13:22
@remcowesterhoud remcowesterhoud force-pushed the 195_test_duplication branch 2 times, most recently from 6fc196f to dad372a Compare March 11, 2022 13:55
Abstract test classes should contain all the test implementations. The other classes that extend this only need to provide the extension. This way we don't have to duplicate all of our test implementations.

One thing to note is that injection of the ZeebeClient, ZeebeTestEngine and RecordStream. These fields get injected in what jUnit defines as the test class:
final Class<?> requiredTestClass = extensionContext.getRequiredTestClass();

We are dealing with 2 different scenarios. We have the assertion tests. Here the abstract classes contain 2 nested classes, HappyPathTests and UnhappyPathTests. In this scenario the nested class is the test class. These fields get injected just fine.
In other tests we do not have any nested classes. Here the class that extends the abstract class will be the test class (e.g. the WorkterTest class). For these classes the fields must be declared on the extending class. We can access these in the abstract class by adding abstract getters.
All unit tests will now extends from an abstract class. This class provides the test implementation. The extending class only needs to have the relevant annotation and in some cases needs to define ZeebeClient, ZeebeTestEngine and RecordStream fields.
The java version in the root pom was 11. This is not accurate as by default we use java 17, unless otherwise specified.
Adds a unit test which will make sure that all the abstract test classes are implemented in the regular and in the testcontainer package, as well as annotated with the correct extension.
In our documentation we talk about the embedded engine. It makes sense to name our packages in a similar way.
Automatically adding the copyright headers included a new line. The maven license plugin does not want this new line, so I have removed this from the configuration.
Copy link
Contributor

@pihme pihme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean. One suggestion to make it even cleaner

@remcowesterhoud remcowesterhoud requested a review from pihme March 15, 2022 10:23
@remcowesterhoud
Copy link
Contributor Author

@pihme I've changed the test to use ClassGraph. Please have another look and see if it's improved. I quite like it 😄

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@pihme pihme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Looks much better already. Some final suggestions for the cherry on top

Replace custom implementation with classgraph. This solution is easier to understand.
@remcowesterhoud remcowesterhoud merged commit 74fab5e into main Mar 15, 2022
@remcowesterhoud remcowesterhoud deleted the 195_test_duplication branch March 15, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove QA test duplication
3 participants