[JBPM-10231] Fixed non-idempotent unit tests in jbpm-runtime-manager
#2421
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
|
@fjtirado looks like it worths review. |
|
jenkins test this |
|
gmunozfe
added
backport-7.67.x
|
Closing this as superseded by #2422 |
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.
link
The Problem
Some unit tests are non-idempotent, as they pass in the first run but fail in the second run in the same environment. A fix is necessary since unit tests shall be self-contained, ensuring that the state of the system under test is consistent at the beginning of each test. In practice, fixing non-idempotent tests can help proactively avoid state pollution that results in test order dependency (which could cause problems under test selection , prioritization or parallelization).
Reproduce
Using the
NIOInspectorplugin that supports rerunning JUnit tests in the same environment. UseEventHnadlingTest#testRunMultiEventProcessPerRequestRuntimeManageras an example:3 Non-Idempotent Tests & Proposed Fix
EventHnadlingTest#testRunMultiEventProcessPerRequestRuntimeManager
Reason: With "Per Request" strategy, for every call to
getRuntimeEngine(), a new instance will usually be delivered with brand newKieSession. However, the exception to this is whengetRuntimeEngine()invoked within the same transaction from different places (which would be the case iftestRunMultiEventProcessPerRequestRuntimeManageris re-executed). In that case, the manager caches the currently active instance. Therefore, in the second execution of the test, an error will be thrown since the retrieved (cached)KieSessionis already closed by the previous test execution. To resolve this, the runtime engine manager shall be disposed to ensure that itsKieSessionis destroyed as well.Error message of the test in the repeated run:
Fix: Add the line
manager.disposeRuntimeEngine(runtime);before closing the managerPerCaseRuntimeManagerTest#testMultipleProcessesInSingleCaseCompletedInSequence
Reason: The "Per Case" strategy specifies that every process instance that belongs to same case will be bound to a single
ksessionfor its entire life time. Once started, whenever such operations are invoked, the manager will retrieve the sameksession. Similarly,manager.disposeRuntimeEngine(runtime)shall be called beforemanager.close()to ensure that the second test execution does not directly retrieve the already closedksession.Error message in the repeated run:
Fix: Same as the previous test: add
manager.disposeRuntimeEngine(runtime);before closing the managerPerRequestRuntimeManagerTest#testMultiplePerRequestManagerFromSingleThread
Reason: This is actually a typo - the process instance
runtime2is managed bymanager2, notmanager.The original code is
It shall be changed to
Error message of one of the tests in the repeated run:
Fix: As described above
Verifying this change
After the patch, running the tests repeatedly in the same environment will not lead to failures.