Order test descriptor children in place

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<List<TestDescriptor>> orderer)`
+  method to order children in place
 
 [[release-notes-5.12.0-M1-junit-platform-bug-fixes]]
 ==== Bug Fixes

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<PARENT> parentTestDescriptorType,
 	protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor, Class<CHILD> matchingChildrenType,
 			Function<CHILD, WRAPPER> descriptorWrapperFactory, DescriptorWrapperOrderer descriptorWrapperOrderer) {
 
-		Set<? extends TestDescriptor> children = parentTestDescriptor.getChildren();
-
-		List<WRAPPER> matchingDescriptorWrappers = children.stream()//
+		List<WRAPPER> 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<TestDescriptor> nonMatchingTestDescriptors = children.stream()//
-					.filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))//
-					.collect(Collectors.toList());
-
-			// Make a local copy for later validation
-			Set<WRAPPER> 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<TestDescriptor> nonMatchingTestDescriptors = children.stream()//
+						.filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor))//
+						.collect(toList());
+
+				// Make a local copy for later validation
+				List<WRAPPER> 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<TestDescriptor> 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<TestDescriptor> 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.

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<? extends TestDescriptor> getDescendants() {
 	 */
 	void removeFromHierarchy();
 
+	/**
+	 * Order all children from this descriptor.
+	 *
+	 * <p>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.
+	 * <p>
+	 * 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<List<TestDescriptor>> orderer) {
+		Preconditions.notNull(orderer, "orderer must not be null");
+		Set<? extends TestDescriptor> children = getChildren();
+		Set<? extends TestDescriptor> 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);
+	}
+
 	/**
 	 * Determine if this descriptor is a <em>root</em> descriptor.
 	 *

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,61 @@ 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<TestDescriptor> 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");