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.

Sign up for GitHub

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

Jump to bottom

Implement test engine #76

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Implement test engine #76

merged 1 commit into from
Nov 30, 2021

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Nov 26, 2021 •
edited
Loading

Description

EZE has been copied and translated into Java.
Still to do to entirely get rid of the EZE dependency:

  • Modify ZeebeAssertionsExtension so that it will take over the functionality of the @EmbeddedZeebeEngine
  • Modify tests to use our own classes
  • Add our own dependency on Zeebe

Related issues

closes #24

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

pihme
pihme suggested changes Nov 26, 2021
View reviewed changes
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.

Great work!

Several comments and change requests.

src/main/java/io/camunda/zeebe/bpmnassert/testengine/EngineFactory.java Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/EngineFactory.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/EngineFactory.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/EngineFactory.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/EngineFactory.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/ZeebeEngineImpl.java Outdated Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/EngineClientTest.java Outdated Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/EngineClientTest.java Outdated Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/EngineClientTest.java Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/PrintRecordStreamExtension.java Show resolved Hide resolved
remcowesterhoud
remcowesterhoud commented Nov 29, 2021
View reviewed changes
Copy link
Contributor Author

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

@pihme The changes look good!
I've added some comments. Please take a look at them once you have time.

src/main/java/io/camunda/zeebe/bpmnassert/testengine/IdleStateMonitor.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/ZeebeEngineImpl.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/ZeebeEngineImpl.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/db/DbNullKey.java Show resolved Hide resolved
...a/io/camunda/zeebe/bpmnassert/testengine/db/InMemoryZeebeDbColumnFamilyIterationContext.java Outdated Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/db/InMemoryZeebeDbTransactionTest.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/db/InMemoryZeebeDbTransactionContext.java Outdated Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/db/InMemoryZeebeDbTransactionTest.java Outdated Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/db/InMemoryZeebeDbTransactionTest.java Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/IdleStateMonitor.java
import java.util.ArrayList;
import java.util.List;

final class IdleStateMonitor implements LogStorage.CommitListener, StreamProcessorListener {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

❌ We should write some unit tests for this logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the IdleStateMonitor is not stable as it is now. I tried writing unit tests to verify the waiting for idle state works correctly, but they kept failing.
I propose we merge this PR first and have another look at this separate from this PR.

@pihme
Copy link
Contributor

pihme commented Nov 29, 2021

@remcowesterhoud Looked over all your comments. Feel free to change things. I will focus on medic duty this week and don't want to stall progress.

@remcowesterhoud
Copy link
Contributor Author

@pihme I've processed all of the review comments. I've split them up by commit so it's easier to review them. If you find the time please have another look 😁

pihme
pihme suggested changes Nov 30, 2021
View reviewed changes
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.

Great work! Thanks a lot!

I have a couple of remaining changes.

Happy to look at it again, if you want. But you can also just merge it.

src/test/java/io/camunda/zeebe/bpmnassert/testengine/EngineClientTest.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/RecordStreamSourceImpl.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/RecordStreamSourceImpl.java
if (lastPosition < 0) {
logStreamReader.seekToFirstEvent();
} else {
logStreamReader.seekToNextEvent(lastPosition);
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Does this work as expected when there is no event after last position?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't throw any errors or that sort of thing. It returns false if there are no events after this position. This means logStreamReader.hasNext() resolves to false as well and the list of records doesn't get updated as there are no new records.

src/main/java/io/camunda/zeebe/bpmnassert/testengine/ZeebeEngineImpl.java Outdated Show resolved Hide resolved
src/main/java/io/camunda/zeebe/bpmnassert/testengine/PartitionCommandSenderImpl.java Show resolved Hide resolved
src/test/java/io/camunda/zeebe/bpmnassert/testengine/db/InMemoryZeebeDbTransactionTest.java Show resolved Hide resolved
@remcowesterhoud remcowesterhoud force-pushed the 24-implement-test-engine branch from 1d5391f to bb32ec1 Compare November 30, 2021 16:12
@remcowesterhoud remcowesterhoud merged commit 6e89e5c into main Nov 30, 2021
@remcowesterhoud remcowesterhoud deleted the 24-implement-test-engine branch December 1, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pihme pihme pihme requested changes

Assignees

@remcowesterhoud remcowesterhoud

@pihme pihme

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Copy EZE to this project
2 participants
@remcowesterhoud @pihme