Validate that test & lifecycle methods are not @Autowired

Prior to this commit, a developer may have accidentally annotated a
JUnit Jupiter test method or lifecycle method with @Autowired, and that
would have potentially resulted in an exception that was hard to
understand. This is because the Spring container considers any
@Autowired method to be a "configuration method" when autowiring the
test class instance. Consequently, such an @Autowired method would be
invoked twice: once by Spring while attempting to autowire the test
instance and another time by JUnit Jupiter when invoking the test or
lifecycle method. The autowiring invocation of the method often leads
to an exception, either because Spring cannot satisfy a dependency (such
as JUnit Jupiter's TestInfo) or because the body of the method fails due
to test setup that has not yet been invoked.

This commit introduces validation for @Autowired test and lifecycle
methods in the SpringExtension that will throw an IllegalStateException
if any @Autowired method in a test class is also annotated with any of
the following JUnit Jupiter annotations.

- @Test
- @TestFactory
- @TestTemplate
- @RepeatedTest
- @ParameterizedTest
- @BeforeAll
- @AfterAll
- @BeforeEach
- @AfterEach

Closes gh-25966

spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java

@@ -16,11 +16,19 @@
 
 package org.springframework.test.context.junit.jupiter;
 
+import java.lang.annotation.Annotation;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Executable;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.lang.reflect.Parameter;
+import java.util.Arrays;
+import java.util.List;
 
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.extension.AfterAllCallback;
 import org.junit.jupiter.api.extension.AfterEachCallback;
 import org.junit.jupiter.api.extension.AfterTestExecutionCallback;
@@ -33,15 +41,22 @@ import org.junit.jupiter.api.extension.ExtensionContext.Store;
 import org.junit.jupiter.api.extension.ParameterContext;
 import org.junit.jupiter.api.extension.ParameterResolver;
 import org.junit.jupiter.api.extension.TestInstancePostProcessor;
+import org.junit.platform.commons.annotation.Testable;
 
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.ParameterResolutionDelegate;
 import org.springframework.context.ApplicationContext;
+import org.springframework.core.annotation.MergedAnnotations;
+import org.springframework.core.annotation.MergedAnnotations.SearchStrategy;
+import org.springframework.core.annotation.RepeatableContainers;
 import org.springframework.lang.Nullable;
 import org.springframework.test.context.TestConstructor;
 import org.springframework.test.context.TestContextManager;
 import org.springframework.test.context.support.PropertyProvider;
 import org.springframework.test.context.support.TestConstructorUtils;
 import org.springframework.util.Assert;
+import org.springframework.util.ReflectionUtils;
+import org.springframework.util.ReflectionUtils.MethodFilter;
 
 /**
  * {@code SpringExtension} integrates the <em>Spring TestContext Framework</em>
@@ -64,10 +79,29 @@ public class SpringExtension implements BeforeAllCallback, AfterAllCallback, Tes
 		ParameterResolver {
 
 	/**
-	 * {@link Namespace} in which {@code TestContextManagers} are stored,
-	 * keyed by test class.
+	 * {@link Namespace} in which {@code TestContextManagers} are stored, keyed
+	 * by test class.
 	 */
-	private static final Namespace NAMESPACE = Namespace.create(SpringExtension.class);
+	private static final Namespace TEST_CONTEXT_MANAGER_NAMESPACE = Namespace.create(SpringExtension.class);
+
+	/**
+	 * {@link Namespace} in which {@code @Autowired} validation error messages
+	 * are stored, keyed by test class.
+	 */
+	private static final Namespace AUTOWIRED_VALIDATION_NAMESPACE = Namespace.create(SpringExtension.class.getName() +
+			"#autowired.validation");
+
+	private static final String NO_AUTOWIRED_VIOLATIONS_DETECTED = "NO AUTOWIRED VIOLATIONS DETECTED";
+
+	// Note that @Test, @TestFactory, @TestTemplate, @RepeatedTest, and @ParameterizedTest
+	// are all meta-annotated with @Testable.
+	private static final List<Class<? extends Annotation>> JUPITER_ANNOTATION_TYPES =
+			Arrays.asList(BeforeAll.class, AfterAll.class, BeforeEach.class, AfterEach.class, Testable.class);
+
+	private static final MethodFilter autowiredTestOrLifecycleMethodFilter = method ->
+			(ReflectionUtils.USER_DECLARED_METHODS.matches(method) &&
+				!Modifier.isPrivate(method.getModifiers()) &&
+				isAutowiredTestOrLifecycleMethod(method));
 
 
 	/**
@@ -93,12 +127,42 @@ public class SpringExtension implements BeforeAllCallback, AfterAllCallback, Tes
 
 	/**
 	 * Delegates to {@link TestContextManager#prepareTestInstance}.
+	 * <p>As of Spring Framework 5.3.2, this method also validates that test
+	 * methods and test lifecycle methods are not annotated with
+	 * {@link Autowired @Autowired}.
 	 */
 	@Override
 	public void postProcessTestInstance(Object testInstance, ExtensionContext context) throws Exception {
+		validateAutowiredConfig(context);
 		getTestContextManager(context).prepareTestInstance(testInstance);
 	}
 
+	/**
+	 * Validate that test methods and test lifecycle methods in the supplied
+	 * test class are not annotated with {@link Autowired @Autowired}.
+	 * @since 5.3.2
+	 */
+	private void validateAutowiredConfig(ExtensionContext context) {
+		// We save the result in the ExtensionContext.Store so that we don't
+		// re-validate all methods for the same test class multiple times.
+		Store store = context.getStore(AUTOWIRED_VALIDATION_NAMESPACE);
+		String errorMessage = store.getOrComputeIfAbsent(context.getRequiredTestClass(),
+			testClass -> {
+				Method[] methodsWithErrors =
+						ReflectionUtils.getUniqueDeclaredMethods(testClass, autowiredTestOrLifecycleMethodFilter);
+				return (methodsWithErrors.length == 0 ? NO_AUTOWIRED_VIOLATIONS_DETECTED :
+						String.format(
+							"Test methods and test lifecycle methods must not be annotated with @Autowired. " +
+							"You should instead annotate individual method parameters with @Autowired, " +
+							"@Qualifier, or @Value. Offending methods in test class %s: %s",
+							testClass.getName(), Arrays.toString(methodsWithErrors)));
+			}, String.class);
+
+		if (errorMessage != NO_AUTOWIRED_VIOLATIONS_DETECTED) {
+			throw new IllegalStateException(errorMessage);
+		}
+	}
+
 	/**
 	 * Delegates to {@link TestContextManager#beforeTestMethod}.
 	 */
@@ -219,7 +283,21 @@ public class SpringExtension implements BeforeAllCallback, AfterAllCallback, Tes
 	}
 
 	private static Store getStore(ExtensionContext context) {
-		return context.getRoot().getStore(NAMESPACE);
+		return context.getRoot().getStore(TEST_CONTEXT_MANAGER_NAMESPACE);
+	}
+
+	private static boolean isAutowiredTestOrLifecycleMethod(Method method) {
+		MergedAnnotations mergedAnnotations =
+				MergedAnnotations.from(method, SearchStrategy.DIRECT, RepeatableContainers.none());
+		if (!mergedAnnotations.isPresent(Autowired.class)) {
+			return false;
+		}
+		for (Class<? extends Annotation> annotationType : JUPITER_ANNOTATION_TYPES) {
+			if (mergedAnnotations.isPresent(annotationType)) {
+				return true;
+			}
+		}
+		return false;
 	}
 
 }

spring-test/src/test/java/org/springframework/test/context/junit/jupiter/AutowiredConfigurationErrorsIntegrationTests.java

@@ -0,0 +1,290 @@
+/*
+ * Copyright 2002-2020 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.test.context.junit.jupiter;
+
+import java.util.stream.Stream;
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DynamicTest;
+import org.junit.jupiter.api.RepeatedTest;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestFactory;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.junit.platform.testkit.engine.EngineTestKit;
+import org.junit.platform.testkit.engine.Events;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Configuration;
+
+import static org.junit.jupiter.api.DynamicTest.dynamicTest;
+import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
+import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
+import static org.junit.platform.testkit.engine.EventConditions.container;
+import static org.junit.platform.testkit.engine.EventConditions.event;
+import static org.junit.platform.testkit.engine.EventConditions.finishedWithFailure;
+import static org.junit.platform.testkit.engine.EventConditions.test;
+import static org.junit.platform.testkit.engine.TestExecutionResultConditions.instanceOf;
+import static org.junit.platform.testkit.engine.TestExecutionResultConditions.message;
+
+/**
+ * Integration tests for {@link Autowired @Autowired} configuration errors in
+ * JUnit Jupiter test classes.
+ *
+ * @author Sam Brannen
+ * @since 5.3.2
+ */
+class AutowiredConfigurationErrorsIntegrationTests {
+
+	@ParameterizedTest
+	@ValueSource(classes = {
+		StaticAutowiredBeforeAllMethod.class,
+		StaticAutowiredAfterAllMethod.class,
+		AutowiredBeforeEachMethod.class,
+		AutowiredAfterEachMethod.class,
+		AutowiredTestMethod.class,
+		AutowiredRepeatedTestMethod.class,
+		AutowiredParameterizedTestMethod.class
+	})
+	void autowiredTestMethodsTestTemplateMethodsAndLifecyleMethods(Class<?> testClass) {
+		testEventsFor(testClass)
+			.assertStatistics(stats -> stats.started(1).succeeded(0).failed(1))
+			.assertThatEvents().haveExactly(1,
+				event(test("test"),
+				finishedWithFailure(
+					instanceOf(IllegalStateException.class),
+					message(msg -> msg.matches(".+must not be annotated with @Autowired.+")))));
+	}
+
+	/**
+	 * A non-autowired test method should fail the same as an autowired test
+	 * method in the same class, since Spring still should not autowire the
+	 * autowired test method as a "configuration method" when JUnit attempts to
+	 * execute the non-autowired test method.
+	 */
+	@Test
+	void autowiredAndNonAutowiredTestMethods() {
+		testEventsFor(AutowiredAndNonAutowiredTestMethods.class)
+		.assertStatistics(stats -> stats.started(2).succeeded(0).failed(2))
+		.assertThatEvents()
+			.haveExactly(1,
+				event(test("autowired"),
+				finishedWithFailure(
+					instanceOf(IllegalStateException.class),
+					message(msg -> msg.matches(".+must not be annotated with @Autowired.+")))))
+			.haveExactly(1,
+				event(test("nonAutowired"),
+				finishedWithFailure(
+					instanceOf(IllegalStateException.class),
+					message(msg -> msg.matches(".+must not be annotated with @Autowired.+")))));
+	}
+
+
+	@ParameterizedTest
+	@ValueSource(classes = {
+		NonStaticAutowiredBeforeAllMethod.class,
+		NonStaticAutowiredAfterAllMethod.class
+	})
+	void autowiredNonStaticClassLevelLifecyleMethods(Class<?> testClass) {
+		containerEventsFor(testClass)
+			.assertStatistics(stats -> stats.started(2).succeeded(1).failed(1))
+			.assertThatEvents().haveExactly(1,
+				event(container(),
+				finishedWithFailure(
+					instanceOf(IllegalStateException.class),
+					message(msg -> msg.matches(".+must not be annotated with @Autowired.+")))));
+	}
+
+	@Test
+	void autowiredTestFactoryMethod() {
+		containerEventsFor(AutowiredTestFactoryMethod.class)
+			.assertStatistics(stats -> stats.started(3).succeeded(2).failed(1))
+			.assertThatEvents().haveExactly(1,
+				event(container(),
+				finishedWithFailure(
+					instanceOf(IllegalStateException.class),
+					message(msg -> msg.matches(".+must not be annotated with @Autowired.+")))));
+	}
+
+	private Events testEventsFor(Class<?> testClass) {
+		return EngineTestKit.engine("junit-jupiter")
+			.selectors(selectClass(testClass))
+			.execute()
+			.testEvents();
+	}
+
+	private Events containerEventsFor(Class<?> testClass) {
+		return EngineTestKit.engine("junit-jupiter")
+			.selectors(selectClass(testClass))
+			.execute()
+			.containerEvents();
+	}
+
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class StaticAutowiredBeforeAllMethod {
+
+		@Autowired
+		@BeforeAll
+		static void beforeAll(TestInfo testInfo) {
+		}
+
+		@Test
+		void test() {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@TestInstance(PER_CLASS)
+	@FailingTestCase
+	static class NonStaticAutowiredBeforeAllMethod {
+
+		@Autowired
+		@BeforeAll
+		void beforeAll(TestInfo testInfo) {
+		}
+
+		@Test
+		void test() {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class StaticAutowiredAfterAllMethod {
+
+		@Test
+		void test() {
+		}
+
+		@AfterAll
+		@Autowired
+		static void afterAll(TestInfo testInfo) {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@TestInstance(PER_CLASS)
+	@FailingTestCase
+	static class NonStaticAutowiredAfterAllMethod {
+
+		@Test
+		void test() {
+		}
+
+		@AfterAll
+		@Autowired
+		void afterAll(TestInfo testInfo) {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class AutowiredBeforeEachMethod {
+
+		@Autowired
+		@BeforeEach
+		void beforeEach(TestInfo testInfo) {
+		}
+
+		@Test
+		void test() {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class AutowiredAfterEachMethod {
+
+		@Test
+		void test() {
+		}
+
+		@Autowired
+		@AfterEach
+		void afterEach(TestInfo testInfo) {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class AutowiredTestMethod {
+
+		@Autowired
+		@Test
+		void test(TestInfo testInfo) {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class AutowiredAndNonAutowiredTestMethods {
+
+		@Autowired
+		@Test
+		void autowired(TestInfo testInfo) {
+		}
+
+		@Test
+		void nonAutowired(TestInfo testInfo) {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class AutowiredRepeatedTestMethod {
+
+		@Autowired
+		@RepeatedTest(1)
+		void repeatedTest(TestInfo testInfo) {
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class AutowiredTestFactoryMethod {
+
+		@Autowired
+		@TestFactory
+		Stream<DynamicTest> testFactory(TestInfo testInfo) {
+			return Stream.of(dynamicTest("dynamicTest", () -> {}));
+		}
+	}
+
+	@SpringJUnitConfig(Config.class)
+	@FailingTestCase
+	static class AutowiredParameterizedTestMethod {
+
+		@Autowired
+		@ParameterizedTest
+		@ValueSource(strings = "ignored")
+		void parameterizedTest(TestInfo testInfo) {
+		}
+	}
+
+	@Configuration
+	static class Config {
+	}
+
+}
+