Move @TestFactory return type validation to discovery phase

documentation/src/docs/asciidoc/release-notes/release-notes-5.13.0-M3.adoc

@@ -45,7 +45,10 @@ repository on GitHub.
 [[release-notes-5.13.0-M3-junit-jupiter-new-features-and-improvements]]
 ==== New Features and Improvements
 
-* ❓
+* Return types of `@TestFactory` methods are now validated during discovery rather than
+  during execution. Invalid `@TestFactory` methods are no longer executed but reported as
+  discovery issues for consistency with how `@Test` and `@TestTemplate` methods are
+  handled.
 
 
 [[release-notes-5.13.0-M3-junit-vintage]]

documentation/src/docs/asciidoc/user-guide/writing-tests.adoc

@@ -2890,8 +2890,8 @@ or extensions between the execution of individual dynamic tests generated by the
 The following `DynamicTestsDemo` class demonstrates several examples of test factories
 and dynamic tests.
 
-The first method returns an invalid return type. Since an invalid return type cannot be
-detected at compile time, a `JUnitException` is thrown when it is detected at runtime.
+The first method returns an invalid return type and will cause a warning to be reported by
+JUnit during test discovery. Such methods are not executed.
 
 The next six methods demonstrate the generation of a `Collection`, `Iterable`, `Iterator`,
 array, or `Stream` of `DynamicTest` instances. Most of these examples do not really

documentation/src/test/java/example/DynamicTestsDemo.java

@@ -43,11 +43,12 @@ class DynamicTestsDemo {
 
 	private final Calculator calculator = new Calculator();
 
+	// This method will not be executed but produce a warning
+	@TestFactory
 	// end::user_guide[]
 	@Tag("exclude")
+	DynamicTest dummy() { return null; }
 	// tag::user_guide[]
-	// This will result in a JUnitException!
-	@TestFactory
 	List<String> dynamicTestsWithInvalidReturnType() {
 		return Arrays.asList("Hello");
 	}

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestFactoryTestDescriptor.java

@@ -35,7 +35,6 @@
 import org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.ReflectiveInterceptorCall;
 import org.junit.jupiter.engine.execution.JupiterEngineExecutionContext;
 import org.junit.platform.commons.JUnitException;
-import org.junit.platform.commons.PreconditionViolationException;
 import org.junit.platform.commons.util.CollectionUtils;
 import org.junit.platform.commons.util.Preconditions;
 import org.junit.platform.engine.TestDescriptor;
@@ -138,17 +137,11 @@ private Stream<DynamicNode> toDynamicNodeStream(Object testFactoryMethodResult)
 		if (testFactoryMethodResult instanceof DynamicNode) {
 			return Stream.of((DynamicNode) testFactoryMethodResult);
 		}
-		try {
-			return (Stream<DynamicNode>) CollectionUtils.toStream(testFactoryMethodResult);
-		}
-		catch (PreconditionViolationException ex) {
-			throw invalidReturnTypeException(ex);
-		}
+		return (Stream<DynamicNode>) CollectionUtils.toStream(testFactoryMethodResult);
 	}
 
 	private JUnitException invalidReturnTypeException(Throwable cause) {
-		String message = String.format(
-			"@TestFactory method [%s] must return a single %2$s or a Stream, Collection, Iterable, Iterator, or array of %2$s.",
+		String message = String.format("Objects produced by @TestFactory method '%s' must be of type %s.",
 			getTestMethod().toGenericString(), DynamicNode.class.getName());
 		return new JUnitException(message, cause);
 	}

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/IsTestFactoryMethod.java

@@ -12,8 +12,20 @@
 
 import static org.apiguardian.api.API.Status.INTERNAL;
 
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
+import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
+import java.lang.reflect.WildcardType;
+import java.util.Iterator;
+import java.util.stream.Stream;
+
 import org.apiguardian.api.API;
+import org.junit.jupiter.api.DynamicNode;
 import org.junit.jupiter.api.TestFactory;
+import org.junit.platform.engine.DiscoveryIssue;
+import org.junit.platform.engine.DiscoveryIssue.Severity;
+import org.junit.platform.engine.support.descriptor.MethodSource;
 import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
 
 /**
@@ -27,8 +39,76 @@
 @API(status = INTERNAL, since = "5.0")
 public class IsTestFactoryMethod extends IsTestableMethod {
 
+	private static final String EXPECTED_RETURN_TYPE_MESSAGE = String.format(
+		"must return a single %1$s or a Stream, Collection, Iterable, Iterator, or array of %1$s",
+		DynamicNode.class.getName());
+
 	public IsTestFactoryMethod(DiscoveryIssueReporter issueReporter) {
-		super(TestFactory.class, false, issueReporter);
+		super(TestFactory.class, IsTestFactoryMethod::hasCompatibleReturnType, issueReporter);
+	}
+
+	private static DiscoveryIssueReporter.Condition<Method> hasCompatibleReturnType(
+			Class<? extends Annotation> annotationType, DiscoveryIssueReporter issueReporter) {
+		return issueReporter.createReportingCondition(method -> isCompatible(method, issueReporter),
+			method -> createIssue(annotationType, method, EXPECTED_RETURN_TYPE_MESSAGE));
+	}
+
+	private static boolean isCompatible(Method method, DiscoveryIssueReporter issueReporter) {
+		Class<?> returnType = method.getReturnType();
+		if (DynamicNode.class.isAssignableFrom(returnType) || DynamicNode[].class.isAssignableFrom(returnType)) {
+			return true;
+		}
+		if (returnType == Object.class || returnType == Object[].class) {
+			issueReporter.reportIssue(createTooGenericReturnTypeIssue(method));
+			return true;
+		}
+		boolean validContainerType = Stream.class.isAssignableFrom(returnType) //
+				|| Iterable.class.isAssignableFrom(returnType) //
+				|| Iterator.class.isAssignableFrom(returnType);
+		return validContainerType && isCompatibleContainerType(method, issueReporter);
+	}
+
+	private static boolean isCompatibleContainerType(Method method, DiscoveryIssueReporter issueReporter) {
+		Type genericReturnType = method.getGenericReturnType();
+
+		if (genericReturnType instanceof ParameterizedType) {
+			Type[] typeArguments = ((ParameterizedType) genericReturnType).getActualTypeArguments();
+			if (typeArguments.length == 1) {
+				Type typeArgument = typeArguments[0];
+				if (typeArgument instanceof Class) {
+					// Stream<DynamicNode> etc.
+					return DynamicNode.class.isAssignableFrom((Class<?>) typeArgument);
+				}
+				if (typeArgument instanceof WildcardType) {
+					WildcardType wildcardType = (WildcardType) typeArgument;
+					Type[] upperBounds = wildcardType.getUpperBounds();
+					Type[] lowerBounds = wildcardType.getLowerBounds();
+					if (upperBounds.length == 1 && lowerBounds.length == 0 && upperBounds[0] instanceof Class) {
+						Class<?> upperBound = (Class<?>) upperBounds[0];
+						if (Object.class.equals(upperBound)) { // Stream<?> etc.
+							issueReporter.reportIssue(createTooGenericReturnTypeIssue(method));
+							return true;
+						}
+						// Stream<? extends DynamicNode> etc.
+						return DynamicNode.class.isAssignableFrom(upperBound);
+					}
+				}
+			}
+			return false;
+		}
+
+		// Raw Stream etc. without type argument
+		issueReporter.reportIssue(createTooGenericReturnTypeIssue(method));
+		return true;
+	}
+
+	private static DiscoveryIssue.Builder createTooGenericReturnTypeIssue(Method method) {
+		String message = String.format(
+			"The declared return type of @TestFactory method '%s' does not support static validation. It "
+					+ EXPECTED_RETURN_TYPE_MESSAGE + ".",
+			method.toGenericString());
+		return DiscoveryIssue.builder(Severity.INFO, message) //
+				.source(MethodSource.from(method));
 	}
 
 }

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/IsTestMethod.java

@@ -25,7 +25,7 @@
 public class IsTestMethod extends IsTestableMethod {
 
 	public IsTestMethod(DiscoveryIssueReporter issueReporter) {
-		super(Test.class, true, issueReporter);
+		super(Test.class, IsTestableMethod::hasVoidReturnType, issueReporter);
 	}
 
 }

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/IsTestTemplateMethod.java

@@ -25,7 +25,7 @@
 public class IsTestTemplateMethod extends IsTestableMethod {
 
 	public IsTestTemplateMethod(DiscoveryIssueReporter issueReporter) {
-		super(TestTemplate.class, true, issueReporter);
+		super(TestTemplate.class, IsTestableMethod::hasVoidReturnType, issueReporter);
 	}
 
 }

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/IsTestableMethod.java

@@ -11,10 +11,10 @@
 package org.junit.jupiter.engine.discovery.predicates;
 
 import static org.junit.platform.commons.support.AnnotationSupport.isAnnotated;
-import static org.junit.platform.commons.util.ReflectionUtils.returnsPrimitiveVoid;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Method;
+import java.util.function.BiFunction;
 import java.util.function.Predicate;
 
 import org.junit.platform.commons.support.ModifierSupport;
@@ -33,13 +33,14 @@ abstract class IsTestableMethod implements Predicate<Method> {
 	private final Class<? extends Annotation> annotationType;
 	private final Condition<Method> condition;
 
-	IsTestableMethod(Class<? extends Annotation> annotationType, boolean mustReturnPrimitiveVoid,
+	IsTestableMethod(Class<? extends Annotation> annotationType,
+			BiFunction<Class<? extends Annotation>, DiscoveryIssueReporter, Condition<Method>> returnTypeConditionFactory,
 			DiscoveryIssueReporter issueReporter) {
 		this.annotationType = annotationType;
-		this.condition = isNotStatic(issueReporter) //
-				.and(isNotPrivate(issueReporter)) //
-				.and(isNotAbstract(issueReporter)) //
-				.and(hasCompatibleReturnType(mustReturnPrimitiveVoid, issueReporter));
+		this.condition = isNotStatic(annotationType, issueReporter) //
+				.and(isNotPrivate(annotationType, issueReporter)) //
+				.and(isNotAbstract(annotationType, issueReporter)) //
+				.and(returnTypeConditionFactory.apply(annotationType, issueReporter));
 	}
 
 	@Override
@@ -50,37 +51,34 @@ public boolean test(Method candidate) {
 		return false;
 	}
 
-	private Condition<Method> isNotStatic(DiscoveryIssueReporter issueReporter) {
+	private static Condition<Method> isNotStatic(Class<? extends Annotation> annotationType,
+			DiscoveryIssueReporter issueReporter) {
 		return issueReporter.createReportingCondition(ModifierSupport::isNotStatic,
-			method -> createIssue(method, "must not be static"));
+			method -> createIssue(annotationType, method, "must not be static"));
 	}
 
-	private Condition<Method> isNotPrivate(DiscoveryIssueReporter issueReporter) {
+	private static Condition<Method> isNotPrivate(Class<? extends Annotation> annotationType,
+			DiscoveryIssueReporter issueReporter) {
 		return issueReporter.createReportingCondition(ModifierSupport::isNotPrivate,
-			method -> createIssue(method, "must not be private"));
+			method -> createIssue(annotationType, method, "must not be private"));
 	}
 
-	private Condition<Method> isNotAbstract(DiscoveryIssueReporter issueReporter) {
+	private static Condition<Method> isNotAbstract(Class<? extends Annotation> annotationType,
+			DiscoveryIssueReporter issueReporter) {
 		return issueReporter.createReportingCondition(ModifierSupport::isNotAbstract,
-			method -> createIssue(method, "must not be abstract"));
+			method -> createIssue(annotationType, method, "must not be abstract"));
 	}
 
-	private Condition<Method> hasCompatibleReturnType(boolean mustReturnPrimitiveVoid,
+	protected static Condition<Method> hasVoidReturnType(Class<? extends Annotation> annotationType,
 			DiscoveryIssueReporter issueReporter) {
-		if (mustReturnPrimitiveVoid) {
-			return issueReporter.createReportingCondition(ReflectionUtils::returnsPrimitiveVoid,
-				method -> createIssue(method, "must not return a value"));
-		}
-		else {
-			// TODO [#4246] Use `Predicate.not`
-			return issueReporter.createReportingCondition(method -> !returnsPrimitiveVoid(method),
-				method -> createIssue(method, "must return a value"));
-		}
+		return issueReporter.createReportingCondition(ReflectionUtils::returnsPrimitiveVoid,
+			method -> createIssue(annotationType, method, "must not return a value"));
 	}
 
-	private DiscoveryIssue createIssue(Method method, String condition) {
+	protected static DiscoveryIssue createIssue(Class<? extends Annotation> annotationType, Method method,
+			String condition) {
 		String message = String.format("@%s method '%s' %s. It will be not be executed.",
-			this.annotationType.getSimpleName(), method.toGenericString(), condition);
+			annotationType.getSimpleName(), method.toGenericString(), condition);
 		return DiscoveryIssue.builder(Severity.WARNING, message) //
 				.source(MethodSource.from(method)) //
 				.build();

jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/TestFactoryTestDescriptorTests.java

@@ -154,11 +154,10 @@ class Streams {
 		private ExtensionContext extensionContext;
 		private TestFactoryTestDescriptor descriptor;
 		private boolean isClosed;
-		private JupiterConfiguration jupiterConfiguration;
 
 		@BeforeEach
 		void before() throws Exception {
-			jupiterConfiguration = mock();
+			JupiterConfiguration jupiterConfiguration = mock();
 			when(jupiterConfiguration.getDefaultDisplayNameGenerator()).thenReturn(new DisplayNameGenerator.Standard());
 
 			extensionContext = mock();