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

Split engine and assertions into separate modules #166

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Feb 3, 2022

Description

The first step to supporting Java 8 is isolating the engine from the assertions. With this change I have split the code into 2 separate modules, the engine module and the assertions module.

The engine module is responsible for everything that has to do with running the processes. It will house the InMemoryEngine and the InMemoryDb. This module will be dependent on other Zeebe modules and will stay up to date with Java versions.

The assertions module is responsible for verifying the process. It contains assertions and other methods to traverse the record stream in order to verify the process behaves as expected. It also contains the ZeebeProcessTest annotation.
This module should support Java 8. This means it cannot have any dependencies on Zeebe modules that will update to higher Java versions. For now it still has a dependency upon the engine module, but this will be removed once we can run the engine in a testcontainer.

Related issues

closes #90

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 Feb 3, 2022

Unit Test Results

  78 files    78 suites   3m 37s ⏱️
148 tests 148 ✔️ 0 💤 0
444 runs  444 ✔️ 0 💤 0

Results for commit c8429fb.

♻️ This comment has been updated with latest results.

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.

So... this one is a little difficult to review. You say it's a first step. Maybe it's just different from the first step I would have made. Or maybe the first step is too small for me.

Let me tell you what I mean, and then maybe you can tell me what your next steps are going to be.

I guess my first step would have been to extract (and rename) InMemoryEngine and RecordStreamSource into their own module. This module will only have these two interfaces, most likely. And it's artifact name is probably something ending in -api.

Then both *-test-engine and *-process-test could depend on these shared interfaces to a) control the engine and b) fetch the records from it.

I am also still dwelling on the idea that I would at some point in the future like to see a module which has just the assertions, without the extension. I guess it is not strictly necessary since we decided we don't wont multi-version extension, but instead always use the containerized form.

However, having the assertions cleanly without the extension would allow us to use them inside Zeebe to rewrite some of the tests which are based on records. Then we would just need to rewrite the logic to provide the records.

But if the extension stay part of it, then it will pull in additional dependencies to test containers and such things. But maybe I am getting ahead of myself.

Lastly, Nicolas was lately talking about using Flatten POM in Zeebe. I must admit I haven't fully understood how it works. But from what I gathered I think it could be a game changer to deal with transient dependencies. So I would encourage you to talk to Nicolas about it and employ it here, whenever you can.

@remcowesterhoud
Copy link
Contributor Author

Thanks for reviewing. I agree that it makes sense to move the interfaces to a separate module as well. The whole communication between the assertions and the testcontainer is still a bit unclear to me. Maybe we can have a chat about that soon?

I hear you about the extension. I wanted to keep this change "small" and figured we could do this in the future. But now that I think about it again, moving the extension would be a breaking change so it'd be better to do this right away.

I saw the post about flattening pom files. I want to look into it properly first, but from what I understood it's just a plugin. I'll create a new issue for it. I isn't part of the module split so I don't want to add it to this PR.

@pihme
Copy link
Contributor

pihme commented Feb 7, 2022

Just had a look at it. I was wondering whether all tests need to be in QA. I would hope that some tests could just as well be in the module of the classes they are testing ( e.g. EngineClientTest, ...inspections.* )

@remcowesterhoud
Copy link
Contributor Author

@pihme I don't agree on moving the ...inspections.* tests. They are the same type of tests as the assertion tests in my opinion. The engine tests I have moved to the engine module.

I have also moved the RecordStreamSourceStore to make it part of BpmnAssert. I'd also like to remove the InspectionUtility and make the entry points of the inspections also part of BpmnAssert (not part of this PR). This would mean renaming BpmnAssert to something that's a bit more descriptive of what it does.

@remcowesterhoud remcowesterhoud requested a review from pihme February 7, 2022 12:42
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 good for the most part. Three comments for the last final touches.

Please also document the different modules and their Java version in README.md

@remcowesterhoud remcowesterhoud requested a review from pihme February 8, 2022 09:04
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.

Did you forget a commit? I cannot find the changes to the POM files which you marked as resolved.

@remcowesterhoud
Copy link
Contributor Author

I committed it to the wrong branch 🙈 It's pushed now.

@remcowesterhoud remcowesterhoud requested a review from pihme February 8, 2022 09:45
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.

Thanks for the work! Looks good now!

In order to support Java 8 a first step is to split the project into separate modules. This enables two things:

1. The test engine can be containerized. This allows us to run tests using testcontainers instead of needing a dependency on the engine.
2. The assertions can now be used stand-alone. This allows for reusing the assertions in different projects that don't require the engine, removing a lot of overhead.

The project has been split in 5 modules:
1. Api
     - This module contains public interfaces. It should always be Java 8 compatible.
 2. Assertions
     - This module contains all the assertions. It should always be Java 8 compatible.
 3. Engine
     - This module contains the in memory engine. It is not bound to a specific Java version. The api and assertions module should never depend on this module.
 4. Extension
     - This module contains the annotation for your unit tests. As of now it depends on the engine and thus is not bound to a specific Java version. In the future this dependency will be removed making this module Java 8 compatible
 5. QA
    - This module contains our QA tests. These are the tests that span multiple modules. There is no reason to depend on this module. It is not bound to a specific Java version.
@remcowesterhoud remcowesterhoud merged commit 43405ed into main Feb 8, 2022
@remcowesterhoud remcowesterhoud deleted the 90-split-modules branch February 8, 2022 10:02
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.

Split in separate modules
2 participants