Fix global read-write lock handling when not declared on top level

Prior to this change additional locks were not cleared from siblings when discovering the global read-write lock on a test descriptor which led to incompatible locks and caused test execution to fail. Fixes #4027. (cherry picked from commit 871e800)

Author: marcphilipp

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<ExecutionMode> getForcedExecutionMode(TestDescriptor testDescriptor) {
 return testDescriptor.getParent().flatMap(this::lookupExecutionModeForcedByAncestor);
 }

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<ExclusiveResource> 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<ExclusiveResource> exclusiveResources) {
-return exclusiveResources.stream().allMatch(
-exclusiveResource -> exclusiveResource.getLockMode() == ExclusiveResource.LockMode.READ);
+return exclusiveResources.stream().allMatch(it -> it.getLockMode() == ExclusiveResource.LockMode.READ);
 }
 
 private Set<ExclusiveResource> getExclusiveResources(TestDescriptor testDescriptor) {

platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java

@@ -18,13 +18,15 @@
 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;
 import static org.junit.jupiter.engine.Constants.PARALLEL_CONFIG_FIXED_PARALLELISM_PROPERTY_NAME;
 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,29 +103,29 @@ 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);
 }
 
 @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);
 }
 
 @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,23 +151,24 @@ 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);
 }
 
 @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);
 }
 
 @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<Event> 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<Event> 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<Instant> getTimestampsFor(List<Event> events, Condition<Event> cond
 // @formatter:on
 }
 
-private List<Event> 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<String, String> configParams = Map.of(DEFAULT_PARALLEL_EXECUTION_MODE, "concurrent");
 return executeWithFixedParallelism(parallelism, configParams, testClasses) //
-.allEvents() //
-.list();
+.allEvents();
 }
 
 private EngineExecutionResults executeWithFixedParallelism(int parallelism, Map<String, String> configParams,