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

[JBPM-10231] Fixed all non-idempotent tests #2422

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kaiyaok2
Copy link

@kaiyaok2 kaiyaok2 commented May 26, 2024

This PR supersedes #2421 - it fixes all non-idempotent unit tests. All tests pass and are idempotent after the fix. Below are all tests fixed:

EventHnadlingTest#testRunMultiEventProcessPerRequestRuntimeManager

Reason: With "Per Request" strategy, for every call to getRuntimeEngine(), a new instance will usually be delivered with brand new KieSession. However, the exception to this is when getRuntimeEngine() invoked within the same transaction from different places (which would be the case if testRunMultiEventProcessPerRequestRuntimeManager is 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) KieSession is already closed by the previous test execution. To resolve this, the runtime engine manager shall be disposed to ensure that its KieSession is destroyed as well. 

Error message of the test in the repeated run:

java.lang.IllegalStateException: Session/EntityManager is closed
	at org.hibernate.internal.AbstractSharedSessionContract.checkOpen(AbstractSharedSessionContract.java:375)
	at org.hibernate.engine.spi.SharedSessionContractImplementor.checkOpen(SharedSessionContractImplementor.java:148)
	at org.hibernate.internal.SessionImpl.joinTransaction(SessionImpl.java:3633)
	at org.drools.persistence.jpa.JpaPersistenceContext.joinTransaction(JpaPersistenceContext.java:106)
	at org.drools.persistence.PersistableRunner$TransactionInterceptor.execute(PersistableRunner.java:603)
	at org.drools.persistence.PersistableRunner$TransactionInterceptor.execute(PersistableRunner.java:572)
	at org.drools.core.command.impl.AbstractInterceptor.executeNext(AbstractInterceptor.java:39)
	at org.drools.persistence.jpa.OptimisticLockRetryInterceptor.internalExecute(OptimisticLockRetryInterceptor.java:102)
	at org.drools.persistence.jpa.OptimisticLockRetryInterceptor.execute(OptimisticLockRetryInterceptor.java:83)
	at org.drools.persistence.jpa.OptimisticLockRetryInterceptor.execute(OptimisticLockRetryInterceptor.java:44)
	at org.drools.core.command.impl.AbstractInterceptor.executeNext(AbstractInterceptor.java:39)
	at org.drools.persistence.jta.TransactionLockInterceptor.execute(TransactionLockInterceptor.java:73)
	at org.drools.persistence.jta.TransactionLockInterceptor.execute(TransactionLockInterceptor.java:45)
	at org.drools.core.command.impl.AbstractInterceptor.executeNext(AbstractInterceptor.java:39)
	at org.jbpm.runtime.manager.impl.error.ExecutionErrorHandlerInterceptor.internalExecute(ExecutionErrorHandlerInterceptor.java:66)
	at org.jbpm.runtime.manager.impl.error.ExecutionErrorHandlerInterceptor.execute(ExecutionErrorHandlerInterceptor.java:52)
	at org.jbpm.runtime.manager.impl.error.ExecutionErrorHandlerInterceptor.execute(ExecutionErrorHandlerInterceptor.java:29)
	at org.drools.persistence.PersistableRunner.execute(PersistableRunner.java:400)
	at org.drools.persistence.PersistableRunner.execute(PersistableRunner.java:68)
	at org.drools.core.runtime.InternalLocalRunner.execute(InternalLocalRunner.java:37)
	at org.drools.core.runtime.InternalLocalRunner.execute(InternalLocalRunner.java:41)
	at org.drools.core.command.impl.CommandBasedStatefulKnowledgeSession.startProcess(CommandBasedStatefulKnowledgeSession.java:275)
	at org.drools.core.command.impl.CommandBasedStatefulKnowledgeSession.startProcess(CommandBasedStatefulKnowledgeSession.java:259)
	at org.jbpm.runtime.manager.concurrent.EventHnadlingTest.testRunMultiEventProcessPerRequestRuntimeManager(EventHnadlingTest.java:86)

Fix: Add the line manager.disposeRuntimeEngine(runtime); before closing the manager

PerCaseRuntimeManagerTest#testMultipleProcessesInSingleCaseCompletedInSequence

Reason: The "Per Case" strategy specifies that every process instance that belongs to same case will be bound to a single ksession for its entire life time.  Once started, whenever such operations are invoked, the manager will retrieve the same ksession. Similarly, manager.disposeRuntimeEngine(runtime) shall be called before manager.close() to ensure that the second test execution does not directly retrieve the already closed ksession.

Error message in the repeated run:

java.lang.IllegalStateException: Session/EntityManager is closed
	at org.hibernate.internal.AbstractSharedSessionContract.checkOpen(AbstractSharedSessionContract.java:375)
	at org.hibernate.engine.spi.SharedSessionContractImplementor.checkOpen(SharedSessionContractImplementor.java:148)
	at org.hibernate.internal.SessionImpl.joinTransaction(SessionImpl.java:3633)
	at org.drools.persistence.jpa.JpaPersistenceContext.joinTransaction(JpaPersistenceContext.java:106)
	at org.drools.persistence.PersistableRunner$TransactionInterceptor.execute(PersistableRunner.java:603)
	at org.drools.persistence.PersistableRunner$TransactionInterceptor.execute(PersistableRunner.java:572)
	at org.drools.core.command.impl.AbstractInterceptor.executeNext(AbstractInterceptor.java:39)
	at org.drools.persistence.jpa.OptimisticLockRetryInterceptor.internalExecute(OptimisticLockRetryInterceptor.java:102)
	at org.drools.persistence.jpa.OptimisticLockRetryInterceptor.execute(OptimisticLockRetryInterceptor.java:83)
	at org.drools.persistence.jpa.OptimisticLockRetryInterceptor.execute(OptimisticLockRetryInterceptor.java:44)
	at org.drools.core.command.impl.AbstractInterceptor.executeNext(AbstractInterceptor.java:39)
	at org.drools.persistence.jta.TransactionLockInterceptor.execute(TransactionLockInterceptor.java:73)
	at org.drools.persistence.jta.TransactionLockInterceptor.execute(TransactionLockInterceptor.java:45)
	at org.drools.core.command.impl.AbstractInterceptor.executeNext(AbstractInterceptor.java:39)
	at org.jbpm.runtime.manager.impl.error.ExecutionErrorHandlerInterceptor.internalExecute(ExecutionErrorHandlerInterceptor.java:66)
	at org.jbpm.runtime.manager.impl.error.ExecutionErrorHandlerInterceptor.execute(ExecutionErrorHandlerInterceptor.java:52)
	at org.jbpm.runtime.manager.impl.error.ExecutionErrorHandlerInterceptor.execute(ExecutionErrorHandlerInterceptor.java:29)
	at org.drools.persistence.PersistableRunner.execute(PersistableRunner.java:400)
	at org.drools.persistence.PersistableRunner.execute(PersistableRunner.java:68)
	at org.drools.core.runtime.InternalLocalRunner.execute(InternalLocalRunner.java:37)
	at org.drools.core.runtime.InternalLocalRunner.execute(InternalLocalRunner.java:41)
	at org.drools.core.command.impl.CommandBasedStatefulKnowledgeSession.getIdentifier(CommandBasedStatefulKnowledgeSession.java:134)
	at org.jbpm.runtime.manager.impl.PerCaseRuntimeManagerTest.testMultipleProcessesInSingleCaseCompletedInSequence(PerCaseRuntimeManagerTest.java:349)

Fix: Same as the previous test: add manager.disposeRuntimeEngine(runtime); before closing the manager

PerRequestRuntimeManagerTest#testMultiplePerRequestManagerFromSingleThread

Reason: This is actually a typo - the process instance runtime2 is managed by manager2, not manager.
The original code is

manager.disposeRuntimeEngine(runtime1);
manager.disposeRuntimeEngine(runtime2);

It shall be changed to 

manager.disposeRuntimeEngine(runtime1);
manager2.disposeRuntimeEngine(runtime2);

Error message of one of the tests in the repeated run:

java.lang.IllegalStateException: RuntimeManager with id second is already active
	at org.jbpm.runtime.manager.impl.AbstractRuntimeManager.<init>(AbstractRuntimeManager.java:141)
	at org.jbpm.runtime.manager.impl.PerRequestRuntimeManager.<init>(PerRequestRuntimeManager.java:74)
	at org.jbpm.runtime.manager.impl.RuntimeManagerFactoryImpl.newPerRequestRuntimeManager(RuntimeManagerFactoryImpl.java:73)
	at org.jbpm.runtime.manager.impl.PerRequestRuntimeManagerTest.testMultiplePerRequestManagerFromSingleThread(PerRequestRuntimeManagerTest.java:543)

Fix: As described above

FilteredKModuleDeploymentServiceTest#testSerializationClassesLimitedInDeploymentDependencies

Reason: The test class keeps track of units, a list of that stores the Kmodule deploy units constructed by each unit test. Unit tests append to this list via units.add(deploymentUnit);, and every deploy unit in the list units is undeployed in the cleanup() method. However, in testSerializationClassesLimitedInDeploymentDependencies(), multiple deploy units are constructed (childDeploymentUnit and parentDeploymentUnit) and are not directly added to units. Therefore, these units are not undeployed after test execution, so the second test execution will throw an error.

Error message in the second run:

java.lang.RuntimeException: java.lang.RuntimeException: java.lang.IllegalStateException: RuntimeManager with id org.test:jbpm-kie-services-filter-test-dep:1.0.0 is already active
	at org.jbpm.kie.services.impl.KModuleDeploymentService.deploy(KModuleDeploymentService.java:221)
	at org.jbpm.kie.services.test.FilteredKModuleDeploymentServiceTest.testSerializationClassesLimitedInDeploymentDependencies(FilteredKModuleDeploymentServiceTest.java:429)

Fix: Add childDeploymentUnit and parentDeploymentUnit to the list units at the end of the test.

FilteredKModuleDeploymentServiceTest#testSerializationClassesLimitedInDeploymentItself

Reason: Similar with the test above, testSerializationClassesLimitedInDeploymentItself() constructs multiple deploy units (limitDeploymentUnit and allDeploymentUnit) without adding them to units, so they're not undeployed after test execution.

Error message in the second run:

java.lang.RuntimeException: java.lang.RuntimeException: java.lang.IllegalStateException: RuntimeManager with id org.test:jbpm-kie-services-filter-test:1.0.0 is already active
	at org.jbpm.kie.services.impl.KModuleDeploymentService.deploy(KModuleDeploymentService.java:221)
	at org.jbpm.kie.services.test.FilteredKModuleDeploymentServiceTest.testSerializationClassesLimitedInDeploymentItself(FilteredKModuleDeploymentServiceTest.java:359)

Fix: Add limitDeploymentUnit and allDeploymentUnit to the list units at the end of the test.

AsyncWorkItemHandlerTest#testRunProcessWithAsyncHandlerCallbackErrorRetry

Reason: The test asserts IncrementService.get() returns 0 at first and then 1 after executing the script task process. However, the static counter in IncrementService records the total number of service processes, so repeated test runs will still increment the counter.

Error message in the second run:

java.lang.AssertionError: expected:<0> but was:<1>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at org.jbpm.executor.impl.wih.AsyncWorkItemHandlerTest.testRunProcessWithAsyncHandlerCallbackErrorRetry(AsyncWorkItemHandlerTest.java:530)

Fix: Record the value returned by IncrementService.get() in a local variable when starting the test. Check if it's unchanged at the time of the first original assertion, and if it's incremented by 1 at the time of the second original assertion. This preserves test logic while removes non-idempotency.

RestWorkitemHandlerClientCreationTest#testPooledClientCreationWithDefaultTimeouts

Reason: When mocking the class RESTWorkItemHandler, the doCacheClient flag is enabled, meaning that repeated calls to executeWorkItem() with the same parameters will use the cached client and not invoke getNewPooledHttpClient() repeatedly. In the repeated execution of this test, since the parameters are unchanged, getNewPooledHttpClient() will not be called at all, thus the exception.

Error message in the second run:

java.lang.AssertionError: expected:<0> but was:<1>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at org.jbpm.executor.impl.wih.AsyncWorkItemHandlerTest.testRunProcessWithAsyncHandlerCallbackErrorRetry(AsyncWorkItemHandlerTest.java:530)

Fix: Ensure there's no existing cachedClient when starting testPooledClientCreationWithDefaultTimeouts().

ParentChildMarshallingJpaTest#testProcess

Reason: The test asserts that em.createQuery("select i from Person i").getResultList().size() is always 1 at the end of the test execution. However, each test execution will add a person to the database, so the size of the result list is accumulative, causing the assertion to fail in repeated runs. Also, the manager is not closed at the end of the test, so repeated runs will throw "already exists" error.

Error message in the second run:

java.lang.AssertionError: expected:<1> but was:<2>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at org.jbpm.test.functional.jpa.ParentChildMarshallingJpaTest.testProcess(ParentChildMarshallingJpaTest.java:121)

Fix: Add a tearDown() method to clear the created person as well as its application in the database after test execution. Also, close the manager at the end of the test.

AsyncWIHOnOracleTest#testAsyncWIHExecutedMoreThanOnceOnOracle

Reason: The helper class CounterCommand records the global number of command executions. The test asserts that CounterCommand.getCounter() is always 1 after executing a process. However, repeated test runs will accumulate the counter, so the assertion would fail.

Error message in the second run:

org.opentest4j.AssertionFailedError: [The job has been executed multiple times] 
Expecting:
 <2>
to be equal to:
 <1>
but was not.
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at org.jbpm.test.regression.async.AsyncWIHOnOracleTest.testAsyncWIHExecutedMoreThanOnceOnOracle(AsyncWIHOnOracleTest.java:73)

Fix: Add a resetCounter() function in the helper class and call it in the tearDown() method.

AsyncMultisubprocessTest#testCorrectProcessStateAfterExceptionEndSignal

Reason: The test defines a static CountDownLatch, which counts down when the signal of process completion is received. Then the test waits for the countdown (via latch.await()) and asserts processInstance is cleared (null) after that. However, the static latch is not reset after the first test execution, so latch.await() will not wait in the repeated executions, so the assertion is made before the process completes, causing the error.

Error message in the second run:

org.opentest4j.AssertionFailedError: 
Expecting:
 <WorkflowProcessInstance1 [processId=TestSubMult,state=1]>
to be equal to:
 <null>
but was not.
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at org.jbpm.test.functional.async.AsyncMultisubprocessTest.testCorrectProcessStateAfterExceptionEndSignal(AsyncMultisubprocessTest.java:108)

Fix: Construct a new latch for each test execution.

MBeansMonitoringWithJBpmTest#testRulesAndProcesses

Reason: In each test execution, a KieContainer kc is built by calling newKieContainer(). The test does not dispose kc after test execution, so in the repeated test execution, newKieContainer() will throw an error since a KieContainer of the same ID is already existing.

Error message in the second run:

java.lang.IllegalStateException: There's already another KieContainer created with the id myContainerId
	at org.drools.compiler.kie.builder.impl.KieServicesImpl.newKieContainer(KieServicesImpl.java:215)
	at org.drools.compiler.kie.builder.impl.KieServicesImpl.newKieContainer(KieServicesImpl.java:180)
	at org.jbpm.test.functional.monitoring.MBeansMonitoringWithJBpmTest.testRulesAndProcesses(MBeansMonitoringWithJBpmTest.java:181)

Fix: Clear kc at the end of the test.

@kaiyaok2 kaiyaok2 changed the title fixed remaining non-idempotent tests [JBPM-10231] Fixed remaining non-idempotent tests May 26, 2024
@kaiyaok2
Copy link
Author

@gmunozfe Please kindly lmk if I shall merge this to the previous PR

@gmunozfe
Copy link
Member

jenkins test this

@gmunozfe
Copy link
Member

@gmunozfe Please kindly lmk if I shall merge this to the previous PR

@kaiyaok2 thanks for your contributions. Yes, you can merge both commits in a single PR as they are related to the same issue

@kaiyaok2 kaiyaok2 changed the title [JBPM-10231] Fixed remaining non-idempotent tests [JBPM-10231] Fixed all non-idempotent tests May 29, 2024
@kaiyaok2
Copy link
Author

@gmunozfe Thanks for your message. Now all relevant code changes are in this PR.

@gmunozfe
Copy link
Member

jenkins test this

Copy link

sonarcloud bot commented May 31, 2024

Quality Gate Passed Quality Gate passed

Issues
44 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@gmunozfe gmunozfe self-requested a review June 3, 2024 21:22
Copy link
Member

@gmunozfe gmunozfe 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 to me, thanks for your work @kaiyaok2 !

@gmunozfe gmunozfe added backport-7.67.x Generate backport PR for 7.67.x branch backport-7.67.x-blue Generate backport PR for 7.67.x-blue branch labels Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-7.67.x Generate backport PR for 7.67.x branch backport-7.67.x-blue Generate backport PR for 7.67.x-blue branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants