From 0ba9f1401ee8059ccce391adcce0766c1e19db0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Leonard=20Br=C3=BCnings?= Date: Tue, 12 Jan 2021 12:49:20 +0100 Subject: [PATCH] Allow parallel execution of child nodes if only read locks are acquired (#2515) --- .../release-notes/release-notes-5.8.0-M1.adoc | 3 +- .../support/hierarchical/NodeTreeWalker.java | 34 ++++-- .../NodeTreeWalkerIntegrationTests.java | 100 ++++++++++++++++++ 3 files changed, 128 insertions(+), 9 deletions(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M1.adoc index 43601da40a9..f40414c2fe2 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M1.adoc @@ -38,7 +38,8 @@ on GitHub. * Documented constant value of `ExclusiveResource.GLOBAL_KEY`. * Instances of `TestIdentifier` and `UniqueId` now retain less memory because they no longer store `String` representations of unique IDs. - +* Improvement of `ExclusiveResource` handling, if a `Node` has only read locks and no read-write locks, + then descendants are not forced into `SAME_THREAD` execution, and can run concurrently. [[release-notes-5.8.0-M1-junit-jupiter]] === JUnit Jupiter 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 3d09201c1c6..01a6ce3faab 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 @@ -57,21 +57,39 @@ private void walk(TestDescriptor globalLockDescriptor, TestDescriptor testDescri } else { Set allResources = new HashSet<>(exclusiveResources); - advisor.forceDescendantExecutionMode(testDescriptor, SAME_THREAD); - doForChildrenRecursively(testDescriptor, child -> { - allResources.addAll(getExclusiveResources(child)); - advisor.forceDescendantExecutionMode(child, SAME_THREAD); - }); + if (isReadOnly(allResources)) { + doForChildrenRecursively(testDescriptor, child -> allResources.addAll(getExclusiveResources(child))); + if (!isReadOnly(allResources)) { + forceDescendantExecutionModeRecursively(advisor, testDescriptor); + } + } + else { + advisor.forceDescendantExecutionMode(testDescriptor, SAME_THREAD); + doForChildrenRecursively(testDescriptor, child -> { + allResources.addAll(getExclusiveResources(child)); + advisor.forceDescendantExecutionMode(child, SAME_THREAD); + }); + } if (!globalLockDescriptor.equals(testDescriptor) && allResources.contains(GLOBAL_READ_WRITE)) { - advisor.forceDescendantExecutionMode(globalLockDescriptor, SAME_THREAD); - doForChildrenRecursively(globalLockDescriptor, - child -> advisor.forceDescendantExecutionMode(child, SAME_THREAD)); + forceDescendantExecutionModeRecursively(advisor, globalLockDescriptor); advisor.useResourceLock(globalLockDescriptor, globalReadWriteLock); } 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 boolean isReadOnly(Set exclusiveResources) { + return exclusiveResources.stream().allMatch( + exclusiveResource -> exclusiveResource.getLockMode() == ExclusiveResource.LockMode.READ); + } + private Set getExclusiveResources(TestDescriptor testDescriptor) { return NodeUtils.asNode(testDescriptor).getExclusiveResources(); } 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 88c397b59e1..7cb996d3e2d 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 @@ -15,6 +15,7 @@ import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass; import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ; import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_READ_WRITE; +import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.LockMode.READ; import static org.junit.platform.engine.support.hierarchical.ExclusiveResource.LockMode.READ_WRITE; import static org.junit.platform.engine.support.hierarchical.Node.ExecutionMode.SAME_THREAD; import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request; @@ -25,6 +26,7 @@ import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.ResourceAccessMode; import org.junit.jupiter.api.parallel.ResourceLock; import org.junit.jupiter.engine.JupiterTestEngine; import org.junit.platform.engine.TestDescriptor; @@ -54,6 +56,70 @@ void pullUpExclusiveChildResourcesToTestClass() { assertThat(advisor.getForcedExecutionMode(testMethodDescriptor)).contains(SAME_THREAD); } + @Test + void setsForceExecutionModeForChildrenWithWriteLocksOnClass() { + var engineDescriptor = discover(TestCaseWithResourceWriteLockOnClass.class); + + var advisor = nodeTreeWalker.walk(engineDescriptor); + + var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // + .isEqualTo(List.of(getReadWriteLock("a"))); + assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); + + var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testMethodDescriptor)).extracting(allLocks()).isEqualTo(List.of()); + assertThat(advisor.getForcedExecutionMode(testMethodDescriptor)).contains(SAME_THREAD); + } + + @Test + void doesntSetForceExecutionModeForChildrenWithReadLocksOnClass() { + var engineDescriptor = discover(TestCaseWithResourceReadLockOnClass.class); + + var advisor = nodeTreeWalker.walk(engineDescriptor); + + var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // + .isEqualTo(List.of(getReadLock("a"))); + assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); + + var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testMethodDescriptor)).extracting(allLocks()).isEqualTo(List.of()); + assertThat(advisor.getForcedExecutionMode(testMethodDescriptor)).isEmpty(); + } + + @Test + void setsForceExecutionModeForChildrenWithReadLocksOnClassAndWriteLockOnTest() { + var engineDescriptor = discover(TestCaseWithResourceReadLockOnClassAndWriteClockOnTest.class); + + var advisor = nodeTreeWalker.walk(engineDescriptor); + + var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // + .isEqualTo(List.of(getReadWriteLock("a"))); + assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); + + var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testMethodDescriptor)).extracting(allLocks()).isEqualTo(List.of()); + assertThat(advisor.getForcedExecutionMode(testMethodDescriptor)).contains(SAME_THREAD); + } + + @Test + void doesntSetForceExecutionModeForChildrenWithReadLocksOnClassAndReadLockOnTest() { + var engineDescriptor = discover(TestCaseWithResourceReadLockOnClassAndReadClockOnTest.class); + + var advisor = nodeTreeWalker.walk(engineDescriptor); + + var testClassDescriptor = getOnlyElement(engineDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testClassDescriptor)).extracting(allLocks()) // + .isEqualTo(List.of(getReadLock("a"), getReadLock("b"))); + assertThat(advisor.getForcedExecutionMode(testClassDescriptor)).isEmpty(); + + var testMethodDescriptor = getOnlyElement(testClassDescriptor.getChildren()); + assertThat(advisor.getResourceLock(testMethodDescriptor)).extracting(allLocks()).isEqualTo(List.of()); + assertThat(advisor.getForcedExecutionMode(testMethodDescriptor)).isEmpty(); + } + @Test void leavesResourceLockOnTestMethodWhenClassDoesNotUseResource() { var engineDescriptor = discover(TestCaseWithoutResourceLock.class); @@ -112,6 +178,10 @@ private Lock getReadWriteLock(String key) { return getLock(new ExclusiveResource(key, READ_WRITE)); } + private Lock getReadLock(String key) { + return getLock(new ExclusiveResource(key, READ)); + } + private Lock getLock(ExclusiveResource exclusiveResource) { return getOnlyElement(ResourceLockSupport.getLocks(lockManager.getLockForResource(exclusiveResource))); } @@ -154,4 +224,34 @@ void test() { } } } + + @ResourceLock("a") + static class TestCaseWithResourceWriteLockOnClass { + @Test + void test() { + } + } + + @ResourceLock(value = "a", mode = ResourceAccessMode.READ) + static class TestCaseWithResourceReadLockOnClass { + @Test + void test() { + } + } + + @ResourceLock(value = "a", mode = ResourceAccessMode.READ) + static class TestCaseWithResourceReadLockOnClassAndWriteClockOnTest { + @Test + @ResourceLock("a") + void test() { + } + } + + @ResourceLock(value = "a", mode = ResourceAccessMode.READ) + static class TestCaseWithResourceReadLockOnClassAndReadClockOnTest { + @Test + @ResourceLock(value = "b", mode = ResourceAccessMode.READ) + void test() { + } + } }