Ensure @Nested classes are executed after sibling test methods (#4603)

In order to validate them, classpath scanning was changed to find them
in 5.13.0. However, that caused `@Nested` classes to be added to their
parent descriptors before their sibling test methods. This made the
order of execution in classes containing nested test classes dependent
on how they were discovered unless a `MethodOrderer` was configured in
which case `MethodOrderingVisitor` ensured test methods came before
`@Nested` test classes on the same level.

This commit changes `MethodOrderingVisitor` to also ensure this
ordering constraint in case no `MethodOrderer` is configured.

Resolves #4600.

Author: marcphilipp

documentation/src/docs/asciidoc/release-notes/release-notes-5.13.1.adoc

@@ -35,7 +35,13 @@ on GitHub.
 [[release-notes-5.13.1-junit-jupiter-bug-fixes]]
 ==== Bug Fixes
 
-* ❓
+* The 5.13.0 release introduced a regression regarding the execution order in test classes
+  containing both test methods and `@Nested` test classes. When classpath scanning was
+  used during test discovery -- for example, when resolving a package selector for a
+  `@Suite` class -- test methods in `@Nested` classes were executed _before_ test methods
+  in their enclosing class. This undesired change in behavior has now been reverted so
+  that tests in `@Nested` test classes are always executed _after_ tests in enclosing test
+  classes again.
 
 [[release-notes-5.13.1-junit-jupiter-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/MethodOrderingVisitor.java

@@ -10,12 +10,14 @@
 
 package org.junit.jupiter.engine.discovery;
 
+import static java.util.Comparator.comparing;
 import static org.junit.platform.commons.support.AnnotationSupport.findAnnotation;
 import static org.junit.platform.commons.support.AnnotationSupport.isAnnotated;
 
 import java.util.List;
 import java.util.Optional;
 import java.util.function.Consumer;
+import java.util.function.UnaryOperator;
 
 import org.junit.jupiter.api.MethodOrderer;
 import org.junit.jupiter.api.Order;
@@ -40,6 +42,9 @@ class MethodOrderingVisitor extends AbstractOrderingVisitor {
 	private final JupiterConfiguration configuration;
 	private final Condition<MethodBasedTestDescriptor> noOrderAnnotation;
 
+	// Not a static field to avoid initialization at build time for GraalVM
+	private final UnaryOperator<List<TestDescriptor>> methodsBeforeNestedClassesOrderer;
+
 	MethodOrderingVisitor(JupiterConfiguration configuration, DiscoveryIssueReporter issueReporter) {
 		super(issueReporter);
 		this.configuration = configuration;
@@ -51,6 +56,7 @@ class MethodOrderingVisitor extends AbstractOrderingVisitor {
 						.source(MethodSource.from(testDescriptor.getTestMethod())) //
 						.build();
 			});
+		this.methodsBeforeNestedClassesOrderer = createMethodsBeforeNestedClassesOrderer();
 	}
 
 	@Override
@@ -80,6 +86,7 @@ private void orderContainedMethods(ClassBasedTestDescriptor classBasedTestDescri
 
 	private void orderContainedMethods(ClassBasedTestDescriptor classBasedTestDescriptor, Class<?> testClass,
 			Optional<MethodOrderer> methodOrderer) {
+
 		DescriptorWrapperOrderer<?, DefaultMethodDescriptor> descriptorWrapperOrderer = createDescriptorWrapperOrderer(
 			testClass, methodOrderer);
 
@@ -89,6 +96,11 @@ private void orderContainedMethods(ClassBasedTestDescriptor classBasedTestDescri
 			DefaultMethodDescriptor::new, //
 			descriptorWrapperOrderer);
 
+		if (methodOrderer.isEmpty()) {
+			// If there is an orderer, this is ensured by the call above
+			classBasedTestDescriptor.orderChildren(methodsBeforeNestedClassesOrderer);
+		}
+
 		// Note: MethodOrderer#getDefaultExecutionMode() is guaranteed
 		// to be invoked after MethodOrderer#orderMethods().
 		methodOrderer //
@@ -126,4 +138,12 @@ private Optional<Consumer<MethodBasedTestDescriptor>> toValidationAction(Optiona
 		return Optional.of(noOrderAnnotation::check);
 	}
 
+	private static UnaryOperator<List<TestDescriptor>> createMethodsBeforeNestedClassesOrderer() {
+		var methodsFirst = comparing(MethodBasedTestDescriptor.class::isInstance).reversed();
+		return children -> {
+			children.sort(methodsFirst);
+			return children;
+		};
+	}
+
 }

jupiter-tests/src/test/java/org/junit/jupiter/engine/NestedTestClassesTests.java

@@ -13,28 +13,36 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.platform.engine.discovery.ClassNameFilter.includeClassNamePatterns;
 import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
 import static org.junit.platform.engine.discovery.DiscoverySelectors.selectMethod;
+import static org.junit.platform.engine.discovery.DiscoverySelectors.selectPackage;
 import static org.junit.platform.engine.discovery.DiscoverySelectors.selectUniqueId;
 import static org.junit.platform.launcher.LauncherConstants.CRITICAL_DISCOVERY_ISSUE_SEVERITY_PROPERTY_NAME;
 import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request;
 import static org.junit.platform.testkit.engine.EventConditions.finishedWithFailure;
 import static org.junit.platform.testkit.engine.TestExecutionResultConditions.message;
 
 import java.util.List;
+import java.util.function.Consumer;
+import java.util.regex.Pattern;
 
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Named;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.engine.NestedTestClassesTests.OuterClass.NestedClass;
 import org.junit.jupiter.engine.NestedTestClassesTests.OuterClass.NestedClass.RecursiveNestedClass;
 import org.junit.jupiter.engine.NestedTestClassesTests.OuterClass.NestedClass.RecursiveNestedSiblingClass;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.junit.platform.engine.DiscoveryIssue.Severity;
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.support.descriptor.ClassSource;
 import org.junit.platform.launcher.LauncherDiscoveryRequest;
+import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder;
 import org.junit.platform.testkit.engine.EngineExecutionResults;
 import org.junit.platform.testkit.engine.Events;
 
@@ -53,20 +61,36 @@ void nestedTestsAreCorrectlyDiscovered() {
 		assertEquals(5, engineDescriptor.getDescendants().size(), "# resolved test descriptors");
 	}
 
-	@Test
-	void nestedTestsAreExecuted() {
-		EngineExecutionResults executionResults = executeTestsForClass(TestCaseWithNesting.class);
-		Events containers = executionResults.containerEvents();
-		Events tests = executionResults.testEvents();
+	@ParameterizedTest(name = "{0}")
+	@MethodSource
+	void nestedTestsAreExecutedInTheRightOrder(Consumer<LauncherDiscoveryRequestBuilder> configurer) {
+		EngineExecutionResults executionResults = executeTests(configurer);
 
+		Events tests = executionResults.testEvents();
 		assertEquals(3, tests.started().count(), "# tests started");
 		assertEquals(2, tests.succeeded().count(), "# tests succeeded");
 		assertEquals(1, tests.failed().count(), "# tests failed");
 
+		assertThat(tests.started().map(it -> it.getTestDescriptor().getDisplayName())) //
+				.containsExactlyInAnyOrder("someTest()", "successful()", "failing()") //
+				.containsSubsequence("someTest()", "successful()") //
+				.containsSubsequence("someTest()", "failing()");
+
+		Events containers = executionResults.containerEvents();
 		assertEquals(3, containers.started().count(), "# containers started");
 		assertEquals(3, containers.finished().count(), "# containers finished");
 	}
 
+	static List<Named<Consumer<LauncherDiscoveryRequestBuilder>>> nestedTestsAreExecutedInTheRightOrder() {
+		return List.of( //
+			Named.of("class selector", request -> request //
+					.selectors(selectClass(TestCaseWithNesting.class))),
+			Named.of("package selector", request -> request //
+					.selectors(selectPackage(TestCaseWithNesting.class.getPackageName())) //
+					.filters(includeClassNamePatterns(Pattern.quote(TestCaseWithNesting.class.getName()) + ".*"))) //
+		);
+	}
+
 	@Test
 	void doublyNestedTestsAreCorrectlyDiscovered() {
 		LauncherDiscoveryRequest request = request().selectors(selectClass(TestCaseWithDoubleNesting.class)).build();