Add waitForProcessingState to engine #218
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
In some instances users might want to wait until the engine is processing. 1 such case is after increasing the time of the ActorScheduler. The engine should start triggering timers. It takes some time before the TRIGGERED records are written to the stream. By introducing a waitForProcessingState method users have the possibility to wait a given amount of time until the TRIGGERED record should be available.
remcowesterhoud
force-pushed
the
202_flaky_test
branch
from
a5cd3f2
to
e5a2797
Compare
In some instances users might want to wait until the engine is processing. 1 such case is after increasing the time of the ActorScheduler. The engine should start triggering timers. It takes some time before the TRIGGERED records are written to the stream. By introducing a waitForProcessingState method users have the possibility to wait a given amount of time until the TRIGGERED record should be available.
remcowesterhoud
force-pushed
the
202_flaky_test
branch
from
e5a2797
to
a6d916d
Compare
When we increase the time it takes some time before the engine has started processing again, and before it is writing new records to the stream. If we immediately wait for an idle state after increasting the time it will immediately say that it is idle, even though any potential timers have not yet been triggered. By waiting for up to 1 second to see if the engine has started processing again we can make sure the timers have been triggered. After this we still need to wait until the engine is idle again, to not modify the existing functionality.
remcowesterhoud
force-pushed
the
202_flaky_test
branch
from
a6d916d
to
8dfcf64
Compare
pihme
suggested changes
Mar 1, 2022
api/src/main/java/io/camunda/zeebe/process/test/api/InMemoryEngine.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/io/camunda/zeebe/process/test/engine/IdleStateMonitor.java
Outdated
Show resolved
Hide resolved
qa/src/test/java/io/camunda/zeebe/process/test/qa/util/Utilities.java
Outdated
Show resolved
Hide resolved
waitForBusyState may be more descriptive to our users compared to waitForProcessingState.
To keep it in line with the waitForIdleState method a new `waitForBusyState()` has been introduced.
EngineStateMonitor is a more fitting name. This is because we're not only interested in the idle state of the engine, but also the busy state. Some methods have also been renamed to increase the clarity of what they do.
Similarly to waitForBusyState, waitForIdleState now has a timeout. If the engine has not reached an idle state within the specified timeout a TimeoutException will be thrown for the user to handle.
remcowesterhoud
force-pushed
the
202_flaky_test
branch
from
a29a95e
to
7e2f6d9
Compare
|
Thanks for reviewing @pihme. I've applied your suggestions and created a unit test for the |
waitForIdleState now requires a timeout to be specified. This commits adds this to all the tests that at some point wait for an idle state to be reached.
Add unit tests to verify the callbacks of the EngineStateMonitor are called at the correct times.
remcowesterhoud
force-pushed
the
202_flaky_test
branch
from
7e2f6d9
to
e69b281
Compare
pihme
approved these changes
Mar 2, 2022
engine/src/main/java/io/camunda/zeebe/process/test/engine/EngineStateMonitor.java
Outdated
Show resolved
Hide resolved
engine/src/test/java/io/camunda/zeebe/process/test/engine/EngineStateMonitorTest.java
Outdated
Show resolved
Hide resolved
engine/src/test/java/io/camunda/zeebe/process/test/engine/EngineStateMonitorTest.java
Outdated
Show resolved
Hide resolved
engine/src/test/java/io/camunda/zeebe/process/test/engine/EngineStateMonitorTest.java
Show resolved
Hide resolved
|
@pihme I made some bigger changes to the test so I'd like another review. The biggest change is that I added a way to lock the state. This is required because when we switch to a busy state and register a callback the check if we are already in an idle state. This executes this method: private void forwardToLastEvent() {
synchronized (reader) {
while (reader.hasNext()) {
lastEventPosition = reader.next().getPosition();
}
}
}This iteration causes the state to become idle. Before this change as a workaround I registered the busy callback before actually changing the state to busy. This was confusing so I switched this up, but because of it I required a way to lock the state into a busy state. That's why I added the |
Renamed runProcessingCallbacks() to notifyProcessingCallbacks() Refactored the EngineStateMonitorTest - Extracted some shared functionality to a beforeEach method - Changed the structure of the tests to be more clear what it does. Given a callback, when the engine is busy/idle and we register a callback, then the callback should be notified. - Explicitly change the to an idle state in the idle state tests (even if we are already idle)
remcowesterhoud
force-pushed
the
202_flaky_test
branch
from
1c7b853
to
9c6c1c3
Compare
pihme
suggested changes
Mar 2, 2022
engine/src/test/java/io/camunda/zeebe/process/test/engine/EngineStateMonitorTest.java
Show resolved
Hide resolved
pihme
suggested changes
Mar 2, 2022
engine/src/test/java/io/camunda/zeebe/process/test/engine/EngineStateMonitorTest.java
Show resolved
Hide resolved
engine/src/test/java/io/camunda/zeebe/process/test/engine/EngineStateMonitorTest.java
Outdated
Show resolved
Hide resolved
Locking the state in idle/busy allows us to add the callback without it being triggered as a result from adding it. We can than make the part where it trigger more explicit by change to the other state.
…sync These positions should stay synchronized. Differences are going to cause weird behaviour where we think we are in an idle/busy state, but the positions indicate something else.
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
In #202 we have a flaky tests. In this test we have a process that's triggered by a timer start event. We increase the time of the engine to trigger this timer, sleep for 100 ms, wait for the engine to reach an idle state and assert that the process instance gets completed.
This solution has a few problems. First of all we use a
Thread.sleep()to give the engine some time to trigger the timers. This is a dirty solution and it'd be better if we have a solution that only waits until the engine is running again.The second problem is that the engine has not always triggered the timers within 100 ms. Because of this the engine is considered idle, without actually finishing the processing.
This PR adds a
waitForProcessingStateto the engine. This method is very similar to thewaitForIdleStatemethod we already have. Just like that method it uses theIdleStateMonitorwith a callback to wait until the engine has started committing records again.A timeout needs to be specified to prevent waiting forever in case the engine will be permanently idle.
Related issues
closes #202
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
Testing:
Documentation: