From b019f30a152560c36f589f1e584dd81706ac0ba8 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Tue, 10 Nov 2020 15:28:34 +0100 Subject: [PATCH] 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 --- .../junit/jupiter/SpringExtension.java | 86 +++++- ...edConfigurationErrorsIntegrationTests.java | 290 ++++++++++++++++++ 2 files changed, 372 insertions(+), 4 deletions(-) create mode 100644 spring-test/src/test/java/org/springframework/test/context/junit/jupiter/AutowiredConfigurationErrorsIntegrationTests.java 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 { + } + +} +