From 8ae90b84cb2490406582935e1e1f6ce636e056c6 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Thu, 13 May 2021 18:21:34 +0200 Subject: [PATCH] Acquire global read lock in presence of other exclusive resources Prior to this commit, the global read lock was not acquired for test classes with `@ResourceLock` annotations causing `@Isolated` tests to run in parallel. Fixes #2605. --- .../release-notes/release-notes-5.7.2.adoc | 5 ++++- .../support/hierarchical/NodeTreeWalker.java | 11 +++++----- .../NodeTreeWalkerIntegrationTests.java | 10 +++++----- .../ParallelExecutionIntegrationTests.java | 20 +++++++++++++++++++ 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.7.2.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.7.2.adoc index 14bf051a4380..44b8253af7fd 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.7.2.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.7.2.adoc @@ -18,6 +18,8 @@ GitHub. * Method `getRootUrisForPackage()` in class `ClasspathScanner` now returns a valid list of class names when the package name is equal to the name of a module on the module path and when running on Java 8. +* Direct child descriptors of the engine descriptor now also acquire the global read lock + when they require other exclusive resources. [[release-notes-5.7.2-junit-jupiter]] @@ -25,7 +27,8 @@ GitHub. ==== Bug Fixes -* ❓ +* Test classes annotated with `@ResourceLock` no longer run in parallel with `@Isolated` + ones. [[release-notes-5.7.2-junit-vintage]] 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 4ba6c20b204e..b1403967eec7 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 @@ -74,15 +74,16 @@ private void walk(TestDescriptor globalLockDescriptor, TestDescriptor testDescri forceDescendantExecutionModeRecursively(advisor, globalLockDescriptor); advisor.useResourceLock(globalLockDescriptor, globalReadWriteLock); } + if (globalLockDescriptor.equals(testDescriptor) && !allResources.contains(GLOBAL_READ_WRITE)) { + allResources.add(GLOBAL_READ); + } advisor.useResourceLock(testDescriptor, lockManager.getLockForResources(allResources)); } } - private void forceDescendantExecutionModeRecursively(NodeExecutionAdvisor advisor, - TestDescriptor globalLockDescriptor) { - advisor.forceDescendantExecutionMode(globalLockDescriptor, SAME_THREAD); - doForChildrenRecursively(globalLockDescriptor, - child -> advisor.forceDescendantExecutionMode(child, SAME_THREAD)); + private void forceDescendantExecutionModeRecursively(NodeExecutionAdvisor advisor, TestDescriptor testDescriptor) { + advisor.forceDescendantExecutionMode(testDescriptor, SAME_THREAD); + doForChildrenRecursively(testDescriptor, child -> advisor.forceDescendantExecutionMode(child, SAME_THREAD)); } private boolean isReadOnly(Set exclusiveResources) { diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalkerIntegrationTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalkerIntegrationTests.java index db399188d59a..4f292c846870 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalkerIntegrationTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalkerIntegrationTests.java @@ -48,7 +48,7 @@ void pullUpExclusiveChildResourcesToTestClass() { var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // - .isEqualTo(List.of(getReadWriteLock("a"), getReadWriteLock("b"))); + .isEqualTo(List.of(getLock(GLOBAL_READ), getReadWriteLock("a"), getReadWriteLock("b"))); assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); @@ -64,7 +64,7 @@ void setsForceExecutionModeForChildrenWithWriteLocksOnClass() { var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // - .isEqualTo(List.of(getReadWriteLock("a"))); + .isEqualTo(List.of(getLock(GLOBAL_READ), getReadWriteLock("a"))); assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); @@ -80,7 +80,7 @@ void doesntSetForceExecutionModeForChildrenWithReadLocksOnClass() { var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // - .isEqualTo(List.of(getReadLock("a"))); + .isEqualTo(List.of(getLock(GLOBAL_READ), getReadLock("a"))); assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); @@ -96,7 +96,7 @@ void setsForceExecutionModeForChildrenWithReadLocksOnClassAndWriteLockOnTest() { var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // - .isEqualTo(List.of(getReadWriteLock("a"))); + .isEqualTo(List.of(getLock(GLOBAL_READ), getReadWriteLock("a"))); assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); @@ -112,7 +112,7 @@ void doesntSetForceExecutionModeForChildrenWithReadLocksOnClassAndReadLockOnTest var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // - .isEqualTo(List.of(getReadLock("a"), getReadLock("b"))); + .isEqualTo(List.of(getLock(GLOBAL_READ), getReadLock("a"), getReadLock("b"))); assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); 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 a7f5eec8d7bf..5facf032285f 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 @@ -243,6 +243,13 @@ void canRunTestsIsolatedFromEachOtherAcrossClasses() { assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).isEmpty(); } + @RepeatedTest(10) + void canRunTestsIsolatedFromEachOtherAcrossClassesWithOtherResourceLocks() { + var events = executeConcurrently(4, IndependentClasses.B.class, IndependentClasses.C.class); + + assertThat(events.stream().filter(event(test(), finishedWithFailure())::matches)).isEmpty(); + } + @Isolated("testing") static class IsolatedTestCase { static AtomicInteger sharedResource; @@ -343,6 +350,19 @@ void b() throws Exception { storeAndBlockAndCheck(sharedResource, countDownLatch); } } + + @ResourceLock("other") + static class C { + @Test + void a() throws Exception { + storeAndBlockAndCheck(sharedResource, countDownLatch); + } + + @Test + void b() throws Exception { + storeAndBlockAndCheck(sharedResource, countDownLatch); + } + } } private List getEventsOfChildren(EngineExecutionResults results, TestDescriptor container) {