diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.2.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.2.adoc index 36d267b0e33..fcf7313a635 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.2.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.2.adoc @@ -16,7 +16,9 @@ on GitHub. [[release-notes-5.11.2-junit-platform-bug-fixes]] ==== Bug Fixes -* ❓ +* Fix regression in parallel execution that was introduced in 5.10.4/5.11.1 regarding + global read-write locks. When such a lock was declared on descendants of top-level nodes + in the test tree, such as Cucumber scenarios, test execution failed. [[release-notes-5.11.2-junit-platform-deprecations-and-breaking-changes]] ==== Deprecations and Breaking Changes diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java index c3863a04934..eb95daab023 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java @@ -33,6 +33,10 @@ void useResourceLock(TestDescriptor testDescriptor, ResourceLock resourceLock) { resourceLocksByTestDescriptor.put(testDescriptor, resourceLock); } + void removeResourceLock(TestDescriptor testDescriptor) { + resourceLocksByTestDescriptor.remove(testDescriptor); + } + Optional getForcedExecutionMode(TestDescriptor testDescriptor) { return testDescriptor.getParent().flatMap(this::lookupExecutionModeForcedByAncestor); } diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java index 8e47d006849..ada030923b7 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java @@ -50,6 +50,12 @@ NodeExecutionAdvisor walk(TestDescriptor rootDescriptor) { private void walk(TestDescriptor globalLockDescriptor, TestDescriptor testDescriptor, NodeExecutionAdvisor advisor) { + + if (advisor.getResourceLock(globalLockDescriptor) == globalReadWriteLock) { + // Global read-write lock is already being enforced, so no additional locks are needed + return; + } + Set exclusiveResources = getExclusiveResources(testDescriptor); if (exclusiveResources.isEmpty()) { if (globalLockDescriptor.equals(testDescriptor)) { @@ -73,7 +79,12 @@ private void walk(TestDescriptor globalLockDescriptor, TestDescriptor testDescri }); } if (allResources.contains(GLOBAL_READ_WRITE)) { - forceDescendantExecutionModeRecursively(advisor, globalLockDescriptor); + advisor.forceDescendantExecutionMode(globalLockDescriptor, SAME_THREAD); + doForChildrenRecursively(globalLockDescriptor, child -> { + advisor.forceDescendantExecutionMode(child, SAME_THREAD); + // Remove any locks that may have been set for siblings or their descendants + advisor.removeResourceLock(child); + }); advisor.useResourceLock(globalLockDescriptor, globalReadWriteLock); } else { @@ -94,8 +105,7 @@ private void forceDescendantExecutionModeRecursively(NodeExecutionAdvisor adviso } private boolean isReadOnly(Set exclusiveResources) { - return exclusiveResources.stream().allMatch( - exclusiveResource -> exclusiveResource.getLockMode() == ExclusiveResource.LockMode.READ); + return exclusiveResources.stream().allMatch(it -> it.getLockMode() == ExclusiveResource.LockMode.READ); } private Set getExclusiveResources(TestDescriptor testDescriptor) { diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java index b556240bc29..4634c4c94f5 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java @@ -18,6 +18,7 @@ import static org.junit.jupiter.api.DynamicTest.dynamicTest; import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT; import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD; +import static org.junit.jupiter.api.parallel.ResourceAccessMode.READ_WRITE; import static org.junit.jupiter.engine.Constants.DEFAULT_CLASSES_EXECUTION_MODE_PROPERTY_NAME; import static org.junit.jupiter.engine.Constants.DEFAULT_PARALLEL_EXECUTION_MODE; import static org.junit.jupiter.engine.Constants.PARALLEL_CONFIG_FIXED_MAX_POOL_SIZE_PROPERTY_NAME; @@ -25,6 +26,7 @@ import static org.junit.jupiter.engine.Constants.PARALLEL_CONFIG_STRATEGY_PROPERTY_NAME; import static org.junit.jupiter.engine.Constants.PARALLEL_EXECUTION_ENABLED_PROPERTY_NAME; import static org.junit.platform.commons.util.CollectionUtils.getOnlyElement; +import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_KEY; import static org.junit.platform.testkit.engine.EventConditions.container; import static org.junit.platform.testkit.engine.EventConditions.event; import static org.junit.platform.testkit.engine.EventConditions.finishedSuccessfully; @@ -65,6 +67,8 @@ import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.Isolated; import org.junit.jupiter.api.parallel.ResourceLock; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.junit.platform.engine.TestDescriptor; import org.junit.platform.engine.discovery.ClassSelector; import org.junit.platform.engine.discovery.DiscoverySelectors; @@ -73,6 +77,7 @@ import org.junit.platform.testkit.engine.EngineExecutionResults; import org.junit.platform.testkit.engine.EngineTestKit; import org.junit.platform.testkit.engine.Event; +import org.junit.platform.testkit.engine.Events; /** * @since 1.3 @@ -82,7 +87,7 @@ class ParallelExecutionIntegrationTests { @Test void successfulParallelTest(TestReporter reporter) { - var events = executeConcurrently(3, SuccessfulParallelTestCase.class); + var events = executeConcurrentlySuccessfully(3, SuccessfulParallelTestCase.class).list(); var startedTimestamps = getTimestampsFor(events, event(test(), started())); var finishedTimestamps = getTimestampsFor(events, event(test(), finishedSuccessfully())); @@ -98,13 +103,13 @@ void successfulParallelTest(TestReporter reporter) { @Test void failingTestWithoutLock() { - var events = executeConcurrently(3, FailingWithoutLockTestCase.class); + var events = executeConcurrently(3, FailingWithoutLockTestCase.class).list(); assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).hasSize(2); } @Test void successfulTestWithMethodLock() { - var events = executeConcurrently(3, SuccessfulWithMethodLockTestCase.class); + var events = executeConcurrentlySuccessfully(3, SuccessfulWithMethodLockTestCase.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(3); assertThat(ThreadReporter.getThreadNames(events)).hasSize(3); @@ -112,7 +117,7 @@ void successfulTestWithMethodLock() { @Test void successfulTestWithClassLock() { - var events = executeConcurrently(3, SuccessfulWithClassLockTestCase.class); + var events = executeConcurrentlySuccessfully(3, SuccessfulWithClassLockTestCase.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(3); assertThat(ThreadReporter.getThreadNames(events)).hasSize(1); @@ -120,7 +125,7 @@ void successfulTestWithClassLock() { @Test void testCaseWithFactory() { - var events = executeConcurrently(3, TestCaseWithTestFactory.class); + var events = executeConcurrentlySuccessfully(3, TestCaseWithTestFactory.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(3); assertThat(ThreadReporter.getThreadNames(events)).hasSize(1); @@ -133,7 +138,7 @@ void customContextClassLoader() { var smilingLoader = new URLClassLoader("(-:", new URL[0], ClassLoader.getSystemClassLoader()); currentThread.setContextClassLoader(smilingLoader); try { - var events = executeConcurrently(3, SuccessfulWithMethodLockTestCase.class); + var events = executeConcurrentlySuccessfully(3, SuccessfulWithMethodLockTestCase.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(3); assertThat(ThreadReporter.getThreadNames(events)).hasSize(3); @@ -146,7 +151,8 @@ void customContextClassLoader() { @RepeatedTest(10) void mixingClassAndMethodLevelLocks() { - var events = executeConcurrently(4, TestCaseWithSortedLocks.class, TestCaseWithUnsortedLocks.class); + var events = executeConcurrentlySuccessfully(4, TestCaseWithSortedLocks.class, + TestCaseWithUnsortedLocks.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(6); assertThat(ThreadReporter.getThreadNames(events).count()).isLessThanOrEqualTo(2); @@ -154,7 +160,7 @@ void mixingClassAndMethodLevelLocks() { @RepeatedTest(10) void locksOnNestedTests() { - var events = executeConcurrently(3, TestCaseWithNestedLocks.class); + var events = executeConcurrentlySuccessfully(3, TestCaseWithNestedLocks.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(6); assertThat(ThreadReporter.getThreadNames(events)).hasSize(1); @@ -162,7 +168,7 @@ void locksOnNestedTests() { @Test void afterHooksAreCalledAfterConcurrentDynamicTestsAreFinished() { - var events = executeConcurrently(3, ConcurrentDynamicTestCase.class); + var events = executeConcurrentlySuccessfully(3, ConcurrentDynamicTestCase.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(1); var timestampedEvents = ConcurrentDynamicTestCase.events; @@ -175,14 +181,14 @@ void afterHooksAreCalledAfterConcurrentDynamicTestsAreFinished() { */ @Test void threadInterruptedByUserCode() { - var events = executeConcurrently(3, InterruptedThreadTestCase.class); + var events = executeConcurrentlySuccessfully(3, InterruptedThreadTestCase.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(4); } @Test void executesTestTemplatesWithResourceLocksInSameThread() { - var events = executeConcurrently(2, ConcurrentTemplateTestCase.class); + var events = executeConcurrentlySuccessfully(2, ConcurrentTemplateTestCase.class).list(); assertThat(events.stream().filter(event(test(), finishedSuccessfully())::matches)).hasSize(10); assertThat(ThreadReporter.getThreadNames(events)).hasSize(1); @@ -228,30 +234,22 @@ void executesMethodsInParallelIfEnabledViaConfigurationParameter() { @Test void canRunTestsIsolatedFromEachOther() { - var events = executeConcurrently(2, IsolatedTestCase.class); - - assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).isEmpty(); + executeConcurrentlySuccessfully(2, IsolatedTestCase.class); } @Test void canRunTestsIsolatedFromEachOtherWithNestedCases() { - var events = executeConcurrently(4, NestedIsolatedTestCase.class); - - assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).isEmpty(); + executeConcurrentlySuccessfully(4, NestedIsolatedTestCase.class); } @Test void canRunTestsIsolatedFromEachOtherAcrossClasses() { - var events = executeConcurrently(4, IndependentClasses.A.class, IndependentClasses.B.class); - - assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).isEmpty(); + executeConcurrentlySuccessfully(4, IndependentClasses.A.class, IndependentClasses.B.class); } @RepeatedTest(10) void canRunTestsIsolatedFromEachOtherAcrossClassesWithOtherResourceLocks() { - var events = executeConcurrently(4, IndependentClasses.B.class, IndependentClasses.C.class); - - assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).isEmpty(); + executeConcurrentlySuccessfully(4, IndependentClasses.B.class, IndependentClasses.C.class); } @Test @@ -262,9 +260,8 @@ void runsIsolatedTestsLastToMaximizeParallelism() { ); Class[] testClasses = { IsolatedTestCase.class, SuccessfulParallelTestCase.class }; var events = executeWithFixedParallelism(3, configParams, testClasses) // - .allEvents(); - - assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).isEmpty(); + .allEvents() // + .assertStatistics(it -> it.failed(0)); List parallelTestMethodEvents = events.reportingEntryPublished() // .filter(e -> e.getTestDescriptor().getSource() // @@ -283,6 +280,15 @@ void runsIsolatedTestsLastToMaximizeParallelism() { assertThat(isolatedClassStart).isAfterOrEqualTo(parallelClassFinish); } + @ParameterizedTest + @ValueSource(classes = { IsolatedMethodFirstTestCase.class, IsolatedMethodLastTestCase.class, + IsolatedNestedMethodFirstTestCase.class, IsolatedNestedMethodLastTestCase.class }) + void canRunTestsIsolatedFromEachOtherWhenDeclaredOnMethodLevel(Class testClass) { + List events = executeConcurrentlySuccessfully(1, testClass).list(); + + assertThat(ThreadReporter.getThreadNames(events)).hasSize(1); + } + @Isolated("testing") static class IsolatedTestCase { static AtomicInteger sharedResource; @@ -355,6 +361,122 @@ void b() throws Exception { } } + @ExtendWith(ThreadReporter.class) + static class IsolatedMethodFirstTestCase { + + static AtomicInteger sharedResource; + static CountDownLatch countDownLatch; + + @BeforeAll + static void initialize() { + sharedResource = new AtomicInteger(); + countDownLatch = new CountDownLatch(2); + } + + @Test + @ResourceLock(value = GLOBAL_KEY, mode = READ_WRITE) // effectively @Isolated + void test1() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + + @Test + @ResourceLock(value = "b", mode = READ_WRITE) + void test2() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + } + + @ExtendWith(ThreadReporter.class) + static class IsolatedMethodLastTestCase { + + static AtomicInteger sharedResource; + static CountDownLatch countDownLatch; + + @BeforeAll + static void initialize() { + sharedResource = new AtomicInteger(); + countDownLatch = new CountDownLatch(2); + } + + @Test + @ResourceLock(value = "b", mode = READ_WRITE) + void test1() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + + @Test + @ResourceLock(value = GLOBAL_KEY, mode = READ_WRITE) // effectively @Isolated + void test2() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + } + + @ExtendWith(ThreadReporter.class) + static class IsolatedNestedMethodFirstTestCase { + + static AtomicInteger sharedResource; + static CountDownLatch countDownLatch; + + @BeforeAll + static void initialize() { + sharedResource = new AtomicInteger(); + countDownLatch = new CountDownLatch(2); + } + + @Nested + class Test1 { + + @Test + @ResourceLock(value = GLOBAL_KEY, mode = READ_WRITE) // effectively @Isolated + void test1() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + } + + @Nested + class Test2 { + + @Test + @ResourceLock(value = "b", mode = READ_WRITE) + void test2() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + } + } + + @ExtendWith(ThreadReporter.class) + static class IsolatedNestedMethodLastTestCase { + + static AtomicInteger sharedResource; + static CountDownLatch countDownLatch; + + @BeforeAll + static void initialize() { + sharedResource = new AtomicInteger(); + countDownLatch = new CountDownLatch(2); + } + + @Nested + class Test1 { + + @Test + @ResourceLock(value = "b", mode = READ_WRITE) + void test1() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + } + + @Nested + class Test2 { + + @Test + @ResourceLock(value = GLOBAL_KEY, mode = READ_WRITE) // effectively @Isolated + void test2() throws InterruptedException { + incrementBlockAndCheck(sharedResource, countDownLatch); + } + } + } + static class IndependentClasses { static AtomicInteger sharedResource = new AtomicInteger(); static CountDownLatch countDownLatch = new CountDownLatch(4); @@ -416,11 +538,21 @@ private List getTimestampsFor(List events, Condition cond // @formatter:on } - private List executeConcurrently(int parallelism, Class... testClasses) { + private Events executeConcurrentlySuccessfully(int parallelism, Class... testClasses) { + var events = executeConcurrently(parallelism, testClasses); + try { + return events.assertStatistics(it -> it.failed(0)); + } + catch (AssertionError error) { + events.debug(); + throw error; + } + } + + private Events executeConcurrently(int parallelism, Class... testClasses) { Map configParams = Map.of(DEFAULT_PARALLEL_EXECUTION_MODE, "concurrent"); return executeWithFixedParallelism(parallelism, configParams, testClasses) // - .allEvents() // - .list(); + .allEvents(); } private EngineExecutionResults executeWithFixedParallelism(int parallelism, Map configParams,