diff --git a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java index c9d185ffb0..97b5f1428f 100644 --- a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java +++ b/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 Spring TestContext Framework @@ -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> 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}. + *

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 annotationType : JUPITER_ANNOTATION_TYPES) { + if (mergedAnnotations.isPresent(annotationType)) { + return true; + } + } + return false; } } diff --git a/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/AutowiredConfigurationErrorsIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/AutowiredConfigurationErrorsIntegrationTests.java new file mode 100644 index 0000000000..e49221470d --- /dev/null +++ b/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 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 { + } + +} +