Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@MockBean leads to Mockito Validation Exceptions presumably masking problems with @Async annotated services #6573

Closed
WalternativE opened this issue Aug 5, 2016 · 9 comments
Labels
type: bug A general bug
Milestone

Comments

@WalternativE
Copy link

WalternativE commented Aug 5, 2016

Hello everyone,

I have been experimenting with the Mockito integration in the new 1.4.0.RELEASE version of spring boot and encountered some difficulties while writing some tests.

I wrote a controller, which injects a service bean and calls an async method (async configuration is written to return a ThreadPoolTaskExecutor). When using the @MockBean annotation on the service in my test class all usages of parameter matchers fail (either while configuring in before method using given/willReturn or while verifying method calls).

I wrote a basic project illustrating this behavior - it can be found at https://github.com/WalternativE/mocking-bug . The tag "problem" contains the failing test while the tag "workaround" contains the profile configuration deactivating the async configuration for the test (the test passes without exception).

While researching the errors I stumbled upon issue #6405 - could be linked to that to some extend. I tried playing around with the proxyTargetClass parameter but I had no luck there.

While debugging the test cases I got the expected behavior while inspecting the mocked service with the IntelliJ Idea debugging tools - might have resolved some proxies manually which maybe cannot be resolved reflectively by Mockito (but that's just guesswork - haven't had the opportunity to reproduce that yet).

Thanks,
Gregor

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 5, 2016
@WalternativE WalternativE changed the title Hello everyone, @MockBean leads to Mockito Validation Exceptions presumably masking problems with @Async annotated services Aug 5, 2016
@wilkinsona wilkinsona self-assigned this Aug 9, 2016
@wilkinsona
Copy link
Member

The problem is that SimpleService.doTask is annotated with @Async. This means that the mock of that interface is automatically proxied for asynchronous execution. As a result, there's a race condition in MockTest.init() as the call to this.simpleService.doTask() is passed to the underlying mock on a separate thread.

What's the reason for having @Async on both SimpleService.doTask() and SimpleServiceImpl.doTask()? If it's only on the latter the mock is no longer asynchronous and the test passes.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 9, 2016
@WalternativE
Copy link
Author

Must have happened when I extracted the interface via IntelliJ. I removed the annotation from the interface which resulted in the passing test you described while keeping the async behavior in production. I tested the same for spying instead of mocking and the conclusion was the same. I could kick myself for that one. Sorry for wasting your time and thank you for the quick response!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 9, 2016
@wilkinsona
Copy link
Member

wilkinsona commented Aug 9, 2016

No apology needed. Ideally, it wouldn't matter if @Async was on the interface but I'm not sure how we can manage that. I'll discuss it with the rest of the team to see if they have any bright ideas.

@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review and removed status: feedback-provided Feedback has been provided labels Aug 9, 2016
@WalternativE
Copy link
Author

Thank you very much for that! If I can anyhow provide useful input just let me know.

@wilkinsona
Copy link
Member

We might be able to enhance MockitoAopProxyTargetInterceptor to bypass the async stuff within a call to when or given.

@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Aug 10, 2016
@philwebb philwebb added this to the 1.4.1 milestone Aug 11, 2016
@wilkinsona
Copy link
Member

Unfortunately, we're not within when or given when the problematic call to the mock is made. It's the result of the problematic call that's then passed into when or given by which time it's too late.

@wilkinsona
Copy link
Member

wilkinsona commented Aug 15, 2016

I think the best we can do here is to fail fast when we encounter @Async on an interface that's going to be mocked. However, that would be a false positive if the test doesn't also have async processing enabled so we'd need to detect that too. We'd also need to consider extra interfaces configured via MockBean's extraInterfaces attribute.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Aug 15, 2016
@patrickherrera
Copy link

I believe I had the same issue when using a @Cacheable method on a mocked bean. I was using a straight class via @EnableCaching(proxyTargetClass=true) and the caching system would complain because it was getting null values for the cache key when the mocked method was called.

I changed the bean to use an interface and dropped the 'proxyTargetClass=true' attribute and it now works fine as a mock.

Perhaps at this stage it might be worth just making a mention in the documentation and seeing how much it impacts people?

@snicoll
Copy link
Member

snicoll commented Aug 16, 2016

I was expecting caching to be affected by this as well. Thanks for sharing that feedback @patrickherrera !

wilkinsona added a commit to wilkinsona/spring-boot that referenced this issue Aug 17, 2016
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 (spring-projectsgh-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 (spring-projectsgh-6573, spring-projectsgh-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 (spring-projectsgh-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 (spring-projectsgh-6405, spring-projectsgh-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 rather than
via a bean definition.
@wilkinsona wilkinsona added type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review labels Aug 18, 2016
@wilkinsona wilkinsona removed their assignment Aug 30, 2016
@snicoll snicoll closed this as completed in 0e00a49 Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

6 participants