From cd60a0013beb090ceda9227a1a66e59d238004a6 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 9 Dec 2024 15:22:15 +0100 Subject: [PATCH] Reject identical Bean Overrides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, the Bean Override feature in the Spring TestContext Framework (for annotations such as @⁠MockitoBean and @⁠TestBean) silently allowed one bean override to override another "identical" bean override; however, Spring Boot's @⁠MockBean and @⁠SpyBean support preemptively rejects identical overrides and throws an IllegalStateException to signal the configuration error to the user. To align with the behavior of @⁠MockBean and @⁠SpyBean in Spring Boot, and to help developers avoid scenarios that are potentially confusing or difficult to debug, this commit rejects identical bean overrides in the Spring TestContext Framework. Note, however, that it is still possible for a bean override to override a logically equivalent bean override. For example, a @⁠TestBean can override a @⁠MockitoBean, and vice versa. Closes gh-34054 --- .../BeanOverrideContextCustomizerFactory.java | 6 +- ...OverrideContextCustomizerFactoryTests.java | 31 +++++-- ...stBeanForByNameLookupIntegrationTests.java | 48 ++--------- ...toBeanForByNameLookupIntegrationTests.java | 21 +---- ...nDuplicateTypeAndNameIntegrationTests.java | 83 ------------------- ...pyBeanForByNameLookupIntegrationTests.java | 39 --------- 6 files changed, 39 insertions(+), 189 deletions(-) delete mode 100644 spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeAndNameIntegrationTests.java diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java index 29cc22fc225..544b43e996b 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java @@ -24,6 +24,7 @@ import org.springframework.lang.Nullable; import org.springframework.test.context.ContextConfigurationAttributes; import org.springframework.test.context.ContextCustomizerFactory; import org.springframework.test.context.TestContextAnnotationUtils; +import org.springframework.util.Assert; /** * {@link ContextCustomizerFactory} implementation that provides support for @@ -51,10 +52,13 @@ class BeanOverrideContextCustomizerFactory implements ContextCustomizerFactory { } private void findBeanOverrideHandler(Class testClass, Set handlers) { - handlers.addAll(BeanOverrideHandler.forTestClass(testClass)); if (TestContextAnnotationUtils.searchEnclosingClass(testClass)) { findBeanOverrideHandler(testClass.getEnclosingClass(), handlers); } + BeanOverrideHandler.forTestClass(testClass).forEach(handler -> + Assert.state(handlers.add(handler), () -> + "Duplicate BeanOverrideHandler discovered in test class %s: %s" + .formatted(testClass.getName(), handler))); } } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactoryTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactoryTests.java index 0dbef0ee7df..2ed2498993e 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactoryTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactoryTests.java @@ -19,18 +19,20 @@ package org.springframework.test.context.bean.override; import java.util.Collections; import java.util.function.Consumer; -import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.springframework.lang.Nullable; import org.springframework.test.context.bean.override.DummyBean.DummyBeanOverrideProcessor.DummyBeanOverrideHandler; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** * Tests for {@link BeanOverrideContextCustomizerFactory}. * * @author Stephane Nicoll + * @author Sam Brannen + * @since 6.2 */ class BeanOverrideContextCustomizerFactoryTests { @@ -65,6 +67,15 @@ class BeanOverrideContextCustomizerFactoryTests { .hasSize(2); } + @Test // gh-34054 + void failsWithDuplicateBeanOverrides() { + Class testClass = DuplicateOverridesTestCase.class; + assertThatIllegalStateException() + .isThrownBy(() -> createContextCustomizer(testClass)) + .withMessageStartingWith("Duplicate BeanOverrideHandler discovered in test class " + testClass.getName()) + .withMessageContaining("DummyBeanOverrideHandler"); + } + private Consumer dummyHandler(@Nullable String beanName, Class beanType) { return dummyHandler(beanName, beanType, BeanOverrideStrategy.REPLACE); @@ -80,15 +91,15 @@ class BeanOverrideContextCustomizerFactoryTests { } @Nullable - BeanOverrideContextCustomizer createContextCustomizer(Class testClass) { + private BeanOverrideContextCustomizer createContextCustomizer(Class testClass) { return this.factory.createContextCustomizer(testClass, Collections.emptyList()); } + static class Test1 { @DummyBean private String descriptor; - } static class Test2 { @@ -96,17 +107,25 @@ class BeanOverrideContextCustomizerFactoryTests { @DummyBean private String name; - @Nested + // @Nested class Orange { } - @Nested + // @Nested class Green { @DummyBean(beanName = "counterBean") private Integer counter; - } } + static class DuplicateOverridesTestCase { + + @DummyBean(beanName = "text") + String text1; + + @DummyBean(beanName = "text") + String text2; + } + } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanForByNameLookupIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanForByNameLookupIntegrationTests.java index e350535623c..63dfd4e9604 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanForByNameLookupIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanForByNameLookupIntegrationTests.java @@ -40,9 +40,6 @@ public class TestBeanForByNameLookupIntegrationTests { @TestBean(name = "field") String field; - @TestBean(name = "field") - String renamed; - @TestBean(name = "methodRenamed1", methodName = "field") String methodRenamed1; @@ -60,12 +57,6 @@ public class TestBeanForByNameLookupIntegrationTests { assertThat(field).as("injection point").isEqualTo("fieldOverride"); } - @Test - void renamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("field")).as("applicationContext").isEqualTo("fieldOverride"); - assertThat(renamed).as("injection point").isEqualTo("fieldOverride"); - } - @Test void fieldWithMethodNameHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("methodRenamed1")).as("applicationContext").isEqualTo("fieldOverride"); @@ -80,9 +71,6 @@ public class TestBeanForByNameLookupIntegrationTests { @TestBean(name = "nestedField") String nestedField; - @TestBean(name = "nestedField") - String renamed2; - @TestBean(name = "methodRenamed2", methodName = "nestedField") String methodRenamed2; @@ -93,12 +81,6 @@ public class TestBeanForByNameLookupIntegrationTests { assertThat(field).as("injection point").isEqualTo("fieldOverride"); } - @Test - void renamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("field")).as("applicationContext").isEqualTo("fieldOverride"); - assertThat(renamed).as("injection point").isEqualTo("fieldOverride"); - } - @Test void fieldWithMethodNameHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("methodRenamed1")).as("applicationContext").isEqualTo("fieldOverride"); @@ -111,12 +93,6 @@ public class TestBeanForByNameLookupIntegrationTests { assertThat(nestedField).isEqualTo("nestedFieldOverride"); } - @Test - void nestedRenamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("nestedField")).as("applicationContext").isEqualTo("nestedFieldOverride"); - assertThat(renamed2).isEqualTo("nestedFieldOverride"); - } - @Test void nestedFieldWithMethodNameHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("methodRenamed2")).as("applicationContext").isEqualTo("nestedFieldOverride"); @@ -133,12 +109,6 @@ public class TestBeanForByNameLookupIntegrationTests { assertThat(field).as("injection point").isEqualTo("fieldOverride"); } - @Test - void renamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("field")).as("applicationContext").isEqualTo("fieldOverride"); - assertThat(renamed).as("injection point").isEqualTo("fieldOverride"); - } - @Test void fieldWithMethodNameHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("methodRenamed1")).as("applicationContext").isEqualTo("fieldOverride"); @@ -151,12 +121,6 @@ public class TestBeanForByNameLookupIntegrationTests { assertThat(nestedField).isEqualTo("nestedFieldOverride"); } - @Test - void nestedRenamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("nestedField")).as("applicationContext").isEqualTo("nestedFieldOverride"); - assertThat(renamed2).isEqualTo("nestedFieldOverride"); - } - @Test void nestedFieldWithMethodNameHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("methodRenamed2")).as("applicationContext").isEqualTo("nestedFieldOverride"); @@ -170,25 +134,25 @@ public class TestBeanForByNameLookupIntegrationTests { public class TestBeanFactoryMethodInEnclosingClassTests { @TestBean(methodName = "nestedField", name = "nestedField") - String nestedField2; + String nestedField; @Test void nestedFieldHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("nestedField")).as("applicationContext").isEqualTo("nestedFieldOverride"); - assertThat(nestedField2).isEqualTo("nestedFieldOverride"); + assertThat(nestedField).isEqualTo("nestedFieldOverride"); } @Nested @DisplayName("With factory method in the enclosing class of the enclosing class") public class TestBeanFactoryMethodInEnclosingClassLevel2Tests { - @TestBean(methodName = "nestedField", name = "nestedField") - String nestedField2; + @TestBean(methodName = "nestedField", name = "nestedNestedField") + String nestedNestedField; @Test void nestedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("nestedField")).as("applicationContext").isEqualTo("nestedFieldOverride"); - assertThat(nestedField2).isEqualTo("nestedFieldOverride"); + assertThat(ctx.getBean("nestedNestedField")).as("applicationContext").isEqualTo("nestedFieldOverride"); + assertThat(nestedNestedField).isEqualTo("nestedFieldOverride"); } } } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForByNameLookupIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForByNameLookupIntegrationTests.java index d8cf06026e8..46643178eb3 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForByNameLookupIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForByNameLookupIntegrationTests.java @@ -45,9 +45,6 @@ public class MockitoBeanForByNameLookupIntegrationTests { @MockitoBean("field") ExampleService field; - @MockitoBean("field") - ExampleService renamed; - @MockitoBean("nonExistingBean") ExampleService nonExisting; @@ -57,11 +54,9 @@ public class MockitoBeanForByNameLookupIntegrationTests { assertThat(ctx.getBean("field")) .isInstanceOf(ExampleService.class) .satisfies(MockitoAssertions::assertIsMock) - .isSameAs(field) - .isSameAs(renamed); + .isSameAs(field); assertThat(field.greeting()).as("mocked greeting").isNull(); - assertThat(renamed.greeting()).as("mocked greeting").isNull(); } @Test @@ -83,10 +78,6 @@ public class MockitoBeanForByNameLookupIntegrationTests { @Qualifier("field") ExampleService localField; - @Autowired - @Qualifier("field") - ExampleService localRenamed; - @Autowired @Qualifier("nonExistingBean") ExampleService localNonExisting; @@ -94,9 +85,6 @@ public class MockitoBeanForByNameLookupIntegrationTests { @MockitoBean("nestedField") ExampleService nestedField; - @MockitoBean("nestedField") - ExampleService nestedRenamed; - @MockitoBean("nestedNonExistingBean") ExampleService nestedNonExisting; @@ -106,11 +94,9 @@ public class MockitoBeanForByNameLookupIntegrationTests { assertThat(ctx.getBean("field")) .isInstanceOf(ExampleService.class) .satisfies(MockitoAssertions::assertIsMock) - .isSameAs(localField) - .isSameAs(localRenamed); + .isSameAs(localField); assertThat(localField.greeting()).as("mocked greeting").isNull(); - assertThat(localRenamed.greeting()).as("mocked greeting").isNull(); } @Test @@ -128,8 +114,7 @@ public class MockitoBeanForByNameLookupIntegrationTests { assertThat(ctx.getBean("nestedField")) .isInstanceOf(ExampleService.class) .satisfies(MockitoAssertions::assertIsMock) - .isSameAs(nestedField) - .isSameAs(nestedRenamed); + .isSameAs(nestedField); } @Test diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeAndNameIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeAndNameIntegrationTests.java deleted file mode 100644 index 66bc7030dcb..00000000000 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeAndNameIntegrationTests.java +++ /dev/null @@ -1,83 +0,0 @@ -/* - * Copyright 2002-2024 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.bean.override.mockito; - -import java.util.List; - -import org.junit.jupiter.api.Test; - -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.test.context.bean.override.example.ExampleService; -import org.springframework.test.context.bean.override.example.RealExampleService; -import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.springframework.test.mockito.MockitoAssertions.assertIsNotSpy; -import static org.springframework.test.mockito.MockitoAssertions.assertIsSpy; - -/** - * Integration tests for duplicate {@link MockitoSpyBean @MockitoSpyBean} - * declarations for the same target bean, selected by-name. - * - * @author Sam Brannen - * @since 6.2.1 - * @see MockitoBeanDuplicateTypeIntegrationTests - * @see MockitoSpyBeanDuplicateTypeIntegrationTests - */ -@SpringJUnitConfig -public class MockitoSpyBeanDuplicateTypeAndNameIntegrationTests { - - @MockitoSpyBean("exampleService1") - ExampleService service1; - - @MockitoSpyBean("exampleService1") - ExampleService service2; - - @Autowired - ExampleService exampleService2; - - @Autowired - List services; - - - @Test - void duplicateMocksShouldHaveBeenCreated() { - assertThat(service1).isSameAs(service2); - assertThat(services).containsExactly(service1, exampleService2); - - assertIsSpy(service1, "service1"); - assertIsNotSpy(exampleService2, "exampleService2"); - } - - - @Configuration(proxyBeanMethods = false) - static class Config { - - @Bean - ExampleService exampleService1() { - return new RealExampleService("@Bean 1"); - } - - @Bean - ExampleService exampleService2() { - return new RealExampleService("@Bean 2"); - } - } - -} diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanForByNameLookupIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanForByNameLookupIntegrationTests.java index be824c52e21..117570a0ce8 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanForByNameLookupIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanForByNameLookupIntegrationTests.java @@ -46,9 +46,6 @@ public class MockitoSpyBeanForByNameLookupIntegrationTests { @MockitoSpyBean("field1") ExampleService field; - @MockitoSpyBean("field1") - ExampleService renamed; - @Test void fieldHasOverride(ApplicationContext ctx) { @@ -60,15 +57,6 @@ public class MockitoSpyBeanForByNameLookupIntegrationTests { assertThat(field.greeting()).isEqualTo("bean1"); } - @Test - void renamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("field1")) - .isInstanceOf(ExampleService.class) - .satisfies(MockitoAssertions::assertIsSpy) - .isSameAs(renamed); - - assertThat(renamed.greeting()).isEqualTo("bean1"); - } @Nested @DisplayName("With @MockitoSpyBean in enclosing class and in @Nested class") @@ -78,16 +66,9 @@ public class MockitoSpyBeanForByNameLookupIntegrationTests { @Qualifier("field1") ExampleService localField; - @Autowired - @Qualifier("field1") - ExampleService localRenamed; - @MockitoSpyBean("field2") ExampleService nestedField; - @MockitoSpyBean("field2") - ExampleService nestedRenamed; - @Test void fieldHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("field1")) @@ -98,16 +79,6 @@ public class MockitoSpyBeanForByNameLookupIntegrationTests { assertThat(localField.greeting()).isEqualTo("bean1"); } - @Test - void renamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("field1")) - .isInstanceOf(ExampleService.class) - .satisfies(MockitoAssertions::assertIsSpy) - .isSameAs(localRenamed); - - assertThat(localRenamed.greeting()).isEqualTo("bean1"); - } - @Test void nestedFieldHasOverride(ApplicationContext ctx) { assertThat(ctx.getBean("field2")) @@ -117,16 +88,6 @@ public class MockitoSpyBeanForByNameLookupIntegrationTests { assertThat(nestedField.greeting()).isEqualTo("bean2"); } - - @Test - void nestedRenamedFieldHasOverride(ApplicationContext ctx) { - assertThat(ctx.getBean("field2")) - .isInstanceOf(ExampleService.class) - .satisfies(MockitoAssertions::assertIsSpy) - .isSameAs(nestedRenamed); - - assertThat(nestedRenamed.greeting()).isEqualTo("bean2"); - } } @Configuration(proxyBeanMethods = false)