From 4445078945933f8db916d76f78daf8d03aef1dd8 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Sun, 19 Oct 2025 15:03:11 +0200 Subject: [PATCH 1/6] Verify work is stolen in reverse order --- ...tHierarchicalTestExecutorServiceTests.java | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java index 306c7c9dbb36..918e550d2ef7 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java @@ -14,6 +14,7 @@ import static java.util.concurrent.Future.State.SUCCESS; import static java.util.function.Predicate.isEqual; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.fail; import static org.junit.platform.commons.test.PreconditionAssertions.assertPreconditionViolationFor; import static org.junit.platform.commons.util.ExceptionUtils.throwAsUncheckedException; @@ -32,6 +33,7 @@ import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; import java.util.concurrent.locks.ReentrantLock; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.stream.Stream; @@ -371,6 +373,86 @@ public void release() { .containsOnly(child2.executionThread); } + @Test + void executesChildrenInOrder() throws Exception { + service = new ConcurrentHierarchicalTestExecutorService(configuration(1, 1)); + + Executable behavior = () -> { + + }; + var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf1a").withLevel(2); + var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf1b").withLevel(2); + var leaf1c = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf1c").withLevel(2); + var leaf1d = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf1d").withLevel(2); + + var root = new TestTaskStub(ExecutionMode.SAME_THREAD, + () -> requiredService().invokeAll(List.of(leaf1a, leaf1b, leaf1c, leaf1d))) // + .withName("root").withLevel(1); + + service.submit(root).get(); + + assertThat(List.of(root, leaf1a, leaf1b, leaf1c, leaf1d)) // + .allSatisfy(TestTaskStub::assertExecutedSuccessfully); + + assertAll(() -> assertThat(leaf1a.startTime).isBeforeOrEqualTo(leaf1b.startTime), + () -> assertThat(leaf1b.startTime).isBeforeOrEqualTo(leaf1c.startTime), + () -> assertThat(leaf1c.startTime).isBeforeOrEqualTo(leaf1d.startTime)); + } + + @Test + void workIsStolenInReverseOrder() throws Exception { + service = new ConcurrentHierarchicalTestExecutorService(configuration(2, 2)); + + // Execute tasks pairwise + CyclicBarrier cyclicBarrier = new CyclicBarrier(2); + Executable behavior = cyclicBarrier::await; + + // With half of the leaves to executed normally + var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf1a").withLevel(2); + var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf1b").withLevel(2); + var leaf1c = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf1c").withLevel(2); + + // And half of the leaves to be stolen + var leaf2a = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf2a").withLevel(2); + var leaf2b = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf2b").withLevel(2); + var leaf2c = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + .withName("leaf2c").withLevel(2); + + var root = new TestTaskStub(ExecutionMode.SAME_THREAD, + () -> requiredService().invokeAll(List.of(leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c))) // + .withName("root").withLevel(1); + + service.submit(root).get(); + + assertThat(List.of(root, leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c)) // + .allSatisfy(TestTaskStub::assertExecutedSuccessfully); + + // If the last node must be stolen. + assertThat(leaf1a.executionThread).isNotEqualTo(leaf2c.executionThread); + // Then must follow that the last half of the nodes were stolen + assertThat(Stream.of(leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c)).extracting( + TestTaskStub::executionThread).containsExactly(leaf1a.executionThread, // + leaf1a.executionThread, // + leaf1a.executionThread, // + leaf2c.executionThread, // + leaf2c.executionThread, // + leaf2c.executionThread // + ); + assertAll(() -> assertThat(leaf1a.startTime).isBeforeOrEqualTo(leaf1b.startTime), + () -> assertThat(leaf1b.startTime).isBeforeOrEqualTo(leaf1c.startTime), + () -> assertThat(leaf2a.startTime).isAfterOrEqualTo(leaf2b.startTime), + () -> assertThat(leaf2b.startTime).isAfterOrEqualTo(leaf2c.startTime)); + } + private static ExclusiveResource exclusiveResource(LockMode lockMode) { return new ExclusiveResource("key", lockMode); } From 09b5d1cf7703f3bcd3518f315ed3afbb7853913b Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Sun, 19 Oct 2025 15:16:41 +0200 Subject: [PATCH 2/6] Ensure work is stolen in reverse order --- ...urrentHierarchicalTestExecutorService.java | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java index 7818c0fb48f7..ba667f58df43 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java @@ -277,7 +277,8 @@ private List forkConcurrentChildren(List ch Consumer isolatedTaskCollector, List sameThreadTasks) { List queueEntries = new ArrayList<>(children.size()); - for (TestTask child : children) { + for (int i = 0, childrenSize = children.size(); i < childrenSize; i++) { + TestTask child = children.get(i); if (requiresGlobalReadWriteLock(child)) { isolatedTaskCollector.accept(child); } @@ -285,7 +286,7 @@ else if (child.getExecutionMode() == SAME_THREAD) { sameThreadTasks.add(child); } else { - queueEntries.add(WorkQueue.Entry.create(child)); + queueEntries.add(WorkQueue.Entry.createIndexed(i, child)); } } @@ -570,12 +571,17 @@ boolean isEmpty() { return queue.isEmpty(); } - private record Entry(TestTask task, CompletableFuture<@Nullable Void> future, int level) + private record Entry(TestTask task, CompletableFuture<@Nullable Void> future, int level, int index) implements Comparable { static Entry create(TestTask task) { int level = task.getTestDescriptor().getUniqueId().getSegments().size(); - return new Entry(task, new CompletableFuture<>(), level); + return new Entry(task, new CompletableFuture<>(), level, 0); + } + + static Entry createIndexed(int index, TestTask task) { + int level = task.getTestDescriptor().getUniqueId().getSegments().size(); + return new Entry(task, new CompletableFuture<>(), level, index); } @SuppressWarnings("FutureReturnValueIgnored") @@ -593,10 +599,14 @@ static Entry create(TestTask task) { @Override public int compareTo(Entry that) { var result = Integer.compare(that.level, this.level); - if (result == 0) { - result = Boolean.compare(this.isContainer(), that.isContainer()); + if (result != 0) { + return result; } - return result; + result = Boolean.compare(this.isContainer(), that.isContainer()); + if (result != 0) { + return result; + } + return Integer.compare(that.index, this.index); } private boolean isContainer() { From 06418caae72218fe1054c73343ac77c3621583e2 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Sun, 19 Oct 2025 15:48:33 +0200 Subject: [PATCH 3/6] Fix prioritizesChildrenOfStartedContainers by swapping order --- ...currentHierarchicalTestExecutorServiceTests.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java index 918e550d2ef7..aeec49d88da0 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java @@ -232,13 +232,14 @@ void prioritizesChildrenOfStartedContainers() throws Exception { service = new ConcurrentHierarchicalTestExecutorService(configuration(2)); var leavesStarted = new CountDownLatch(2); - var leaf = new TestTaskStub(ExecutionMode.CONCURRENT, leavesStarted::countDown) // - .withName("leaf").withLevel(3); - var child1 = new TestTaskStub(ExecutionMode.CONCURRENT, () -> requiredService().submit(leaf).get()) // + + var child1 = new TestTaskStub(ExecutionMode.CONCURRENT, leavesStarted::await) // .withName("child1").withLevel(2); var child2 = new TestTaskStub(ExecutionMode.CONCURRENT, leavesStarted::countDown) // .withName("child2").withLevel(2); - var child3 = new TestTaskStub(ExecutionMode.SAME_THREAD, leavesStarted::await) // + var leaf = new TestTaskStub(ExecutionMode.CONCURRENT, leavesStarted::countDown) // + .withName("leaf").withLevel(3); + var child3 = new TestTaskStub(ExecutionMode.CONCURRENT, () -> requiredService().submit(leaf).get()) // .withName("child3").withLevel(2); var root = new TestTaskStub(ExecutionMode.SAME_THREAD, @@ -248,11 +249,11 @@ void prioritizesChildrenOfStartedContainers() throws Exception { service.submit(root).get(); root.assertExecutedSuccessfully(); - assertThat(List.of(child1, child2, child3)).allSatisfy(TestTaskStub::assertExecutedSuccessfully); + assertThat(List.of(child1, child2, leaf, child3)).allSatisfy(TestTaskStub::assertExecutedSuccessfully); leaf.assertExecutedSuccessfully(); assertThat(leaf.startTime).isBeforeOrEqualTo(child2.startTime); - assertThat(leaf.executionThread).isSameAs(child1.executionThread); + assertThat(leaf.executionThread).isSameAs(child3.executionThread); } @Test From ec3af8610f385da4ad21c0eda2331c7f4e955673 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Sun, 19 Oct 2025 15:21:53 +0200 Subject: [PATCH 4/6] Ensure task entry index is not sparse --- .../ConcurrentHierarchicalTestExecutorService.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java index ba667f58df43..88e2966ac8e8 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java @@ -276,9 +276,9 @@ void invokeAll(List testTasks) { private List forkConcurrentChildren(List children, Consumer isolatedTaskCollector, List sameThreadTasks) { + int index = 0; List queueEntries = new ArrayList<>(children.size()); - for (int i = 0, childrenSize = children.size(); i < childrenSize; i++) { - TestTask child = children.get(i); + for (TestTask child : children) { if (requiresGlobalReadWriteLock(child)) { isolatedTaskCollector.accept(child); } @@ -286,7 +286,7 @@ else if (child.getExecutionMode() == SAME_THREAD) { sameThreadTasks.add(child); } else { - queueEntries.add(WorkQueue.Entry.createIndexed(i, child)); + queueEntries.add(WorkQueue.Entry.createWithIndex(child, index++)); } } @@ -579,7 +579,7 @@ static Entry create(TestTask task) { return new Entry(task, new CompletableFuture<>(), level, 0); } - static Entry createIndexed(int index, TestTask task) { + static Entry createWithIndex(TestTask task, int index) { int level = task.getTestDescriptor().getUniqueId().getSegments().size(); return new Entry(task, new CompletableFuture<>(), level, index); } From a659ba7d74dc513f7bfed3831dff5f841789f778 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Sun, 19 Oct 2025 15:58:51 +0200 Subject: [PATCH 5/6] Typos and formatting --- ...ntHierarchicalTestExecutorServiceTests.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java index aeec49d88da0..14957004be0e 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java @@ -412,7 +412,7 @@ void workIsStolenInReverseOrder() throws Exception { CyclicBarrier cyclicBarrier = new CyclicBarrier(2); Executable behavior = cyclicBarrier::await; - // With half of the leaves to executed normally + // With half of the leaves to be executed normally var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // .withName("leaf1a").withLevel(2); var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // @@ -437,18 +437,16 @@ void workIsStolenInReverseOrder() throws Exception { assertThat(List.of(root, leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c)) // .allSatisfy(TestTaskStub::assertExecutedSuccessfully); - // If the last node must be stolen. + // If the last node was stolen. assertThat(leaf1a.executionThread).isNotEqualTo(leaf2c.executionThread); - // Then must follow that the last half of the nodes were stolen + // Then it must follow that the last half of the nodes were stolen assertThat(Stream.of(leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c)).extracting( - TestTaskStub::executionThread).containsExactly(leaf1a.executionThread, // - leaf1a.executionThread, // - leaf1a.executionThread, // - leaf2c.executionThread, // - leaf2c.executionThread, // - leaf2c.executionThread // + TestTaskStub::executionThread).containsExactly( // + leaf1a.executionThread, leaf1a.executionThread, leaf1a.executionThread, // + leaf2c.executionThread, leaf2c.executionThread, leaf2c.executionThread // ); - assertAll(() -> assertThat(leaf1a.startTime).isBeforeOrEqualTo(leaf1b.startTime), + assertAll( // + () -> assertThat(leaf1a.startTime).isBeforeOrEqualTo(leaf1b.startTime), () -> assertThat(leaf1b.startTime).isBeforeOrEqualTo(leaf1c.startTime), () -> assertThat(leaf2a.startTime).isAfterOrEqualTo(leaf2b.startTime), () -> assertThat(leaf2b.startTime).isAfterOrEqualTo(leaf2c.startTime)); From ebd724c66d1957aea2def70cc5477c26718d37de Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Fri, 24 Oct 2025 16:09:19 +0200 Subject: [PATCH 6/6] Polishing --- ...urrentHierarchicalTestExecutorService.java | 3 +- ...tHierarchicalTestExecutorServiceTests.java | 50 ++++++++++--------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java index 88e2966ac8e8..365f72d67d80 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorService.java @@ -575,8 +575,7 @@ private record Entry(TestTask task, CompletableFuture<@Nullable Void> future, in implements Comparable { static Entry create(TestTask task) { - int level = task.getTestDescriptor().getUniqueId().getSegments().size(); - return new Entry(task, new CompletableFuture<>(), level, 0); + return createWithIndex(task, 0); } static Entry createWithIndex(TestTask task, int index) { diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java index 14957004be0e..b996eb887352 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/ConcurrentHierarchicalTestExecutorServiceTests.java @@ -14,7 +14,6 @@ import static java.util.concurrent.Future.State.SUCCESS; import static java.util.function.Predicate.isEqual; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.fail; import static org.junit.platform.commons.test.PreconditionAssertions.assertPreconditionViolationFor; import static org.junit.platform.commons.util.ExceptionUtils.throwAsUncheckedException; @@ -374,20 +373,17 @@ public void release() { .containsOnly(child2.executionThread); } - @Test + @RepeatedTest(value = 100, failureThreshold = 1) void executesChildrenInOrder() throws Exception { service = new ConcurrentHierarchicalTestExecutorService(configuration(1, 1)); - Executable behavior = () -> { - - }; - var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + var leaf1a = new TestTaskStub(ExecutionMode.CONCURRENT) // .withName("leaf1a").withLevel(2); - var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + var leaf1b = new TestTaskStub(ExecutionMode.CONCURRENT) // .withName("leaf1b").withLevel(2); - var leaf1c = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + var leaf1c = new TestTaskStub(ExecutionMode.CONCURRENT) // .withName("leaf1c").withLevel(2); - var leaf1d = new TestTaskStub(ExecutionMode.CONCURRENT, behavior) // + var leaf1d = new TestTaskStub(ExecutionMode.CONCURRENT) // .withName("leaf1d").withLevel(2); var root = new TestTaskStub(ExecutionMode.SAME_THREAD, @@ -399,12 +395,12 @@ void executesChildrenInOrder() throws Exception { assertThat(List.of(root, leaf1a, leaf1b, leaf1c, leaf1d)) // .allSatisfy(TestTaskStub::assertExecutedSuccessfully); - assertAll(() -> assertThat(leaf1a.startTime).isBeforeOrEqualTo(leaf1b.startTime), - () -> assertThat(leaf1b.startTime).isBeforeOrEqualTo(leaf1c.startTime), - () -> assertThat(leaf1c.startTime).isBeforeOrEqualTo(leaf1d.startTime)); + assertThat(Stream.of(leaf1a, leaf1b, leaf1c, leaf1d)) // + .extracting(TestTaskStub::startTime) // + .isSorted(); } - @Test + @RepeatedTest(value = 100, failureThreshold = 1) void workIsStolenInReverseOrder() throws Exception { service = new ConcurrentHierarchicalTestExecutorService(configuration(2, 2)); @@ -440,16 +436,19 @@ void workIsStolenInReverseOrder() throws Exception { // If the last node was stolen. assertThat(leaf1a.executionThread).isNotEqualTo(leaf2c.executionThread); // Then it must follow that the last half of the nodes were stolen - assertThat(Stream.of(leaf1a, leaf1b, leaf1c, leaf2a, leaf2b, leaf2c)).extracting( - TestTaskStub::executionThread).containsExactly( // - leaf1a.executionThread, leaf1a.executionThread, leaf1a.executionThread, // - leaf2c.executionThread, leaf2c.executionThread, leaf2c.executionThread // - ); - assertAll( // - () -> assertThat(leaf1a.startTime).isBeforeOrEqualTo(leaf1b.startTime), - () -> assertThat(leaf1b.startTime).isBeforeOrEqualTo(leaf1c.startTime), - () -> assertThat(leaf2a.startTime).isAfterOrEqualTo(leaf2b.startTime), - () -> assertThat(leaf2b.startTime).isAfterOrEqualTo(leaf2c.startTime)); + assertThat(Stream.of(leaf1a, leaf1b, leaf1c)) // + .extracting(TestTaskStub::executionThread) // + .containsOnly(leaf1a.executionThread); + assertThat(Stream.of(leaf2a, leaf2b, leaf2c)) // + .extracting(TestTaskStub::executionThread) // + .containsOnly(leaf2c.executionThread); + + assertThat(Stream.of(leaf1a, leaf1b, leaf1c)) // + .extracting(TestTaskStub::startTime) // + .isSorted(); + assertThat(Stream.of(leaf2c, leaf2b, leaf2a)) // + .extracting(TestTaskStub::startTime) // + .isSorted(); } private static ExclusiveResource exclusiveResource(LockMode lockMode) { @@ -567,6 +566,11 @@ Thread executionThread() { return executionThread; } + @Nullable + Instant startTime() { + return startTime; + } + @Override public String toString() { return "%s @ %s".formatted(new ToStringBuilder(this).append("name", name), Integer.toHexString(hashCode()));