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.

Sign up for GitHub

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

Jump to bottom

Allow parallel execution of child nodes if only read locks are acquired #2515

Merged
merged 5 commits into from
Jan 12, 2021
Merged

Allow parallel execution of child nodes if only read locks are acquired #2515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f132c89
Allow parallel execution of child nodes if only read locks are acquired
leonard84 Jan 4, 2021
1c305c5
Accept suggestion
leonard84 Jan 7, 2021
0d86acc
Implement review
leonard84 Jan 7, 2021
a69adfd
Update release-notes-5.8.0-M1.adoc
leonard84 Jan 9, 2021
463465e
Merge branch 'main' into implement-2423
leonard84 Jan 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M1.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 26 additions & 8 deletions ...m-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,21 +57,39 @@ private void walk(TestDescriptor globalLockDescriptor, TestDescriptor testDescri
}
else {
Set<ExclusiveResource> 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));
leonard84 marked this conversation as resolved.
Show resolved Hide resolved
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<ExclusiveResource> exclusiveResources) {
return exclusiveResources.stream().allMatch(
exclusiveResource -> exclusiveResource.getLockMode() == ExclusiveResource.LockMode.READ);
}

private Set<ExclusiveResource> getExclusiveResources(TestDescriptor testDescriptor) {
return NodeUtils.asNode(testDescriptor).getExclusiveResources();
}
Expand Down
100 changes: 100 additions & 0 deletions ...t/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalkerIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)));
}
Expand Down Expand Up @@ -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() {
}
}
leonard84 marked this conversation as resolved.
Show resolved Hide resolved

@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() {
}
}
}