Skip to content

Commit

Permalink
Fix global read-write lock handling when not declared on top level
Browse files Browse the repository at this point in the history 
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)
  • Loading branch information
@marcphilipp
marcphilipp committed Oct 3, 2024
1 parent 9658fac commit ab94140
Show file tree
Hide file tree
Showing 4 changed files with 181 additions and 33 deletions.
4 changes: 3 additions & 1 deletion documentation/src/docs/asciidoc/release-notes/release-notes-5.11.2.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ on GitHub.
[[release-notes-5.11.2-junit-platform-bug-fixes]]
==== Bug Fixes

* ❓
* Fix regression in parallel execution that was introduced in 5.10.4/5.11.1 regarding
global read-write locks. When such a lock was declared on descendants of top-level nodes
in the test tree, such as Cucumber scenarios, test execution failed.

[[release-notes-5.11.2-junit-platform-deprecations-and-breaking-changes]]
==== Deprecations and Breaking Changes
Expand Down
4 changes: 4 additions & 0 deletions ...ne/src/main/java/org/junit/platform/engine/support/hierarchical/NodeExecutionAdvisor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
16 changes: 13 additions & 3 deletions ...m-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTreeWalker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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 {
Expand All @@ -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) {
Expand Down
190 changes: 161 additions & 29 deletions ...ava/org/junit/platform/engine/support/hierarchical/ParallelExecutionIntegrationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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()));
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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() //
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit ab94140

Please sign in to comment.