From 0e00a49dcc246893fcaf1c7a0497fac6dcbc7454 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 17 Aug 2016 10:19:47 +0100 Subject: [PATCH] Prevent beans created with @MockBean from being post-processed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-processing of mocked beans causes a number of problems: - The mock may be proxied for asynchronous processing which can cause problems when configuring expectations on a mock (gh-6573) - The mock may be proxied so that its return values can be cached or so that its methods can be transactional. This causes problems with verification of the expected calls to a mock (gh-6573, gh-5837) - If the mock is created from a class that uses field injection, the container will attempt to inject values into its fields. This causes problems if the mock is being created to avoid the use of one of those dependencies (gh-6663) - Proxying a mocked bean can lead to a JDK proxy being created (if proxyTargetClass=false) as the mock implements a Mockito interface. This can then cause injection failures as the types don’t match (gh-6405, gh-6665) All of these problems can be avoided if a mocked bean is not post-processed. Avoiding post-processing prevents proxies from being created and autowiring from being performed. This commit avoids post-processing by registering mocked beans as singletons as well as via a bean definition. The latter is still used by the context for type matching purposes. Closes gh-6573, gh-6663, gh-6664 --- .../test/mock/mockito/DefinitionsParser.java | 3 +- .../boot/test/mock/mockito/MockBean.java | 12 --- .../test/mock/mockito/MockDefinition.java | 7 +- .../boot/test/mock/mockito/MockitoBeans.java | 41 +++++++++ .../mock/mockito/MockitoPostProcessor.java | 9 ++ .../ResetMocksTestExecutionListener.java | 12 +++ ...thAopProxyAndNotProxyTargetAwareTests.java | 91 ------------------- .../mockito/MockBeanWithAopProxyTests.java | 9 +- ...hAsyncInterfaceMethodIntegrationTests.java | 83 +++++++++++++++++ ...BeanWithInjectedFieldIntegrationTests.java | 64 +++++++++++++ .../mock/mockito/MockDefinitionTests.java | 10 +- .../ResetMocksTestExecutionListenerTests.java | 16 +++- 12 files changed, 235 insertions(+), 122 deletions(-) create mode 100644 spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoBeans.java delete mode 100644 spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyAndNotProxyTargetAwareTests.java create mode 100644 spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAsyncInterfaceMethodIntegrationTests.java create mode 100644 spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithInjectedFieldIntegrationTests.java diff --git a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/DefinitionsParser.java b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/DefinitionsParser.java index 227842b174..09dc9795ba 100644 --- a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/DefinitionsParser.java +++ b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/DefinitionsParser.java @@ -92,8 +92,7 @@ class DefinitionsParser { for (ResolvableType typeToMock : typesToMock) { MockDefinition definition = new MockDefinition(annotation.name(), typeToMock, annotation.extraInterfaces(), annotation.answer(), - annotation.serializable(), annotation.reset(), - annotation.proxyTargetAware()); + annotation.serializable(), annotation.reset()); addDefinition(element, definition, "mock"); } } diff --git a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockBean.java b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockBean.java index bb19f10d54..4f76b9f6df 100644 --- a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockBean.java +++ b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockBean.java @@ -26,7 +26,6 @@ import java.lang.annotation.Target; import org.junit.runner.RunWith; import org.mockito.Answers; import org.mockito.MockSettings; -import org.mockito.Mockito; import org.springframework.context.ApplicationContext; import org.springframework.core.annotation.AliasFor; @@ -140,15 +139,4 @@ public @interface MockBean { */ MockReset reset() default MockReset.AFTER; - /** - * Indicates that Mockito methods such as {@link Mockito#verify(Object) verify(mock)} - * should use the {@code target} of AOP advised beans, rather than the proxy itself. - * If set to {@code false} you may need to use the result of - * {@link org.springframework.test.util.AopTestUtils#getUltimateTargetObject(Object) - * AopTestUtils.getUltimateTargetObject(...)} when calling Mockito methods. - * @return {@code true} if the target of AOP advised beans is used or {@code false} if - * the proxy is used directly - */ - boolean proxyTargetAware() default true; - } diff --git a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockDefinition.java b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockDefinition.java index 5b8e49f45b..3da9b306fc 100644 --- a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockDefinition.java +++ b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockDefinition.java @@ -54,13 +54,12 @@ class MockDefinition extends Definition { } MockDefinition(ResolvableType typeToMock) { - this(null, typeToMock, null, null, false, null, true); + this(null, typeToMock, null, null, false, null); } MockDefinition(String name, ResolvableType typeToMock, Class[] extraInterfaces, - Answers answer, boolean serializable, MockReset reset, - boolean proxyTargetAware) { - super(name, reset, proxyTargetAware); + Answers answer, boolean serializable, MockReset reset) { + super(name, reset, false); Assert.notNull(typeToMock, "TypeToMock must not be null"); this.typeToMock = typeToMock; this.extraInterfaces = asClassSet(extraInterfaces); diff --git a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoBeans.java b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoBeans.java new file mode 100644 index 0000000000..db2a88b4fb --- /dev/null +++ b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoBeans.java @@ -0,0 +1,41 @@ +/* + * Copyright 2012-2016 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 + * + * http://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.boot.test.mock.mockito; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; + +/** + * Beans created using Mockito. + * + * @author Andy Wilkinson + */ +public class MockitoBeans implements Iterable { + + private final List beans = new ArrayList(); + + void add(Object bean) { + this.beans.add(bean); + } + + @Override + public Iterator iterator() { + return this.beans.iterator(); + } + +} diff --git a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoPostProcessor.java b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoPostProcessor.java index d0d91b9312..d666c64441 100644 --- a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoPostProcessor.java +++ b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/MockitoPostProcessor.java @@ -94,6 +94,8 @@ public class MockitoPostProcessor extends InstantiationAwareBeanPostProcessorAda private final BeanNameGenerator beanNameGenerator = new DefaultBeanNameGenerator(); + private final MockitoBeans mockitoBeans = new MockitoBeans(); + private Map beanNameRegistry = new HashMap(); private Map fieldRegistry = new HashMap(); @@ -132,6 +134,7 @@ public class MockitoPostProcessor extends InstantiationAwareBeanPostProcessorAda private void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory, BeanDefinitionRegistry registry) { + beanFactory.registerSingleton(MockitoBeans.class.getName(), this.mockitoBeans); DefinitionsParser parser = new DefinitionsParser(this.definitions); for (Class configurationClass : getConfigurationClasses(beanFactory)) { parser.parse(configurationClass); @@ -182,7 +185,13 @@ public class MockitoPostProcessor extends InstantiationAwareBeanPostProcessorAda String beanName = getBeanName(beanFactory, registry, definition, beanDefinition); beanDefinition.getConstructorArgumentValues().addIndexedArgumentValue(1, beanName); + if (registry.containsBeanDefinition(beanName)) { + registry.removeBeanDefinition(beanName); + } registry.registerBeanDefinition(beanName, beanDefinition); + Object mock = createMock(definition, beanName); + beanFactory.registerSingleton(beanName, mock); + this.mockitoBeans.add(mock); this.beanNameRegistry.put(definition, beanName); if (field != null) { this.fieldRegistry.put(field, new RegisteredField(definition, beanName)); diff --git a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListener.java b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListener.java index 155814906c..987564ae7c 100644 --- a/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListener.java +++ b/spring-boot-test/src/main/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListener.java @@ -22,6 +22,7 @@ import java.util.Set; import org.mockito.Mockito; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.context.ApplicationContext; @@ -70,6 +71,17 @@ public class ResetMocksTestExecutionListener extends AbstractTestExecutionListen } } } + try { + MockitoBeans mockedBeans = beanFactory.getBean(MockitoBeans.class); + for (Object mockedBean : mockedBeans) { + if (reset.equals(MockReset.get(mockedBean))) { + Mockito.reset(mockedBean); + } + } + } + catch (NoSuchBeanDefinitionException ex) { + // Continue + } if (applicationContext.getParent() != null) { resetMocks(applicationContext.getParent(), reset); } diff --git a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyAndNotProxyTargetAwareTests.java b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyAndNotProxyTargetAwareTests.java deleted file mode 100644 index 878c67df7b..0000000000 --- a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyAndNotProxyTargetAwareTests.java +++ /dev/null @@ -1,91 +0,0 @@ -/* - * Copyright 2012-2016 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 - * - * http://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.boot.test.mock.mockito; - -import java.util.Arrays; - -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.exceptions.misusing.UnfinishedVerificationException; - -import org.springframework.cache.CacheManager; -import org.springframework.cache.annotation.Cacheable; -import org.springframework.cache.annotation.EnableCaching; -import org.springframework.cache.concurrent.ConcurrentMapCacheManager; -import org.springframework.cache.interceptor.CacheResolver; -import org.springframework.cache.interceptor.SimpleCacheResolver; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.Import; -import org.springframework.stereotype.Service; -import org.springframework.test.context.junit4.SpringRunner; - -import static org.mockito.Mockito.reset; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; - -/** - * Test {@link MockBean} when mixed with Spring AOP. - * - * @author Phillip Webb - * @see 5837 - */ -@RunWith(SpringRunner.class) -public class MockBeanWithAopProxyAndNotProxyTargetAwareTests { - - @MockBean(proxyTargetAware = false) - private DateService dateService; - - @Test(expected = UnfinishedVerificationException.class) - public void verifyShouldUseProxyTarget() throws Exception { - this.dateService.getDate(); - verify(this.dateService, times(1)).getDate(); - reset(this.dateService); - } - - @Configuration - @EnableCaching(proxyTargetClass = true) - @Import(DateService.class) - static class Config { - - @Bean - public CacheResolver cacheResolver(CacheManager cacheManager) { - SimpleCacheResolver resolver = new SimpleCacheResolver(); - resolver.setCacheManager(cacheManager); - return resolver; - } - - @Bean - public ConcurrentMapCacheManager cacheManager() { - ConcurrentMapCacheManager cacheManager = new ConcurrentMapCacheManager(); - cacheManager.setCacheNames(Arrays.asList("test")); - return cacheManager; - } - - } - - @Service - static class DateService { - - @Cacheable(cacheNames = "test") - public Long getDate() { - return System.nanoTime(); - } - - } - -} diff --git a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyTests.java b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyTests.java index 27c199c374..548a7af55b 100644 --- a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyTests.java +++ b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAopProxyTests.java @@ -34,6 +34,7 @@ import org.springframework.stereotype.Service; import org.springframework.test.context.junit4.SpringRunner; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -51,11 +52,13 @@ public class MockBeanWithAopProxyTests { @Test public void verifyShouldUseProxyTarget() throws Exception { + given(this.dateService.getDate()).willReturn(1L); Long d1 = this.dateService.getDate(); - Thread.sleep(200); + assertThat(d1).isEqualTo(1L); + given(this.dateService.getDate()).willReturn(2L); Long d2 = this.dateService.getDate(); - assertThat(d1).isEqualTo(d2); - verify(this.dateService, times(1)).getDate(); + assertThat(d2).isEqualTo(2L); + verify(this.dateService, times(2)).getDate(); } @Configuration diff --git a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAsyncInterfaceMethodIntegrationTests.java b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAsyncInterfaceMethodIntegrationTests.java new file mode 100644 index 0000000000..f42c82fd41 --- /dev/null +++ b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithAsyncInterfaceMethodIntegrationTests.java @@ -0,0 +1,83 @@ +/* + * Copyright 2012-2016 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 + * + * http://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.boot.test.mock.mockito; + +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.scheduling.annotation.Async; +import org.springframework.scheduling.annotation.EnableAsync; +import org.springframework.test.context.junit4.SpringRunner; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.given; + +/** + * Tests for a mock bean where the mocked interface has an async method. + * + * @author Andy Wilkinson + */ +@RunWith(SpringRunner.class) +public class MockBeanWithAsyncInterfaceMethodIntegrationTests { + + @MockBean + private Transformer transformer; + + @Autowired + private MyService service; + + @Test + public void mockedMethodsAreNotAsync() { + given(this.transformer.transform("foo")).willReturn("bar"); + assertThat(this.service.transform("foo")).isEqualTo("bar"); + } + + private interface Transformer { + + @Async + String transform(String input); + + } + + private static class MyService { + + private final Transformer transformer; + + MyService(Transformer transformer) { + this.transformer = transformer; + } + + public String transform(String input) { + return this.transformer.transform(input); + } + + } + + @Configuration + @EnableAsync + static class MyConfiguration { + + @Bean + public MyService myService(Transformer transformer) { + return new MyService(transformer); + } + } + +} diff --git a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithInjectedFieldIntegrationTests.java b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithInjectedFieldIntegrationTests.java new file mode 100644 index 0000000000..a447bbb03f --- /dev/null +++ b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockBeanWithInjectedFieldIntegrationTests.java @@ -0,0 +1,64 @@ +/* + * Copyright 2012-2016 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 + * + * http://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.boot.test.mock.mockito; + +import java.util.List; + +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.context.junit4.SpringRunner; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.given; + +/** + * Tests for a mock bean where the class being mocked uses field injection. + * + * @author Andy Wilkinson + */ +@RunWith(SpringRunner.class) +public class MockBeanWithInjectedFieldIntegrationTests { + + @MockBean + private MyService myService; + + @Test + public void fieldInjectionIntoMyServiceMockIsNotAttempted() { + given(this.myService.getCount()).willReturn(5); + assertThat(this.myService.getCount()).isEqualTo(5); + } + + private static class MyService { + + @Autowired + private MyRepository repository; + + public int getCount() { + return this.repository.findAll().size(); + } + + } + + private interface MyRepository { + + List findAll(); + + } + +} diff --git a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockDefinitionTests.java b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockDefinitionTests.java index 72b91fab4a..391381d5e3 100644 --- a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockDefinitionTests.java +++ b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/MockDefinitionTests.java @@ -43,16 +43,16 @@ public class MockDefinitionTests { public ExpectedException thrown = ExpectedException.none(); @Test - public void ClassToMockMustNotBeNull() throws Exception { + public void classToMockMustNotBeNull() throws Exception { this.thrown.expect(IllegalArgumentException.class); this.thrown.expectMessage("TypeToMock must not be null"); - new MockDefinition(null, null, null, null, false, null, true); + new MockDefinition(null, null, null, null, false, null); } @Test public void createWithDefaults() throws Exception { MockDefinition definition = new MockDefinition(null, EXAMPLE_SERVICE_TYPE, null, - null, false, null, true); + null, false, null); assertThat(definition.getName()).isNull(); assertThat(definition.getTypeToMock()).isEqualTo(EXAMPLE_SERVICE_TYPE); assertThat(definition.getExtraInterfaces()).isEmpty(); @@ -65,7 +65,7 @@ public class MockDefinitionTests { public void createExplicit() throws Exception { MockDefinition definition = new MockDefinition("name", EXAMPLE_SERVICE_TYPE, new Class[] { ExampleExtraInterface.class }, - Answers.RETURNS_SMART_NULLS, true, MockReset.BEFORE, false); + Answers.RETURNS_SMART_NULLS, true, MockReset.BEFORE); assertThat(definition.getName()).isEqualTo("name"); assertThat(definition.getTypeToMock()).isEqualTo(EXAMPLE_SERVICE_TYPE); assertThat(definition.getExtraInterfaces()) @@ -80,7 +80,7 @@ public class MockDefinitionTests { public void createMock() throws Exception { MockDefinition definition = new MockDefinition("name", EXAMPLE_SERVICE_TYPE, new Class[] { ExampleExtraInterface.class }, - Answers.RETURNS_SMART_NULLS, true, MockReset.BEFORE, true); + Answers.RETURNS_SMART_NULLS, true, MockReset.BEFORE); ExampleService mock = definition.createMock(); MockCreationSettings settings = new MockUtil().getMockSettings(mock); assertThat(mock).isInstanceOf(ExampleService.class); diff --git a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListenerTests.java b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListenerTests.java index 24d7dce2a8..90a75cc5bc 100644 --- a/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListenerTests.java +++ b/spring-boot-test/src/test/java/org/springframework/boot/test/mock/mockito/ResetMocksTestExecutionListenerTests.java @@ -67,17 +67,23 @@ public class ResetMocksTestExecutionListenerTests { static class Config { @Bean - public ExampleService before() { - return mock(ExampleService.class, MockReset.before()); + public ExampleService before(MockitoBeans mockedBeans) { + ExampleService mock = mock(ExampleService.class, MockReset.before()); + mockedBeans.add(mock); + return mock; } @Bean - public ExampleService after() { - return mock(ExampleService.class, MockReset.before()); + public ExampleService after(MockitoBeans mockedBeans) { + ExampleService mock = mock(ExampleService.class, MockReset.before()); + mockedBeans.add(mock); + return mock; } @Bean - public ExampleService none() { + public ExampleService none(MockitoBeans mockedBeans) { + ExampleService mock = mock(ExampleService.class, MockReset.before()); + mockedBeans.add(mock); return mock(ExampleService.class); }