Relax constraints on orderer

junit-team · Jan 31, 2025 · 9b8c821 · 9b8c821
1 parent 00310ee
commit 9b8c821
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 40 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
@@ -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;
 
@@ -183,21 +184,19 @@ default Set<? extends TestDescriptor> getDescendants() {
 	 * 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.
+	 * 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<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);
+		List<TestDescriptor> copyOfChildren = new ArrayList<>(getChildren());
+		List<TestDescriptor> suggestedOrder = orderer.apply(new ArrayList<>(copyOfChildren));
+		copyOfChildren.sort(comparingInt(suggestedOrder::indexOf));
+		copyOfChildren.forEach(this::removeChild);
+		copyOfChildren.forEach(this::addChild);
 	}
 
 	/**

diff --git a/...ne/src/main/java/org/junit/platform/engine/support/descriptor/AbstractTestDescriptor.java b/...ne/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,11 @@ public void removeFromHierarchy() {
 	@Override
 	public void orderChildren(UnaryOperator<List<TestDescriptor>> orderer) {
 		Preconditions.notNull(orderer, "orderer must not be null");
-		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");
-		children.clear();
-		children.addAll(orderedChildren);
+		List<TestDescriptor> copyOfChildren = new ArrayList<>(this.children);
+		List<TestDescriptor> suggestedOrder = orderer.apply(new ArrayList<>(copyOfChildren));
+		copyOfChildren.sort(comparingInt(suggestedOrder::indexOf));
+		this.children.clear();
+		this.children.addAll(copyOfChildren);
 	}
 
 	@Override

diff --git a/...st/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java b/...st/java/org/junit/platform/engine/support/descriptor/TestDescriptorOrderChildrenTest.java
@@ -12,13 +12,11 @@
 
 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;
 
@@ -64,49 +62,62 @@ 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<TestDescriptor> children = new ArrayList<>(testDescriptor.getChildren());
+		assertThat(children).isEqualTo(List.of(//
+			childrenInOriginalOrder.get(0), //
+			childrenInOriginalOrder.get(2), //
+			childrenInOriginalOrder.get(1))//
+		);
 	}
 
 	@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<TestDescriptor> 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<TestDescriptor> children = new ArrayList<>(testDescriptor.getChildren());
+		assertThat(children).isEqualTo(List.of(//
+			childrenInOriginalOrder.get(0), //
+			childrenInOriginalOrder.get(2), //
+			childrenInOriginalOrder.get(1))//
+		);
 	}
 
 	@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<TestDescriptor> children = new ArrayList<>(testDescriptor.getChildren());
+		assertThat(children).isEqualTo(List.of(//
+			childrenInOriginalOrder.get(0), //
+			childrenInOriginalOrder.get(2), //
+			childrenInOriginalOrder.get(1))//
+		);
 	}
 }