Improve error msg for non-static external @MethodSource factory methods

Prior to this commit, the error message was cryptic if an external
factory method was non-static and the test class was configured to run
with test instance per-class lifecycle semantics.

Specifically, the error was the following from the native implementation
of Method#invoke in the JDK.

java.lang.IllegalArgumentException: object is not an instance of declaring class

This commit builds on the work done in PR #3263 by expanding the "static
is required" check to any resolved factory method that cannot be invoked
on the current test instance (which may be null).

Consequently, the isPerMethodLifecycle() check is no longer necessary.

Closes #3276

Author: sbrannen

junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/MethodArgumentsProvider.java

@@ -24,7 +24,6 @@
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestFactory;
-import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.junit.jupiter.api.TestTemplate;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.platform.commons.JUnitException;
@@ -51,7 +50,7 @@ protected Stream<? extends Arguments> provideArguments(ExtensionContext context,
 		// @formatter:off
 		return stream(methodNames)
 				.map(factoryMethodName -> findFactoryMethod(testClass, testMethod, factoryMethodName))
-				.map(factoryMethod -> validateFactoryMethod(context, factoryMethod))
+				.map(factoryMethod -> validateFactoryMethod(context, factoryMethod, testInstance))
 				.map(factoryMethod -> context.getExecutableInvoker().invoke(factoryMethod, testInstance))
 				.flatMap(CollectionUtils::toStream)
 				.map(MethodArgumentsProvider::toArguments);
@@ -176,8 +175,8 @@ private static Class<?> loadRequiredClass(String className) {
 			cause -> new JUnitException(format("Could not load class [%s]", className), cause));
 	}
 
-	private static Method validateFactoryMethod(ExtensionContext context, Method factoryMethod) {
-		if (isPerMethodLifecycle(context)) {
+	private static Method validateFactoryMethod(ExtensionContext context, Method factoryMethod, Object testInstance) {
+		if (!factoryMethod.getDeclaringClass().isInstance(testInstance)) {
 			Preconditions.condition(ReflectionUtils.isStatic(factoryMethod),
 				() -> format("Method '%s' must be static: local factory methods must be static unless "
 						+ "the test class is annotated with @TestInstance(Lifecycle.PER_CLASS); external "
@@ -187,10 +186,6 @@ private static Method validateFactoryMethod(ExtensionContext context, Method fac
 		return factoryMethod;
 	}
 
-	private static boolean isPerMethodLifecycle(ExtensionContext context) {
-		return context.getTestInstanceLifecycle().orElse(Lifecycle.PER_CLASS) == Lifecycle.PER_METHOD;
-	}
-
 	private static Arguments toArguments(Object item) {
 
 		// Nothing to do except cast.

junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java

@@ -174,10 +174,38 @@ void providesArgumentsUsingListOfObjectArrays() {
 	}
 
 	@Test
-	void throwsExceptionWhenNonStaticFactoryMethodIsReferencedAndStaticIsRequired() {
+	void throwsExceptionWhenNonStaticLocalFactoryMethodIsReferencedWithLifecyclePerMethodSemantics() {
+		var lifecyclePerClass = false;
 		var exception = assertThrows(PreconditionViolationException.class,
-			() -> provideArguments(NonStaticTestCase.class, null, false, "nonStaticStringStreamProvider").toArray());
+			() -> provideArguments(NonStaticTestCase.class, lifecyclePerClass,
+				"nonStaticStringStreamProvider").toArray());
 
+		assertStaticIsRequired(exception);
+	}
+
+	@Test
+	void throwsExceptionWhenNonStaticExternalFactoryMethodIsReferencedWithLifecyclePerMethodSemantics() {
+		var factoryClass = NonStaticTestCase.class.getName();
+		var factoryMethod = factoryClass + "#nonStaticStringStreamProvider";
+		var lifecyclePerClass = false;
+		var exception = assertThrows(PreconditionViolationException.class,
+			() -> provideArguments(TestCase.class, lifecyclePerClass, factoryMethod).toArray());
+
+		assertStaticIsRequired(exception);
+	}
+
+	@Test
+	void throwsExceptionWhenNonStaticExternalFactoryMethodIsReferencedWithLifecyclePerClassSemantics() {
+		var factoryClass = NonStaticTestCase.class.getName();
+		var factoryMethod = factoryClass + "#nonStaticStringStreamProvider";
+		boolean lifecyclePerClass = true;
+		var exception = assertThrows(PreconditionViolationException.class,
+			() -> provideArguments(TestCase.class, lifecyclePerClass, factoryMethod).toArray());
+
+		assertStaticIsRequired(exception);
+	}
+
+	private static void assertStaticIsRequired(PreconditionViolationException exception) {
 		assertThat(exception).hasMessageContainingAll("Method '",
 			"' must be static: local factory methods must be static ",
 			"unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS); ",
@@ -186,7 +214,7 @@ void throwsExceptionWhenNonStaticFactoryMethodIsReferencedAndStaticIsRequired()
 
 	@Test
 	void providesArgumentsFromNonStaticFactoryMethodWhenStaticIsNotRequired() {
-		var arguments = provideArguments(NonStaticTestCase.class, null, true, "nonStaticStringStreamProvider");
+		var arguments = provideArguments(NonStaticTestCase.class, true, "nonStaticStringStreamProvider");
 
 		assertThat(arguments).containsExactly(array("foo"), array("bar"));
 	}
@@ -609,7 +637,7 @@ void throwsExceptionWhenExternalFactoryMethodHasInvalidReturnType() {
 			String factoryMethod = factoryClass + "#factoryWithInvalidReturnType";
 
 			var exception = assertThrows(PreconditionViolationException.class,
-				() -> provideArguments(TestCase.class, null, false, factoryMethod).toArray());
+				() -> provideArguments(TestCase.class, false, factoryMethod).toArray());
 
 			assertThat(exception.getMessage())//
 					.containsSubsequence("Could not find valid factory method [" + factoryMethod + "] for test class [",
@@ -676,30 +704,24 @@ private static Object[] array(Object... objects) {
 	}
 
 	private Stream<Object[]> provideArguments(String... factoryMethodNames) {
-		return provideArguments(TestCase.class, null, false, factoryMethodNames);
+		return provideArguments(TestCase.class, false, factoryMethodNames);
 	}
 
 	private Stream<Object[]> provideArguments(Method testMethod, String factoryMethodName) {
 		return provideArguments(TestCase.class, testMethod, false, factoryMethodName);
 	}
 
-	private Stream<Object[]> provideArguments(Class<?> testClass, Method testMethod, boolean allowNonStaticMethod,
+	private Stream<Object[]> provideArguments(Class<?> testClass, boolean allowNonStaticMethod,
 			String... factoryMethodNames) {
 
-		if (testMethod == null) {
-			try {
-				class DummyClass {
-					@SuppressWarnings("unused")
-					public void dummyMethod() {
-					};
-				}
-				// ensure we have a non-null method, even if it's not a real test method.
-				testMethod = DummyClass.class.getMethod("dummyMethod");
-			}
-			catch (Exception ex) {
-				throw new RuntimeException(ex);
-			}
-		}
+		// Ensure we have a non-null test method, even if it's not a real test method.
+		// If this throws an exception, make sure that the supplied test class defines a "void test()" method.
+		Method testMethod = ReflectionUtils.findMethod(testClass, "test").get();
+		return provideArguments(testClass, testMethod, allowNonStaticMethod, factoryMethodNames);
+	}
+
+	private Stream<Object[]> provideArguments(Class<?> testClass, Method testMethod, boolean allowNonStaticMethod,
+			String... factoryMethodNames) {
 
 		var methodSource = mock(MethodSource.class);
 
@@ -940,6 +962,9 @@ static Object[][] twoDimensionalObjectArrayProvider() {
 	// This test case mimics @TestInstance(Lifecycle.PER_CLASS)
 	static class NonStaticTestCase {
 
+		void test() {
+		}
+
 		Stream<String> nonStaticStringStreamProvider() {
 			return Stream.of("foo", "bar");
 		}