Skip to content

Commit

Permalink
Acquire global read lock in presence of other exclusive resources
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
marcphilipp committed May 15, 2021
1 parent 267ce62 commit e6d74ab
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 11 deletions.
Expand Up @@ -18,14 +18,17 @@ 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]]
=== JUnit Jupiter

==== Bug Fixes

* ❓
* Test classes annotated with `@ResourceLock` no longer run in parallel with `@Isolated`
ones.


[[release-notes-5.7.2-junit-vintage]]
Expand Down
Expand Up @@ -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<ExclusiveResource> exclusiveResources) {
Expand Down
Expand Up @@ -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());
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -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());
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Event> getEventsOfChildren(EngineExecutionResults results, TestDescriptor container) {
Expand Down

0 comments on commit e6d74ab

Please sign in to comment.