From 91529a4f6f62d115332a292db25349ae2d6a9a50 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Thu, 30 Jan 2025 19:22:54 +0100 Subject: [PATCH 01/24] Order test descriptors children in place Currently, it is not possible to order children without first removing all children and then adding them back in order. E.g: ```java var children = new ArrayList<>(testDescriptor.getChildren()); Collections.shuffle(children); children.forEach(testDescriptor::removeChild); children.forEach(testDescriptor::addChild); ``` By adding `orderChildren(UnaryOperator> orderer)` it now becomes possible to write: ```java testDescriptor.orderChildren(children -> { Collections.shuffle(children); return children; }); ``` --- .../discovery/AbstractOrderingVisitor.java | 93 +++++++++---------- .../junit/platform/engine/TestDescriptor.java | 28 ++++++ .../AbstractTestDescriptorTests.java | 42 ++++++++- 3 files changed, 112 insertions(+), 51 deletions(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java index e2da60a53aba..cd5355339ceb 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java @@ -11,14 +11,12 @@ package org.junit.jupiter.engine.discovery; import static java.util.stream.Collectors.toCollection; +import static java.util.stream.Collectors.toList; import java.util.ArrayList; -import java.util.LinkedHashSet; import java.util.List; -import java.util.Set; import java.util.function.Consumer; import java.util.function.Function; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor; @@ -60,9 +58,8 @@ protected void doWithMatchingDescriptor(Class parentTestDescriptorType, protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, Class matchingChildrenType, Function descriptorWrapperFactory, DescriptorWrapperOrderer descriptorWrapperOrderer) { - Set children = parentTestDescriptor.getChildren(); - - List matchingDescriptorWrappers = children.stream()// + List matchingDescriptorWrappers = parentTestDescriptor.getChildren()// + .stream()// .filter(matchingChildrenType::isInstance)// .map(matchingChildrenType::cast)// .map(descriptorWrapperFactory)// @@ -74,50 +71,48 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, } if (descriptorWrapperOrderer.canOrderWrappers()) { - List nonMatchingTestDescriptors = children.stream()// - .filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))// - .collect(Collectors.toList()); - - // Make a local copy for later validation - Set originalWrappers = new LinkedHashSet<>(matchingDescriptorWrappers); - - descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers); - - int difference = matchingDescriptorWrappers.size() - originalWrappers.size(); - if (difference > 0) { - descriptorWrapperOrderer.logDescriptorsAddedWarning(difference); - } - else if (difference < 0) { - descriptorWrapperOrderer.logDescriptorsRemovedWarning(difference); - } + parentTestDescriptor.orderChildren(children -> { + List nonMatchingTestDescriptors = children.stream()// + .filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))// + .collect(toList()); + + // Make a local copy for later validation + List originalWrappers = new ArrayList<>(matchingDescriptorWrappers); + + descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers); + + int difference = matchingDescriptorWrappers.size() - originalWrappers.size(); + if (difference > 0) { + descriptorWrapperOrderer.logDescriptorsAddedWarning(difference); + } + else if (difference < 0) { + descriptorWrapperOrderer.logDescriptorsRemovedWarning(difference); + } + + List orderedTestDescriptors = matchingDescriptorWrappers.stream()// + .filter(originalWrappers::contains)// + .map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor)// + .collect(toList()); + + // If we are ordering children of type ClassBasedTestDescriptor, that means we + // are ordering top-level classes or @Nested test classes. Thus, the + // nonMatchingTestDescriptors list is either empty (for top-level classes) or + // contains only local test methods (for @Nested classes) which must be executed + // before tests in @Nested test classes. So we add the test methods before adding + // the @Nested test classes. + if (matchingChildrenType == ClassBasedTestDescriptor.class) { + return Stream.concat(nonMatchingTestDescriptors.stream(), orderedTestDescriptors.stream())// + .collect(toList()); + } + // Otherwise, we add the ordered descriptors before the non-matching descriptors, + // which is the case when we are ordering test methods. In other words, local + // test methods always get added before @Nested test classes. + else { + return Stream.concat(orderedTestDescriptors.stream(), nonMatchingTestDescriptors.stream())// + .collect(toList()); + } + }); - Set orderedTestDescriptors = matchingDescriptorWrappers.stream()// - .filter(originalWrappers::contains)// - .map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor)// - .collect(toCollection(LinkedHashSet::new)); - - // There is currently no way to removeAll or addAll children at once, so we - // first remove them all and then add them all back. - Stream.concat(orderedTestDescriptors.stream(), nonMatchingTestDescriptors.stream())// - .forEach(parentTestDescriptor::removeChild); - - // If we are ordering children of type ClassBasedTestDescriptor, that means we - // are ordering top-level classes or @Nested test classes. Thus, the - // nonMatchingTestDescriptors list is either empty (for top-level classes) or - // contains only local test methods (for @Nested classes) which must be executed - // before tests in @Nested test classes. So we add the test methods before adding - // the @Nested test classes. - if (matchingChildrenType == ClassBasedTestDescriptor.class) { - Stream.concat(nonMatchingTestDescriptors.stream(), orderedTestDescriptors.stream())// - .forEach(parentTestDescriptor::addChild); - } - // Otherwise, we add the ordered descriptors before the non-matching descriptors, - // which is the case when we are ordering test methods. In other words, local - // test methods always get added before @Nested test classes. - else { - Stream.concat(orderedTestDescriptors.stream(), nonMatchingTestDescriptors.stream())// - .forEach(parentTestDescriptor::addChild); - } } // Recurse through the children in order to support ordering for @Nested test classes. diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index 0d3df349bffa..3a79b7087ee2 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -10,12 +10,16 @@ package org.junit.platform.engine; +import static org.apiguardian.api.API.Status.EXPERIMENTAL; import static org.apiguardian.api.API.Status.STABLE; +import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashSet; +import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.function.UnaryOperator; import org.apiguardian.api.API; import org.junit.platform.commons.util.Preconditions; @@ -172,6 +176,30 @@ default Set getDescendants() { */ void removeFromHierarchy(); + /** + * Order all children from this descriptor. + * + *

The {@code orderer} is provided a modifiable list of children in + * this test descriptor. Never {@code null}. The {@code orderer} may return + * the elements of list in any order but may not add or remove descriptors. + *

+ * If descriptors were added or removed this method must throw a + * {@link org.junit.platform.commons.JUnitException JUnitException} explaining + * that an {@code orderer} may not remove children from this descriptor. + * + * @param orderer a unary operator to order the children from this test descriptor. + */ + @API(since = "5.12", status = EXPERIMENTAL) + default void orderChildren(UnaryOperator> orderer) { + Preconditions.notNull(orderer, "orderer must not be null"); + Set children = getChildren(); + List orderedChildren = orderer.apply(new ArrayList<>(children)); + boolean unmodified = children.size() == orderedChildren.size() && children.containsAll(orderedChildren); + Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); + orderedChildren.forEach(this::removeChild); + orderedChildren.forEach(this::addChild); + } + /** * Determine if this descriptor is a root descriptor. * diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java index 5f4fe3011fb2..506e20444f9b 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java @@ -10,6 +10,7 @@ package org.junit.platform.engine.support.descriptor; +import static java.util.Comparator.comparing; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -38,6 +39,8 @@ class AbstractTestDescriptorTests { private GroupDescriptor group1; private GroupDescriptor group11; private LeafDescriptor leaf111; + private LeafDescriptor leaf11; + private LeafDescriptor leaf12; @BeforeEach void initTree() { @@ -49,8 +52,10 @@ void initTree() { group11 = new GroupDescriptor(UniqueId.root("group", "group1-1")); group1.addChild(group11); - group1.addChild(new LeafDescriptor(UniqueId.root("leaf", "leaf1-1"))); - group1.addChild(new LeafDescriptor(UniqueId.root("leaf", "leaf1-2"))); + leaf11 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-1")); + group1.addChild(leaf11); + leaf12 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-2")); + group1.addChild(leaf12); group2.addChild(new LeafDescriptor(UniqueId.root("leaf", "leaf2-1"))); @@ -78,6 +83,39 @@ void removeFromHierarchyClearsParentFromAllChildren() { assertTrue(formerChildren.stream().noneMatch(d -> d.getParent().isPresent())); } + @Test + void orderChildren() { + group1.orderChildren(children -> { + children.sort(comparing(TestDescriptor::getDisplayName).reversed()); + return children; + }); + // Makes the compiler happy + List children = new ArrayList<>(group1.getChildren()); + assertThat(children).containsExactly(leaf12, leaf11, group11); + } + + @Test + void orderChildrenRemovesDescriptor() { + var exception = assertThrows(JUnitException.class, () -> { + group1.orderChildren(children -> { + children.removeFirst(); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } + + @Test + void orderChildrenAddsDescriptor() { + var exception = assertThrows(JUnitException.class, () -> { + group1.orderChildren(children -> { + children.add(new LeafDescriptor(UniqueId.root("leaf", "leaf1-3"))); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } + @Test void setParentToOtherInstance() { TestDescriptor newEngine = new EngineDescriptor(UniqueId.forEngine("newEngine"), "newEngine"); From 1f151e6436a0649342edd837f7cdb5c48d87a05c Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Thu, 30 Jan 2025 19:35:06 +0100 Subject: [PATCH 02/24] Add test for replacement --- .../descriptor/AbstractTestDescriptorTests.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java index 506e20444f9b..8ee5f357f599 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java @@ -116,6 +116,17 @@ void orderChildrenAddsDescriptor() { assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); } + @Test + void orderChildrenReplacesDescriptor() { + var exception = assertThrows(JUnitException.class, () -> { + group1.orderChildren(children -> { + children.set(0, new LeafDescriptor(UniqueId.root("leaf", "leaf1-3"))); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } + @Test void setParentToOtherInstance() { TestDescriptor newEngine = new EngineDescriptor(UniqueId.forEngine("newEngine"), "newEngine"); From 96507be974bbbffd4d25fc3511bab62db2e2d5c8 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Thu, 30 Jan 2025 19:40:36 +0100 Subject: [PATCH 03/24] Update release notes --- .../docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc index ff501e489fcf..f201acf94140 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc @@ -23,6 +23,8 @@ JUnit repository on GitHub. [[release-notes-5.12.0-M1-junit-platform]] === JUnit Platform +* New `TestDescriptor.orderChildren(UnaryOperator> orderer)` + method to order children in place [[release-notes-5.12.0-M1-junit-platform-bug-fixes]] ==== Bug Fixes From 6fa81f4434886b532314ef94d93257b2cca0d664 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Thu, 30 Jan 2025 20:33:56 +0100 Subject: [PATCH 04/24] Check against duplication --- .../org/junit/platform/engine/TestDescriptor.java | 2 +- .../descriptor/AbstractTestDescriptorTests.java | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index 3a79b7087ee2..dba64e87a34f 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -193,7 +193,7 @@ default Set getDescendants() { default void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); Set children = getChildren(); - List orderedChildren = orderer.apply(new ArrayList<>(children)); + Set orderedChildren = new LinkedHashSet<>(orderer.apply(new ArrayList<>(children))); boolean unmodified = children.size() == orderedChildren.size() && children.containsAll(orderedChildren); Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); orderedChildren.forEach(this::removeChild); diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java index 8ee5f357f599..4b34d334a4af 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java @@ -127,6 +127,17 @@ void orderChildrenReplacesDescriptor() { assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); } + @Test + void orderChildrenDuplicatesDescriptor() { + var exception = assertThrows(JUnitException.class, () -> { + group1.orderChildren(children -> { + children.set(1, children.get(0)); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } + @Test void setParentToOtherInstance() { TestDescriptor newEngine = new EngineDescriptor(UniqueId.forEngine("newEngine"), "newEngine"); From eadab24d8457568994020e2748f5010d07859995 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Thu, 30 Jan 2025 20:36:57 +0100 Subject: [PATCH 05/24] Use Java 21 --- .../engine/support/descriptor/AbstractTestDescriptorTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java index 4b34d334a4af..48b52ff39843 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java @@ -131,7 +131,7 @@ void orderChildrenReplacesDescriptor() { void orderChildrenDuplicatesDescriptor() { var exception = assertThrows(JUnitException.class, () -> { group1.orderChildren(children -> { - children.set(1, children.get(0)); + children.set(1, children.getFirst()); return children; }); }); From fc6975153ed615099f2ae6cc3ae7090eacebf85e Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Thu, 30 Jan 2025 20:41:50 +0100 Subject: [PATCH 06/24] Simplify --- .../src/main/java/org/junit/platform/engine/TestDescriptor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index dba64e87a34f..855102721ab2 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -194,7 +194,7 @@ default void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); Set children = getChildren(); Set orderedChildren = new LinkedHashSet<>(orderer.apply(new ArrayList<>(children))); - boolean unmodified = children.size() == orderedChildren.size() && children.containsAll(orderedChildren); + boolean unmodified = children.equals(orderedChildren); Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); orderedChildren.forEach(this::removeChild); orderedChildren.forEach(this::addChild); From 00310ee2aff9f36d04e618f84af7e20a855ffab8 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Fri, 31 Jan 2025 13:00:05 +0100 Subject: [PATCH 07/24] Add optimized version of orderChildren to AbstractTestDescriptor --- .../descriptor/AbstractTestDescriptor.java | 13 ++ .../AbstractTestDescriptorTests.java | 69 ++-------- .../TestDescriptorOrderChildrenTest.java | 124 ++++++++++++++++++ .../descriptor/TestDescriptorTest.java | 93 +++++++++++++ 4 files changed, 238 insertions(+), 61 deletions(-) create mode 100644 platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java create mode 100644 platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java index e8bc7920792c..753d2a4ab582 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java @@ -13,10 +13,13 @@ import static java.util.Collections.emptySet; import static org.apiguardian.api.API.Status.STABLE; +import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashSet; +import java.util.List; import java.util.Optional; import java.util.Set; +import java.util.function.UnaryOperator; import org.apiguardian.api.API; import org.junit.platform.commons.util.Preconditions; @@ -147,6 +150,16 @@ public void removeFromHierarchy() { this.children.clear(); } + @Override + public void orderChildren(UnaryOperator> orderer) { + Preconditions.notNull(orderer, "orderer must not be null"); + Set orderedChildren = new LinkedHashSet<>(orderer.apply(new ArrayList<>(children))); + boolean unmodified = children.equals(orderedChildren); + Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); + children.clear(); + children.addAll(orderedChildren); + } + @Override public Optional findByUniqueId(UniqueId uniqueId) { Preconditions.notNull(uniqueId, "UniqueId must not be null"); diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java index 48b52ff39843..a134af0af348 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java @@ -10,7 +10,6 @@ package org.junit.platform.engine.support.descriptor; -import static java.util.Comparator.comparing; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -33,14 +32,12 @@ * * @since 1.0 */ -class AbstractTestDescriptorTests { +class AbstractTestDescriptorTests implements TestDescriptorOrderChildrenTest { private EngineDescriptor engineDescriptor; private GroupDescriptor group1; private GroupDescriptor group11; private LeafDescriptor leaf111; - private LeafDescriptor leaf11; - private LeafDescriptor leaf12; @BeforeEach void initTree() { @@ -52,9 +49,9 @@ void initTree() { group11 = new GroupDescriptor(UniqueId.root("group", "group1-1")); group1.addChild(group11); - leaf11 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-1")); + var leaf11 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-1")); group1.addChild(leaf11); - leaf12 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-2")); + var leaf12 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-2")); group1.addChild(leaf12); group2.addChild(new LeafDescriptor(UniqueId.root("leaf", "leaf2-1"))); @@ -63,6 +60,11 @@ void initTree() { group11.addChild(leaf111); } + @Override + public TestDescriptor createEmptyTestDescriptor() { + return new GroupDescriptor(UniqueId.root("group", "1")); + } + @Test void removeRootFromHierarchyFails() { var e = assertThrows(JUnitException.class, () -> engineDescriptor.removeFromHierarchy()); @@ -83,61 +85,6 @@ void removeFromHierarchyClearsParentFromAllChildren() { assertTrue(formerChildren.stream().noneMatch(d -> d.getParent().isPresent())); } - @Test - void orderChildren() { - group1.orderChildren(children -> { - children.sort(comparing(TestDescriptor::getDisplayName).reversed()); - return children; - }); - // Makes the compiler happy - List children = new ArrayList<>(group1.getChildren()); - assertThat(children).containsExactly(leaf12, leaf11, group11); - } - - @Test - void orderChildrenRemovesDescriptor() { - var exception = assertThrows(JUnitException.class, () -> { - group1.orderChildren(children -> { - children.removeFirst(); - return children; - }); - }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); - } - - @Test - void orderChildrenAddsDescriptor() { - var exception = assertThrows(JUnitException.class, () -> { - group1.orderChildren(children -> { - children.add(new LeafDescriptor(UniqueId.root("leaf", "leaf1-3"))); - return children; - }); - }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); - } - - @Test - void orderChildrenReplacesDescriptor() { - var exception = assertThrows(JUnitException.class, () -> { - group1.orderChildren(children -> { - children.set(0, new LeafDescriptor(UniqueId.root("leaf", "leaf1-3"))); - return children; - }); - }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); - } - - @Test - void orderChildrenDuplicatesDescriptor() { - var exception = assertThrows(JUnitException.class, () -> { - group1.orderChildren(children -> { - children.set(1, children.getFirst()); - return children; - }); - }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); - } - @Test void setParentToOtherInstance() { TestDescriptor newEngine = new EngineDescriptor(UniqueId.forEngine("newEngine"), "newEngine"); diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java new file mode 100644 index 000000000000..c91da524d028 --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java @@ -0,0 +1,124 @@ +/* + * Copyright 2015-2025 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.engine.support.descriptor; + +import static java.util.Comparator.comparing; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.junit.platform.commons.JUnitException; +import org.junit.platform.engine.TestDescriptor; +import org.junit.platform.engine.UniqueId; + +public interface TestDescriptorOrderChildrenTest { + + /** + * @return a test descriptor without any children. + */ + TestDescriptor createEmptyTestDescriptor(); + + default TestDescriptor createTestDescriptorWithChildren() { + var testDescriptor = createEmptyTestDescriptor(); + testDescriptor.addChild(new StubTestDescriptor(UniqueId.root("child", "1"))); + testDescriptor.addChild(new StubTestDescriptor(UniqueId.root("child", "2"))); + testDescriptor.addChild(new StubTestDescriptor(UniqueId.root("child", "3"))); + return testDescriptor; + } + + @Test + default void orderChildrenInReverseOrder() { + var testDescriptor = createTestDescriptorWithChildren(); + var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); + testDescriptor.orderChildren(children -> { + children.sort(comparing((TestDescriptor o) -> childrenInOriginalOrder.indexOf(o)).reversed()); + return children; + }); + List children = new ArrayList<>(testDescriptor.getChildren()); + assertThat(children).isEqualTo(childrenInOriginalOrder.reversed()); + } + + @Test + default void orderChildrenInSameOrder() { + var testDescriptor = createTestDescriptorWithChildren(); + var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); + testDescriptor.orderChildren(children -> { + children.sort(comparing(childrenInOriginalOrder::indexOf)); + return children; + }); + List children = new ArrayList<>(testDescriptor.getChildren()); + assertThat(children).isEqualTo(childrenInOriginalOrder); + } + + @Test + default void orderChildrenRemovesDescriptor() { + var testDescriptor = createTestDescriptorWithChildren(); + var exception = assertThrows(JUnitException.class, () -> { + testDescriptor.orderChildren(children -> { + children.removeFirst(); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } + + @Test + default void orderChildrenAddsDescriptor() { + var testDescriptor = createTestDescriptorWithChildren(); + var exception = assertThrows(JUnitException.class, () -> { + testDescriptor.orderChildren(children -> { + children.add(new StubTestDescriptor(UniqueId.root("extra", "extra1"))); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } + + @Test + default void orderChildrenReplacesDescriptor() { + var testDescriptor = createTestDescriptorWithChildren(); + var exception = assertThrows(JUnitException.class, () -> { + testDescriptor.orderChildren(children -> { + children.set(0, new StubTestDescriptor(UniqueId.root("replaced", "replaced1"))); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } + + @Test + default void orderChildrenDuplicatesDescriptor() { + var testDescriptor = createTestDescriptorWithChildren(); + var exception = assertThrows(JUnitException.class, () -> { + testDescriptor.orderChildren(children -> { + children.set(1, children.getFirst()); + return children; + }); + }); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + } +} + +class StubTestDescriptor extends AbstractTestDescriptor { + + StubTestDescriptor(UniqueId uniqueId) { + super(uniqueId, "stub: " + uniqueId); + } + + @Override + public Type getType() { + return Type.TEST; + } + +} diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java new file mode 100644 index 000000000000..7846b9def82d --- /dev/null +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java @@ -0,0 +1,93 @@ +/* + * Copyright 2015-2025 the original author or authors. + * + * All rights reserved. This program and the accompanying materials are + * made available under the terms of the Eclipse Public License v2.0 which + * accompanies this distribution and is available at + * + * https://www.eclipse.org/legal/epl-v20.html + */ + +package org.junit.platform.engine.support.descriptor; + +import java.util.LinkedHashSet; +import java.util.Optional; +import java.util.Set; + +import org.junit.platform.engine.TestDescriptor; +import org.junit.platform.engine.TestSource; +import org.junit.platform.engine.TestTag; +import org.junit.platform.engine.UniqueId; + +class TestDescriptorTest implements TestDescriptorOrderChildrenTest { + + @Override + public TestDescriptor createEmptyTestDescriptor() { + return new MinimalTestDescriptorImplementation(); + } + + private static class MinimalTestDescriptorImplementation implements TestDescriptor { + + private final LinkedHashSet children = new LinkedHashSet<>(); + + @Override + public UniqueId getUniqueId() { + return UniqueId.root("root", "value"); + } + + @Override + public String getDisplayName() { + return "TestDescriptorImplementation"; + } + + @Override + public Set getTags() { + return Set.of(); + } + + @Override + public Optional getSource() { + return Optional.empty(); + } + + @Override + public Optional getParent() { + return Optional.empty(); + } + + @Override + public void setParent(TestDescriptor parent) { + throw new UnsupportedOperationException("Not implemented"); + } + + @Override + public Set getChildren() { + return children; + } + + @Override + public void addChild(TestDescriptor descriptor) { + children.add(descriptor); + } + + @Override + public void removeChild(TestDescriptor descriptor) { + children.remove(descriptor); + } + + @Override + public void removeFromHierarchy() { + throw new UnsupportedOperationException("Not implemented"); + } + + @Override + public Type getType() { + return Type.CONTAINER; + } + + @Override + public Optional findByUniqueId(UniqueId uniqueId) { + throw new UnsupportedOperationException("Not implemented"); + } + } +} From 45bb7c8d128fa895b63d2ed637c3ef4b47a5686b Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Fri, 31 Jan 2025 13:46:31 +0100 Subject: [PATCH 08/24] Relax constraints on orderer --- .../junit/platform/engine/TestDescriptor.java | 24 +++--- .../descriptor/AbstractTestDescriptor.java | 12 +-- .../TestDescriptorOrderChildrenTest.java | 83 +++++++++++++------ 3 files changed, 76 insertions(+), 43 deletions(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index 855102721ab2..472702f672d1 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -10,6 +10,7 @@ package org.junit.platform.engine; +import static java.util.Comparator.comparingInt; import static org.apiguardian.api.API.Status.EXPERIMENTAL; import static org.apiguardian.api.API.Status.STABLE; @@ -179,25 +180,24 @@ default Set getDescendants() { /** * Order all children from this descriptor. * - *

The {@code orderer} is provided a modifiable list of children in - * this test descriptor. Never {@code null}. The {@code orderer} may return - * the elements of list in any order but may not add or remove descriptors. + *

The {@code orderer} is provided a modifiable list of child test descriptors in + * this test descriptor. Never {@code null}. The {@code orderer} must return + * a list of descriptors in any order, never {@code null}. *

- * If descriptors were added or removed this method must throw a - * {@link org.junit.platform.commons.JUnitException JUnitException} explaining - * that an {@code orderer} may not remove children from this descriptor. + * If descriptors were added, these descriptors will be ignored. If descriptors + * are removed, the original descriptors will be retained in arbitrary order. * * @param orderer a unary operator to order the children from this test descriptor. */ @API(since = "5.12", status = EXPERIMENTAL) default void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); - Set children = getChildren(); - Set orderedChildren = new LinkedHashSet<>(orderer.apply(new ArrayList<>(children))); - boolean unmodified = children.equals(orderedChildren); - Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); - orderedChildren.forEach(this::removeChild); - orderedChildren.forEach(this::addChild); + List copyOfChildren = new ArrayList<>(getChildren()); + List suggestedOrder = orderer.apply(new ArrayList<>(copyOfChildren)); + Preconditions.notNull(suggestedOrder, "orderer may not return null"); + copyOfChildren.sort(comparingInt(suggestedOrder::indexOf)); + copyOfChildren.forEach(this::removeChild); + copyOfChildren.forEach(this::addChild); } /** diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java index 753d2a4ab582..bf936b398444 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java @@ -11,6 +11,7 @@ package org.junit.platform.engine.support.descriptor; import static java.util.Collections.emptySet; +import static java.util.Comparator.comparingInt; import static org.apiguardian.api.API.Status.STABLE; import java.util.ArrayList; @@ -153,11 +154,12 @@ public void removeFromHierarchy() { @Override public void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); - Set orderedChildren = new LinkedHashSet<>(orderer.apply(new ArrayList<>(children))); - boolean unmodified = children.equals(orderedChildren); - Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); - children.clear(); - children.addAll(orderedChildren); + List copyOfChildren = new ArrayList<>(this.children); + List suggestedOrder = orderer.apply(new ArrayList<>(copyOfChildren)); + Preconditions.notNull(suggestedOrder, "orderer may not return null"); + copyOfChildren.sort(comparingInt(suggestedOrder::indexOf)); + this.children.clear(); + this.children.addAll(copyOfChildren); } @Override diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java index c91da524d028..6c475f0ccbc2 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java @@ -10,6 +10,7 @@ package org.junit.platform.engine.support.descriptor; +import static java.util.Collections.emptyList; import static java.util.Comparator.comparing; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -18,7 +19,7 @@ import java.util.List; import org.junit.jupiter.api.Test; -import org.junit.platform.commons.JUnitException; +import org.junit.platform.commons.PreconditionViolationException; import org.junit.platform.engine.TestDescriptor; import org.junit.platform.engine.UniqueId; @@ -31,9 +32,9 @@ public interface TestDescriptorOrderChildrenTest { default TestDescriptor createTestDescriptorWithChildren() { var testDescriptor = createEmptyTestDescriptor(); + testDescriptor.addChild(new StubTestDescriptor(UniqueId.root("child", "0"))); testDescriptor.addChild(new StubTestDescriptor(UniqueId.root("child", "1"))); testDescriptor.addChild(new StubTestDescriptor(UniqueId.root("child", "2"))); - testDescriptor.addChild(new StubTestDescriptor(UniqueId.root("child", "3"))); return testDescriptor; } @@ -49,6 +50,15 @@ default void orderChildrenInReverseOrder() { assertThat(children).isEqualTo(childrenInOriginalOrder.reversed()); } + @Test + default void orderChildrenEmptyList() { + var testDescriptor = createTestDescriptorWithChildren(); + var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); + testDescriptor.orderChildren(children -> emptyList()); + List children = new ArrayList<>(testDescriptor.getChildren()); + assertThat(children).isEqualTo(childrenInOriginalOrder); + } + @Test default void orderChildrenInSameOrder() { var testDescriptor = createTestDescriptorWithChildren(); @@ -64,49 +74,70 @@ default void orderChildrenInSameOrder() { @Test default void orderChildrenRemovesDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var exception = assertThrows(JUnitException.class, () -> { - testDescriptor.orderChildren(children -> { - children.removeFirst(); - return children; - }); + var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); + testDescriptor.orderChildren(children -> { + children.remove(1); + return children; }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + List children = new ArrayList<>(testDescriptor.getChildren()); + assertThat(children).isEqualTo(List.of(// + childrenInOriginalOrder.get(1), // + childrenInOriginalOrder.get(0), // + childrenInOriginalOrder.get(2))// + ); } @Test default void orderChildrenAddsDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var exception = assertThrows(JUnitException.class, () -> { - testDescriptor.orderChildren(children -> { - children.add(new StubTestDescriptor(UniqueId.root("extra", "extra1"))); - return children; - }); + var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); + testDescriptor.orderChildren(children -> { + children.add(1, new StubTestDescriptor(UniqueId.root("extra", "extra1"))); + return children; }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + List children = new ArrayList<>(testDescriptor.getChildren()); + assertThat(children).isEqualTo(childrenInOriginalOrder); } @Test default void orderChildrenReplacesDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var exception = assertThrows(JUnitException.class, () -> { - testDescriptor.orderChildren(children -> { - children.set(0, new StubTestDescriptor(UniqueId.root("replaced", "replaced1"))); - return children; - }); + var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); + + testDescriptor.orderChildren(children -> { + children.set(1, new StubTestDescriptor(UniqueId.root("replaced", "replaced1"))); + return children; }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + List children = new ArrayList<>(testDescriptor.getChildren()); + assertThat(children).isEqualTo(List.of(// + childrenInOriginalOrder.get(1), // + childrenInOriginalOrder.get(0), // + childrenInOriginalOrder.get(2))// + ); } @Test default void orderChildrenDuplicatesDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var exception = assertThrows(JUnitException.class, () -> { - testDescriptor.orderChildren(children -> { - children.set(1, children.getFirst()); - return children; - }); + var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); + testDescriptor.orderChildren(children -> { + children.add(1, children.getLast()); + return children; }); - assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); + List children = new ArrayList<>(testDescriptor.getChildren()); + assertThat(children).isEqualTo(List.of(// + childrenInOriginalOrder.get(0), // + childrenInOriginalOrder.get(2), // + childrenInOriginalOrder.get(1))// + ); + } + + @Test + default void orderChildrenOrdererReturnsNull() { + var testDescriptor = createTestDescriptorWithChildren(); + var exception = assertThrows(PreconditionViolationException.class, + () -> testDescriptor.orderChildren(children -> null)); + assertThat(exception).hasMessage("orderer may not return null"); } } From c8e7f92edf0c6bd2666523b8f98274fcf31cf233 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Fri, 31 Jan 2025 14:49:17 +0100 Subject: [PATCH 09/24] Optimize ordering --- .../org/junit/platform/engine/TestDescriptor.java | 15 +++++++++------ .../descriptor/AbstractTestDescriptor.java | 14 ++++++++------ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index 472702f672d1..e004322f3a94 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -10,7 +10,6 @@ package org.junit.platform.engine; -import static java.util.Comparator.comparingInt; import static org.apiguardian.api.API.Status.EXPERIMENTAL; import static org.apiguardian.api.API.Status.STABLE; @@ -192,12 +191,16 @@ default Set getDescendants() { @API(since = "5.12", status = EXPERIMENTAL) default void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); - List copyOfChildren = new ArrayList<>(getChildren()); - List suggestedOrder = orderer.apply(new ArrayList<>(copyOfChildren)); + Set originalChildren = getChildren(); + List suggestedOrder = orderer.apply(new ArrayList<>(originalChildren)); Preconditions.notNull(suggestedOrder, "orderer may not return null"); - copyOfChildren.sort(comparingInt(suggestedOrder::indexOf)); - copyOfChildren.forEach(this::removeChild); - copyOfChildren.forEach(this::addChild); + suggestedOrder.stream() // + .distinct() // + .filter(originalChildren::contains)// + .forEach(testDescriptor -> { + removeChild(testDescriptor); + addChild(testDescriptor); + }); } /** diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java index bf936b398444..290af90dbca3 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java @@ -11,7 +11,6 @@ package org.junit.platform.engine.support.descriptor; import static java.util.Collections.emptySet; -import static java.util.Comparator.comparingInt; import static org.apiguardian.api.API.Status.STABLE; import java.util.ArrayList; @@ -154,12 +153,15 @@ public void removeFromHierarchy() { @Override public void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); - List copyOfChildren = new ArrayList<>(this.children); - List suggestedOrder = orderer.apply(new ArrayList<>(copyOfChildren)); + List suggestedOrder = orderer.apply(new ArrayList<>(this.children)); Preconditions.notNull(suggestedOrder, "orderer may not return null"); - copyOfChildren.sort(comparingInt(suggestedOrder::indexOf)); - this.children.clear(); - this.children.addAll(copyOfChildren); + suggestedOrder.stream() // + .distinct() // + .filter(this.children::contains)// + .forEach(testDescriptor -> { + this.children.remove(testDescriptor); + this.children.add(testDescriptor); + }); } @Override From fbe5b33f3df7ff81d10067496805fc22670d117f Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Fri, 31 Jan 2025 14:52:52 +0100 Subject: [PATCH 10/24] Make MinimalTestDescriptorImplementation follow getChildren contract --- .../engine/support/descriptor/TestDescriptorTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java index 7846b9def82d..8a858e7be0ed 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java @@ -10,6 +10,7 @@ package org.junit.platform.engine.support.descriptor; +import java.util.Collections; import java.util.LinkedHashSet; import java.util.Optional; import java.util.Set; @@ -28,7 +29,7 @@ public TestDescriptor createEmptyTestDescriptor() { private static class MinimalTestDescriptorImplementation implements TestDescriptor { - private final LinkedHashSet children = new LinkedHashSet<>(); + private final Set children = Collections.synchronizedSet(new LinkedHashSet<>()); @Override public UniqueId getUniqueId() { @@ -62,7 +63,7 @@ public void setParent(TestDescriptor parent) { @Override public Set getChildren() { - return children; + return Collections.unmodifiableSet(children); } @Override From 2c76db4b1f175eb79135de827b646c35f95af4bc Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Fri, 31 Jan 2025 14:59:15 +0100 Subject: [PATCH 11/24] Verify children provided to order can be modified --- .../descriptor/TestDescriptorOrderChildrenTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java index 6c475f0ccbc2..0b5814c84276 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java @@ -17,6 +17,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import org.junit.jupiter.api.Test; import org.junit.platform.commons.PreconditionViolationException; @@ -139,6 +140,17 @@ default void orderChildrenOrdererReturnsNull() { () -> testDescriptor.orderChildren(children -> null)); assertThat(exception).hasMessage("orderer may not return null"); } + + @Test + default void orderChildrenProvidedChildrenAreModifiable() { + var testDescriptor = createTestDescriptorWithChildren(); + AtomicReference> childrenRef = new AtomicReference<>(); + testDescriptor.orderChildren(children -> { + childrenRef.set(children); + return children; + }); + assertThat(childrenRef.get()).isInstanceOf(ArrayList.class); + } } class StubTestDescriptor extends AbstractTestDescriptor { From 8df407bb9858caf2e4f2dc69ccc08ab02edb0577 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Mon, 3 Feb 2025 11:14:19 +0100 Subject: [PATCH 12/24] Forbid adding/removing children while ordering --- .../discovery/AbstractOrderingVisitor.java | 59 +++++++++++++------ .../junit/platform/engine/TestDescriptor.java | 7 +++ .../descriptor/AbstractTestDescriptor.java | 16 ++--- .../TestDescriptorOrderChildrenTest.java | 56 +++++++----------- 4 files changed, 77 insertions(+), 61 deletions(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java index cd5355339ceb..88d78bff79bb 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java @@ -14,6 +14,7 @@ import static java.util.stream.Collectors.toList; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; import java.util.function.Consumer; import java.util.function.Function; @@ -76,23 +77,10 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, .filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))// .collect(toList()); - // Make a local copy for later validation - List originalWrappers = new ArrayList<>(matchingDescriptorWrappers); - descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers); - int difference = matchingDescriptorWrappers.size() - originalWrappers.size(); - if (difference > 0) { - descriptorWrapperOrderer.logDescriptorsAddedWarning(difference); - } - else if (difference < 0) { - descriptorWrapperOrderer.logDescriptorsRemovedWarning(difference); - } - - List orderedTestDescriptors = matchingDescriptorWrappers.stream()// - .filter(originalWrappers::contains)// - .map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor)// - .collect(toList()); + Stream orderedTestDescriptors = matchingDescriptorWrappers.stream()// + .map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor); // If we are ordering children of type ClassBasedTestDescriptor, that means we // are ordering top-level classes or @Nested test classes. Thus, the @@ -101,18 +89,17 @@ else if (difference < 0) { // before tests in @Nested test classes. So we add the test methods before adding // the @Nested test classes. if (matchingChildrenType == ClassBasedTestDescriptor.class) { - return Stream.concat(nonMatchingTestDescriptors.stream(), orderedTestDescriptors.stream())// + return Stream.concat(nonMatchingTestDescriptors.stream(), orderedTestDescriptors)// .collect(toList()); } // Otherwise, we add the ordered descriptors before the non-matching descriptors, // which is the case when we are ordering test methods. In other words, local // test methods always get added before @Nested test classes. else { - return Stream.concat(orderedTestDescriptors.stream(), nonMatchingTestDescriptors.stream())// + return Stream.concat(orderedTestDescriptors, nonMatchingTestDescriptors.stream())// .collect(toList()); } }); - } // Recurse through the children in order to support ordering for @Nested test classes. @@ -162,7 +149,43 @@ private boolean canOrderWrappers() { } private void orderWrappers(List wrappers) { + // Make a local copy for later validation + List originalWrappers = new ArrayList<>(wrappers); + this.orderingAction.accept(wrappers); + + warnAboutAndRemoveAddedWrappers(originalWrappers, wrappers); + warnAboutAndReAddRemovedWrappers(originalWrappers, wrappers); + } + + private void warnAboutAndRemoveAddedWrappers(List originalWrappers, List wrappers) { + int numAddedWrappers = 0; + Iterator iterator = wrappers.iterator(); + while (iterator.hasNext()) { + // Avoid ClassCastException if a misbehaving ordered added a non-WRAPPER + Object wrapper = iterator.next(); + //noinspection SuspiciousMethodCalls + if (!originalWrappers.contains(wrapper)) { + numAddedWrappers++; + iterator.remove(); + } + } + if (numAddedWrappers > 0) { + logDescriptorsAddedWarning(numAddedWrappers); + } + } + + private void warnAboutAndReAddRemovedWrappers(List originalWrappers, List wrappers) { + int numRemovedWrappers = 0; + for (WRAPPER wrapper : originalWrappers) { + if (!wrappers.contains(wrapper)) { + wrappers.add(numRemovedWrappers, wrapper); + numRemovedWrappers++; + } + } + if (numRemovedWrappers > 0) { + logDescriptorsRemovedWarning(numRemovedWrappers); + } } private void logDescriptorsAddedWarning(int number) { diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index e004322f3a94..0b4963c5545a 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -194,6 +194,13 @@ default void orderChildren(UnaryOperator> orderer) { Set originalChildren = getChildren(); List suggestedOrder = orderer.apply(new ArrayList<>(originalChildren)); Preconditions.notNull(suggestedOrder, "orderer may not return null"); + + Set orderedChildren = new LinkedHashSet<>(suggestedOrder); + boolean unmodified = originalChildren.equals(orderedChildren); + Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); + Preconditions.condition(originalChildren.size() == suggestedOrder.size(), + "orderer may not add duplicate test descriptors"); + suggestedOrder.stream() // .distinct() // .filter(originalChildren::contains)// diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java index 290af90dbca3..d69653b72347 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java @@ -155,13 +155,15 @@ public void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); List suggestedOrder = orderer.apply(new ArrayList<>(this.children)); Preconditions.notNull(suggestedOrder, "orderer may not return null"); - suggestedOrder.stream() // - .distinct() // - .filter(this.children::contains)// - .forEach(testDescriptor -> { - this.children.remove(testDescriptor); - this.children.add(testDescriptor); - }); + + Set orderedChildren = new LinkedHashSet<>(suggestedOrder); + boolean unmodified = this.children.equals(orderedChildren); + Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); + Preconditions.condition(this.children.size() == suggestedOrder.size(), + "orderer may not add duplicate test descriptors"); + + this.children.clear(); + this.children.addAll(orderedChildren); } @Override diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java index 0b5814c84276..689d74f0419f 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java @@ -18,6 +18,7 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.UnaryOperator; import org.junit.jupiter.api.Test; import org.junit.platform.commons.PreconditionViolationException; @@ -54,10 +55,9 @@ default void orderChildrenInReverseOrder() { @Test default void orderChildrenEmptyList() { var testDescriptor = createTestDescriptorWithChildren(); - var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); - testDescriptor.orderChildren(children -> emptyList()); - List children = new ArrayList<>(testDescriptor.getChildren()); - assertThat(children).isEqualTo(childrenInOriginalOrder); + var exception = assertThrows(PreconditionViolationException.class, + () -> testDescriptor.orderChildren(children -> emptyList())); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); } @Test @@ -75,62 +75,46 @@ default void orderChildrenInSameOrder() { @Test default void orderChildrenRemovesDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); - testDescriptor.orderChildren(children -> { + UnaryOperator> orderer = children -> { children.remove(1); return children; - }); - List children = new ArrayList<>(testDescriptor.getChildren()); - assertThat(children).isEqualTo(List.of(// - childrenInOriginalOrder.get(1), // - childrenInOriginalOrder.get(0), // - childrenInOriginalOrder.get(2))// - ); + }; + var exception = assertThrows(PreconditionViolationException.class, () -> testDescriptor.orderChildren(orderer)); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); } @Test default void orderChildrenAddsDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); - testDescriptor.orderChildren(children -> { + UnaryOperator> orderer = children -> { children.add(1, new StubTestDescriptor(UniqueId.root("extra", "extra1"))); return children; - }); - List children = new ArrayList<>(testDescriptor.getChildren()); - assertThat(children).isEqualTo(childrenInOriginalOrder); + }; + var exception = assertThrows(PreconditionViolationException.class, () -> testDescriptor.orderChildren(orderer)); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); } @Test default void orderChildrenReplacesDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); - - testDescriptor.orderChildren(children -> { + UnaryOperator> orderer = children -> { children.set(1, new StubTestDescriptor(UniqueId.root("replaced", "replaced1"))); return children; - }); - List children = new ArrayList<>(testDescriptor.getChildren()); - assertThat(children).isEqualTo(List.of(// - childrenInOriginalOrder.get(1), // - childrenInOriginalOrder.get(0), // - childrenInOriginalOrder.get(2))// - ); + }; + var exception = assertThrows(PreconditionViolationException.class, () -> testDescriptor.orderChildren(orderer)); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); } @Test default void orderChildrenDuplicatesDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); - testDescriptor.orderChildren(children -> { + UnaryOperator> orderer = children -> { children.add(1, children.getLast()); return children; - }); - List children = new ArrayList<>(testDescriptor.getChildren()); - assertThat(children).isEqualTo(List.of(// - childrenInOriginalOrder.get(0), // - childrenInOriginalOrder.get(2), // - childrenInOriginalOrder.get(1))// - ); + }; + var exception = assertThrows(PreconditionViolationException.class, () -> testDescriptor.orderChildren(orderer)); + assertThat(exception).hasMessage("orderer may not add duplicate test descriptors"); } @Test From 348986e5b11ddec9f342ce6fc9bbc5b31842803a Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Mon, 3 Feb 2025 12:11:33 +0100 Subject: [PATCH 13/24] Rename test classes to adhere to naming convention --- .../engine/support/descriptor/AbstractTestDescriptorTests.java | 2 +- ...rChildrenTest.java => TestDescriptorOrderChildrenTests.java} | 2 +- .../{TestDescriptorTest.java => TestDescriptorTests.java} | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) rename platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/{TestDescriptorOrderChildrenTest.java => TestDescriptorOrderChildrenTests.java} (99%) rename platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/{TestDescriptorTest.java => TestDescriptorTests.java} (96%) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java index a134af0af348..d884b61695e5 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java @@ -32,7 +32,7 @@ * * @since 1.0 */ -class AbstractTestDescriptorTests implements TestDescriptorOrderChildrenTest { +class AbstractTestDescriptorTests implements TestDescriptorOrderChildrenTests { private EngineDescriptor engineDescriptor; private GroupDescriptor group1; diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTests.java similarity index 99% rename from platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java rename to platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTests.java index 689d74f0419f..bf07b9035b6f 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTests.java @@ -25,7 +25,7 @@ import org.junit.platform.engine.TestDescriptor; import org.junit.platform.engine.UniqueId; -public interface TestDescriptorOrderChildrenTest { +public interface TestDescriptorOrderChildrenTests { /** * @return a test descriptor without any children. diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTests.java similarity index 96% rename from platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java rename to platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTests.java index 8a858e7be0ed..2d7d96308ecd 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTest.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorTests.java @@ -20,7 +20,7 @@ import org.junit.platform.engine.TestTag; import org.junit.platform.engine.UniqueId; -class TestDescriptorTest implements TestDescriptorOrderChildrenTest { +class TestDescriptorTests implements TestDescriptorOrderChildrenTests { @Override public TestDescriptor createEmptyTestDescriptor() { From aa0329be66e660fce3683ceb625f6fd50f7d443c Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Mon, 3 Feb 2025 12:48:11 +0100 Subject: [PATCH 14/24] Add integration test for partial ordering --- .../engine/extension/OrderedMethodTests.java | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java index 2d859001066a..f1eb99d7d838 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java @@ -10,6 +10,7 @@ package org.junit.jupiter.engine.extension; +import static java.util.Comparator.comparing; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.DynamicTest.dynamicTest; import static org.junit.jupiter.api.MethodOrderer.Random.RANDOM_SEED_PROPERTY_NAME; @@ -270,7 +271,7 @@ void misbehavingMethodOrdererThatAddsElements(@TrackLogRecords LogRecordListener executeTestsInParallel(testClass).assertStatistics(stats -> stats.succeeded(2)); - assertThat(callSequence).containsExactlyInAnyOrder("test1()", "test2()"); + assertThat(callSequence).containsExactly("test1()", "test2()"); var expectedMessage = "MethodOrderer [" + MisbehavingByAdding.class.getName() + "] added 2 MethodDescriptor(s) for test class [" + testClass.getName() + "] which will be ignored."; @@ -282,9 +283,15 @@ void misbehavingMethodOrdererThatAddsElements(@TrackLogRecords LogRecordListener void misbehavingMethodOrdererThatRemovesElements(@TrackLogRecords LogRecordListener listener) { Class testClass = MisbehavingByRemovingTestCase.class; - executeTestsInParallel(testClass).assertStatistics(stats -> stats.succeeded(3)); + executeTestsInParallel(testClass).assertStatistics(stats -> stats.succeeded(4)); - assertThat(callSequence).containsExactlyInAnyOrder("test1()", "test2()", "test3()"); + assertThat(callSequence) // + .containsExactlyInAnyOrder("test1()", "test2()", "test3()", "test4()") // + .containsSubsequence("test3()", "test4()") // ordered in MisbehavingByRemoving + .containsSubsequence("test1()", "test3()") // removed item is re-added before ordered item + .containsSubsequence("test1()", "test4()") // removed item is re-added before ordered item + .containsSubsequence("test2()", "test3()") // removed item is re-added before ordered item + .containsSubsequence("test2()", "test4()");// removed item is re-added before ordered item var expectedMessage = "MethodOrderer [" + MisbehavingByRemoving.class.getName() + "] removed 2 MethodDescriptor(s) for test class [" + testClass.getName() @@ -640,11 +647,11 @@ void trackInvocations(TestInfo testInfo) { } @Test - void test1() { + void test2() { } @Test - void test2() { + void test1() { } } @@ -661,6 +668,10 @@ void trackInvocations(TestInfo testInfo) { void test1() { } + @Test + void test4() { + } + @Test void test2() { } @@ -698,6 +709,7 @@ static class MisbehavingByAdding implements MethodOrderer { @Override public void orderMethods(MethodOrdererContext context) { + context.getMethodDescriptors().sort(comparing(MethodDescriptor::getDisplayName)); context.getMethodDescriptors().add(mockMethodDescriptor()); context.getMethodDescriptors().add(mockMethodDescriptor()); } @@ -713,6 +725,7 @@ static class MisbehavingByRemoving implements MethodOrderer { @Override public void orderMethods(MethodOrdererContext context) { + context.getMethodDescriptors().sort(comparing(MethodDescriptor::getDisplayName)); context.getMethodDescriptors().remove(0); context.getMethodDescriptors().remove(0); } From 99ee376d3c7a27c95f71e653c157e9dcd303ce20 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Mon, 3 Feb 2025 12:50:39 +0100 Subject: [PATCH 15/24] Move release notes to 5.12.0-RC1 --- .../docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc | 2 -- .../docs/asciidoc/release-notes/release-notes-5.12.0-RC1.adoc | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc index 436edc15e1ce..cbc023e40a0c 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc @@ -38,8 +38,6 @@ JUnit repository on GitHub. [[release-notes-5.12.0-M1-junit-platform]] === JUnit Platform -* New `TestDescriptor.orderChildren(UnaryOperator> orderer)` - method to order children in place [[release-notes-5.12.0-M1-junit-platform-deprecations-and-breaking-changes]] ==== Deprecations and Breaking Changes diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-RC1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-RC1.adoc index 7ac0dada50ff..f11f281eceb3 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-RC1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-RC1.adoc @@ -26,7 +26,8 @@ JUnit repository on GitHub. [[release-notes-5.12.0-RC1-junit-platform-new-features-and-improvements]] ==== New Features and Improvements -* ❓ +* New `TestDescriptor.orderChildren(UnaryOperator> orderer)` + method to order children in place [[release-notes-5.12.0-RC1-junit-jupiter]] From 842d554ca1d8c20c3b3f5bfba7c81945a9429efb Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Mon, 3 Feb 2025 21:49:04 +0100 Subject: [PATCH 16/24] Use difference and distinct difference trick --- .../discovery/AbstractOrderingVisitor.java | 60 ++++++++----------- 1 file changed, 25 insertions(+), 35 deletions(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java index 88d78bff79bb..1e98016e9ced 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java @@ -14,8 +14,9 @@ import static java.util.stream.Collectors.toList; import java.util.ArrayList; -import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Stream; @@ -59,12 +60,12 @@ protected void doWithMatchingDescriptor(Class parentTestDescriptorType, protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, Class matchingChildrenType, Function descriptorWrapperFactory, DescriptorWrapperOrderer descriptorWrapperOrderer) { - List matchingDescriptorWrappers = parentTestDescriptor.getChildren()// + Set matchingDescriptorWrappers = parentTestDescriptor.getChildren()// .stream()// .filter(matchingChildrenType::isInstance)// .map(matchingChildrenType::cast)// .map(descriptorWrapperFactory)// - .collect(toCollection(ArrayList::new)); + .collect(toCollection(LinkedHashSet::new)); // If there are no children to order, abort early. if (matchingDescriptorWrappers.isEmpty()) { @@ -77,7 +78,7 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, .filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))// .collect(toList()); - descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers); + descriptorWrapperOrderer.orderWrappersWithOrderFrom(matchingDescriptorWrappers); Stream orderedTestDescriptors = matchingDescriptorWrappers.stream()// .map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor); @@ -148,44 +149,33 @@ private boolean canOrderWrappers() { return this.orderingAction != null; } - private void orderWrappers(List wrappers) { - // Make a local copy for later validation - List originalWrappers = new ArrayList<>(wrappers); + private void orderWrappersWithOrderFrom(Set wrappers) { + List orderedWrappers = new ArrayList<>(wrappers); + this.orderingAction.accept(orderedWrappers); + Set distinctOrderedWrappers = new LinkedHashSet<>(orderedWrappers); - this.orderingAction.accept(wrappers); - - warnAboutAndRemoveAddedWrappers(originalWrappers, wrappers); - warnAboutAndReAddRemovedWrappers(originalWrappers, wrappers); - } - - private void warnAboutAndRemoveAddedWrappers(List originalWrappers, List wrappers) { - int numAddedWrappers = 0; - Iterator iterator = wrappers.iterator(); - while (iterator.hasNext()) { - // Avoid ClassCastException if a misbehaving ordered added a non-WRAPPER - Object wrapper = iterator.next(); - //noinspection SuspiciousMethodCalls - if (!originalWrappers.contains(wrapper)) { - numAddedWrappers++; - iterator.remove(); - } + int difference = orderedWrappers.size() - wrappers.size(); + int distinctDifference = distinctOrderedWrappers.size() - wrappers.size(); + if (difference > 0) { // difference >= distinctDifference + logDescriptorsAddedWarning(difference); } - if (numAddedWrappers > 0) { - logDescriptorsAddedWarning(numAddedWrappers); + if (distinctDifference < 0) { // distinctDifference <= difference + logDescriptorsRemovedWarning(distinctDifference); } + + orderWrappersWithOrderFrom(wrappers, distinctOrderedWrappers); } - private void warnAboutAndReAddRemovedWrappers(List originalWrappers, List wrappers) { - int numRemovedWrappers = 0; - for (WRAPPER wrapper : originalWrappers) { - if (!wrappers.contains(wrapper)) { - wrappers.add(numRemovedWrappers, wrapper); - numRemovedWrappers++; + @SuppressWarnings("unchecked") + private void orderWrappersWithOrderFrom(Set wrappers, Set orderedWrappers) { + // Avoid ClassCastException if a misbehaving ordering action added a non-WRAPPER + for (Object wrapper : orderedWrappers) { + //noinspection SuspiciousMethodCalls + if (wrappers.remove(wrapper)) { + // guaranteed by wrappers.contains + wrappers.add((WRAPPER) wrapper); } } - if (numRemovedWrappers > 0) { - logDescriptorsRemovedWarning(numRemovedWrappers); - } } private void logDescriptorsAddedWarning(int number) { From 88680b3017c03f6146a76bc5062471d1e12c459f Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Mon, 3 Feb 2025 22:52:41 +0100 Subject: [PATCH 17/24] Fix method name --- .../jupiter/engine/discovery/AbstractOrderingVisitor.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java index 1e98016e9ced..012bc2e82f18 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java @@ -78,7 +78,7 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, .filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))// .collect(toList()); - descriptorWrapperOrderer.orderWrappersWithOrderFrom(matchingDescriptorWrappers); + descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers); Stream orderedTestDescriptors = matchingDescriptorWrappers.stream()// .map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor); @@ -149,7 +149,7 @@ private boolean canOrderWrappers() { return this.orderingAction != null; } - private void orderWrappersWithOrderFrom(Set wrappers) { + private void orderWrappers(Set wrappers) { List orderedWrappers = new ArrayList<>(wrappers); this.orderingAction.accept(orderedWrappers); Set distinctOrderedWrappers = new LinkedHashSet<>(orderedWrappers); From a96fcd61f82f48e7a3901bf88cb48859e914e377 Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Mon, 3 Feb 2025 22:54:14 +0100 Subject: [PATCH 18/24] Minimize diff AbstractTestDescriptorTests --- .../support/descriptor/AbstractTestDescriptorTests.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java index d884b61695e5..bfa105bf1168 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptorTests.java @@ -49,10 +49,8 @@ void initTree() { group11 = new GroupDescriptor(UniqueId.root("group", "group1-1")); group1.addChild(group11); - var leaf11 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-1")); - group1.addChild(leaf11); - var leaf12 = new LeafDescriptor(UniqueId.root("leaf", "leaf1-2")); - group1.addChild(leaf12); + group1.addChild(new LeafDescriptor(UniqueId.root("leaf", "leaf1-1"))); + group1.addChild(new LeafDescriptor(UniqueId.root("leaf", "leaf1-2"))); group2.addChild(new LeafDescriptor(UniqueId.root("leaf", "leaf2-1"))); From 011263184dd6d1659bfe6e2f3043c83b4e0a68bd Mon Sep 17 00:00:00 2001 From: "M.P. Korstanje" Date: Mon, 3 Feb 2025 23:14:37 +0100 Subject: [PATCH 19/24] Order wrappers without transferring between collections --- .../discovery/AbstractOrderingVisitor.java | 34 ++++---- .../engine/extension/OrderedMethodTests.java | 86 +++++++++++++++++++ 2 files changed, 103 insertions(+), 17 deletions(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java index 012bc2e82f18..6863baaaa9b8 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java @@ -10,13 +10,13 @@ package org.junit.jupiter.engine.discovery; -import static java.util.stream.Collectors.toCollection; +import static java.util.Comparator.comparing; import static java.util.stream.Collectors.toList; import java.util.ArrayList; -import java.util.LinkedHashSet; +import java.util.HashMap; import java.util.List; -import java.util.Set; +import java.util.Map; import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Stream; @@ -60,12 +60,12 @@ protected void doWithMatchingDescriptor(Class parentTestDescriptorType, protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, Class matchingChildrenType, Function descriptorWrapperFactory, DescriptorWrapperOrderer descriptorWrapperOrderer) { - Set matchingDescriptorWrappers = parentTestDescriptor.getChildren()// + List matchingDescriptorWrappers = parentTestDescriptor.getChildren()// .stream()// .filter(matchingChildrenType::isInstance)// .map(matchingChildrenType::cast)// .map(descriptorWrapperFactory)// - .collect(toCollection(LinkedHashSet::new)); + .collect(toList()); // If there are no children to order, abort early. if (matchingDescriptorWrappers.isEmpty()) { @@ -149,13 +149,13 @@ private boolean canOrderWrappers() { return this.orderingAction != null; } - private void orderWrappers(Set wrappers) { + private void orderWrappers(List wrappers) { List orderedWrappers = new ArrayList<>(wrappers); this.orderingAction.accept(orderedWrappers); - Set distinctOrderedWrappers = new LinkedHashSet<>(orderedWrappers); + Map distinctWrappersToIndex = distinctWrappersToIndex(orderedWrappers); int difference = orderedWrappers.size() - wrappers.size(); - int distinctDifference = distinctOrderedWrappers.size() - wrappers.size(); + int distinctDifference = distinctWrappersToIndex.size() - wrappers.size(); if (difference > 0) { // difference >= distinctDifference logDescriptorsAddedWarning(difference); } @@ -163,19 +163,19 @@ private void orderWrappers(Set wrappers) { logDescriptorsRemovedWarning(distinctDifference); } - orderWrappersWithOrderFrom(wrappers, distinctOrderedWrappers); + wrappers.sort(comparing(wrapper -> distinctWrappersToIndex.getOrDefault(wrapper, -1))); } - @SuppressWarnings("unchecked") - private void orderWrappersWithOrderFrom(Set wrappers, Set orderedWrappers) { - // Avoid ClassCastException if a misbehaving ordering action added a non-WRAPPER - for (Object wrapper : orderedWrappers) { - //noinspection SuspiciousMethodCalls - if (wrappers.remove(wrapper)) { - // guaranteed by wrappers.contains - wrappers.add((WRAPPER) wrapper); + private Map distinctWrappersToIndex(List wrappers) { + Map toIndex = new HashMap<>(); + for (int i = 0; i < wrappers.size(); i++) { + // Avoid ClassCastException if a misbehaving ordering action added a non-WRAPPER + Object wrapper = wrappers.get(i); + if (!toIndex.containsKey(wrapper)) { + toIndex.put(wrapper, i); } } + return toIndex; } private void logDescriptorsAddedWarning(int number) { diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java index f1eb99d7d838..12818d6686bc 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java @@ -20,10 +20,14 @@ import static org.junit.jupiter.engine.Constants.PARALLEL_EXECUTION_ENABLED_PROPERTY_NAME; import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass; +import java.lang.annotation.Annotation; +import java.lang.reflect.Method; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.logging.Level; import java.util.logging.LogRecord; @@ -279,6 +283,15 @@ void misbehavingMethodOrdererThatAddsElements(@TrackLogRecords LogRecordListener assertExpectedLogMessage(listener, expectedMessage); } + @Test + void misbehavingMethodOrdererThatImpersonatesElements() { + Class testClass = MisbehavingByImpersonatingTestCase.class; + + executeTestsInParallel(testClass).assertStatistics(stats -> stats.succeeded(2)); + + assertThat(callSequence).containsExactlyInAnyOrder("test1()", "test2()"); + } + @Test void misbehavingMethodOrdererThatRemovesElements(@TrackLogRecords LogRecordListener listener) { Class testClass = MisbehavingByRemovingTestCase.class; @@ -655,6 +668,24 @@ void test1() { } } + @SuppressWarnings("JUnitMalformedDeclaration") + @TestMethodOrder(MisbehavingByImpersonating.class) + static class MisbehavingByImpersonatingTestCase { + + @BeforeEach + void trackInvocations(TestInfo testInfo) { + callSequence.add(testInfo.getDisplayName()); + } + + @Test + void test2() { + } + + @Test + void test1() { + } + } + @SuppressWarnings("JUnitMalformedDeclaration") @TestMethodOrder(MisbehavingByRemoving.class) static class MisbehavingByRemovingTestCase { @@ -721,6 +752,61 @@ static T mockMethodDescriptor() { } + static class MisbehavingByImpersonating implements MethodOrderer { + + @Override + public void orderMethods(MethodOrdererContext context) { + context.getMethodDescriptors().sort(comparing(MethodDescriptor::getDisplayName)); + MethodDescriptor method1 = context.getMethodDescriptors().get(0); + MethodDescriptor method2 = context.getMethodDescriptors().get(1); + + context.getMethodDescriptors().set(0, createMethodDescriptorImpersonator(method1)); + context.getMethodDescriptors().set(1, createMethodDescriptorImpersonator(method2)); + } + + @SuppressWarnings("unchecked") + static T createMethodDescriptorImpersonator(MethodDescriptor method) { + MethodDescriptor stub = new MethodDescriptor() { + @Override + public Method getMethod() { + return null; + } + + @Override + public String getDisplayName() { + return null; + } + + @Override + public boolean isAnnotated(Class annotationType) { + return false; + } + + @Override + public Optional findAnnotation(Class annotationType) { + return null; + } + + @Override + public List findRepeatableAnnotations(Class annotationType) { + return null; + } + + @SuppressWarnings("EqualsWhichDoesntCheckParameterClass") + @Override + public boolean equals(Object obj) { + return method.equals(obj); + } + + @Override + public int hashCode() { + return method.hashCode(); + } + }; + return (T) stub; + } + } + static class MisbehavingByRemoving implements MethodOrderer { @Override From 5bcef5239af18d1fbcd24fdd7f219be0a4a7f9aa Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 4 Feb 2025 15:59:36 +0100 Subject: [PATCH 20/24] Ensure mutated list is mutable --- .../jupiter/engine/discovery/AbstractOrderingVisitor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java index 6863baaaa9b8..ec99407cfa23 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java @@ -11,6 +11,7 @@ package org.junit.jupiter.engine.discovery; import static java.util.Comparator.comparing; +import static java.util.stream.Collectors.toCollection; import static java.util.stream.Collectors.toList; import java.util.ArrayList; @@ -65,7 +66,7 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, .filter(matchingChildrenType::isInstance)// .map(matchingChildrenType::cast)// .map(descriptorWrapperFactory)// - .collect(toList()); + .collect(toCollection(ArrayList::new)); // If there are no children to order, abort early. if (matchingDescriptorWrappers.isEmpty()) { From f73616904a6e12b912f102bab89747a0c5e5504e Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 4 Feb 2025 15:59:55 +0100 Subject: [PATCH 21/24] Avoid intermediate list --- .../engine/discovery/AbstractOrderingVisitor.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java index ec99407cfa23..0c7147d987dd 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java @@ -75,9 +75,8 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, if (descriptorWrapperOrderer.canOrderWrappers()) { parentTestDescriptor.orderChildren(children -> { - List nonMatchingTestDescriptors = children.stream()// - .filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))// - .collect(toList()); + Stream nonMatchingTestDescriptors = children.stream()// + .filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor)); descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers); @@ -91,14 +90,14 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, // before tests in @Nested test classes. So we add the test methods before adding // the @Nested test classes. if (matchingChildrenType == ClassBasedTestDescriptor.class) { - return Stream.concat(nonMatchingTestDescriptors.stream(), orderedTestDescriptors)// + return Stream.concat(nonMatchingTestDescriptors, orderedTestDescriptors)// .collect(toList()); } // Otherwise, we add the ordered descriptors before the non-matching descriptors, // which is the case when we are ordering test methods. In other words, local // test methods always get added before @Nested test classes. else { - return Stream.concat(orderedTestDescriptors, nonMatchingTestDescriptors.stream())// + return Stream.concat(orderedTestDescriptors, nonMatchingTestDescriptors)// .collect(toList()); } }); From c520b80fe2692c9698902522dae179a7c07b3bc2 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 4 Feb 2025 16:01:25 +0100 Subject: [PATCH 22/24] Update Javadoc --- .../junit/platform/engine/TestDescriptor.java | 16 ++++++++-------- .../descriptor/AbstractTestDescriptor.java | 3 +++ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index 0b4963c5545a..3225a17b475b 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -177,16 +177,16 @@ default Set getDescendants() { void removeFromHierarchy(); /** - * Order all children from this descriptor. + * Order the children from this descriptor. * - *

The {@code orderer} is provided a modifiable list of child test descriptors in - * this test descriptor. Never {@code null}. The {@code orderer} must return - * a list of descriptors in any order, never {@code null}. - *

- * If descriptors were added, these descriptors will be ignored. If descriptors - * are removed, the original descriptors will be retained in arbitrary order. + *

The {@code orderer} is provided a modifiable list of child test + * descriptors in this test descriptor; never {@code null}. The + * {@code orderer} must return a list containing the same descriptors in any + * order; potentially the same list, but never {@code null}. If descriptors + * were added or removed, an exception is thrown. * - * @param orderer a unary operator to order the children from this test descriptor. + * @param orderer a unary operator to order the children of this test + * descriptor. */ @API(since = "5.12", status = EXPERIMENTAL) default void orderChildren(UnaryOperator> orderer) { diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java index d69653b72347..1cc690d7e69d 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java @@ -150,6 +150,9 @@ public void removeFromHierarchy() { this.children.clear(); } + /** + * {@inheritDoc} + */ @Override public void orderChildren(UnaryOperator> orderer) { Preconditions.notNull(orderer, "orderer must not be null"); From 1eb86b496cfc2b0ee1e11baaa7e243ce03cc70e6 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 4 Feb 2025 16:12:17 +0100 Subject: [PATCH 23/24] Document status quo in test --- .../junit/jupiter/engine/extension/OrderedMethodTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java index 12818d6686bc..0e245db1a2a1 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/OrderedMethodTests.java @@ -284,12 +284,14 @@ void misbehavingMethodOrdererThatAddsElements(@TrackLogRecords LogRecordListener } @Test - void misbehavingMethodOrdererThatImpersonatesElements() { + void misbehavingMethodOrdererThatImpersonatesElements(@TrackLogRecords LogRecordListener listener) { Class testClass = MisbehavingByImpersonatingTestCase.class; executeTestsInParallel(testClass).assertStatistics(stats -> stats.succeeded(2)); assertThat(callSequence).containsExactlyInAnyOrder("test1()", "test2()"); + + assertThat(listener.stream(Level.WARNING)).isEmpty(); } @Test From 56dfbfb5ed79f59dad86b25bb0a3090ad77d6035 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 4 Feb 2025 16:21:50 +0100 Subject: [PATCH 24/24] Use a single check/message for illegal modification --- .../main/java/org/junit/platform/engine/TestDescriptor.java | 5 ++--- .../engine/support/descriptor/AbstractTestDescriptor.java | 5 ++--- .../support/descriptor/TestDescriptorOrderChildrenTests.java | 3 +-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java index 3225a17b475b..6dfd169ab9f4 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java @@ -197,9 +197,8 @@ default void orderChildren(UnaryOperator> orderer) { Set orderedChildren = new LinkedHashSet<>(suggestedOrder); boolean unmodified = originalChildren.equals(orderedChildren); - Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); - Preconditions.condition(originalChildren.size() == suggestedOrder.size(), - "orderer may not add duplicate test descriptors"); + Preconditions.condition(unmodified && originalChildren.size() == suggestedOrder.size(), + "orderer may not add or remove test descriptors"); suggestedOrder.stream() // .distinct() // diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java index 1cc690d7e69d..336b6d686d95 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java @@ -161,9 +161,8 @@ public void orderChildren(UnaryOperator> orderer) { Set orderedChildren = new LinkedHashSet<>(suggestedOrder); boolean unmodified = this.children.equals(orderedChildren); - Preconditions.condition(unmodified, "orderer may not add or remove test descriptors"); - Preconditions.condition(this.children.size() == suggestedOrder.size(), - "orderer may not add duplicate test descriptors"); + Preconditions.condition(unmodified && this.children.size() == suggestedOrder.size(), + "orderer may not add or remove test descriptors"); this.children.clear(); this.children.addAll(orderedChildren); diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTests.java index bf07b9035b6f..55e0d80a20b9 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTests.java @@ -108,13 +108,12 @@ default void orderChildrenReplacesDescriptor() { @Test default void orderChildrenDuplicatesDescriptor() { var testDescriptor = createTestDescriptorWithChildren(); - var childrenInOriginalOrder = new ArrayList<>(testDescriptor.getChildren()); UnaryOperator> orderer = children -> { children.add(1, children.getLast()); return children; }; var exception = assertThrows(PreconditionViolationException.class, () -> testDescriptor.orderChildren(orderer)); - assertThat(exception).hasMessage("orderer may not add duplicate test descriptors"); + assertThat(exception).hasMessage("orderer may not add or remove test descriptors"); } @Test