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 fields are not reset for JUnit 5 @Nested tests #12470

Closed
slu-it opened this issue Mar 13, 2018 · 25 comments
Closed

@MockBean fields are not reset for JUnit 5 @Nested tests #12470

slu-it opened this issue Mar 13, 2018 · 25 comments
Labels
theme: testing Issues related to testing type: bug A general bug
Milestone

Comments

@slu-it
Copy link

slu-it commented Mar 13, 2018

Hello Spring Boot team!

Today a colleague of mine came across strange behaviour when writing some JUnit 5 tests. It seems that @MochBean annotated fields, or more specifically the actual mocks behind that, are not reset for @Test methods residing in @Nested inner classes.

Example:

@SpringBootTest
@ExtendWith(SpringExtension.class)
class FailingTests {

    @MockBean
    ServiceB serviceB;
    @Autowired
    ServiceA cut;

    @Nested
    class NestedTest {

        @Test
        void mockWasInvokedOnce() {
            cut.foo();
            verify(serviceB, times(1)).bar();
        }

        @Test
        void mockWasInvokedTwice() {
            cut.foo();
            cut.foo();
            verify(serviceB, times(2)).bar();
        }

    }

}

One of these tests will fail because the mock will have been invoked 3 times at that point.

Tested with Spring Boot 2.0.1.BUILD-SNAPSHOT and JUnit 5.1.0 on 2018-03-13 at around 8:00 PM. (also tested with 2.0.0.RELEASE earlier)

I created a simple example application (thx start.spring.io ;) ) to reproduce the issue: https://github.com/slu-it/bug-spring-mockbean-reset

I also tried to find the cause of the issue, but I might have underestimated the complexity of the spring-test and spring-boot-test libraries... As far as I can see, the application context returned from the TestContext instance for tests within the @Nested class does not include references to the ServiceB bean. (From debugging MockitoTestExecutionListener)

When I debug the same location for a test without the @Nested part, the returned context contains a clearly identifiable instance of ServiceB.

I think the correct behaviour would be to have a parent child relationships between application contexts in case of inner classes (like @Nested JUnit 5 test classes).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 13, 2018
@slu-it
Copy link
Author

slu-it commented Mar 14, 2018

As a side note: I think this might also be the reason why Spring REST Docs did not work with @Nested JUnit 5 tests last I checked (somewhere around M5). But I'll have to see, if this is still true.

@wilkinsona wilkinsona self-assigned this Mar 14, 2018
@wilkinsona
Copy link
Member

Thanks for the minimal sample, @slu-it.

The problem is that NestedTest is using a separate application context to the one that's created for FailingTests. This means that the before and after test method callbacks for NestedTest.mockWasInvokedOnce() and NestedTest.mockWasInvokedTwice() are called with the application context created for NestedTest. It doesn't contain any mocks so there's nothing to reset. There's no link that I can see between the two contexts (no parent-child relationship, for example) so there's also no obvious way for us to get from the context for NestedTest to the context for FailingTests.

I'm rather surprised that there's a second application context created for NestedTest. My hunch was that it would share the context that was created for FailingTests. There's no mention of @Nested in the relevant section of Spring Framework's reference documentation so I'm not sure what is the right way to use @Nested with Spring Framework's testing support.

@sbrannen can you help us to get to the bottom of this one please?

@wilkinsona
Copy link
Member

A bit of hunting around led me to NestedTestsWithSpringAndJUnitJupiterTestCase.java . It's clear from those tests that the behaviour I described above is intentional and that my hunch was wrong. It's also taught me that in this case NestedTest should be annotated with @SpringBootTest. This gives it a chance to reuse the context that was created and cached for FailingTests. In fact, it would appear that it's only prevented from doing so due to there being no mock for serviceB in NestedTest. If FailingTests and NestedTest have the same mocks, the context is reused and the tests pass.

The change described above makes the test code look like this:

package example;

import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.test.context.junit.jupiter.SpringExtension;


@SpringBootTest
@ExtendWith(SpringExtension.class)
class FailingTests {

    @Autowired
    ServiceA cut;

    @MockBean
    ServiceB serviceB;

    @Nested
    @SpringBootTest
    class NestedTest {

        @MockBean
        ServiceB serviceB;

        @Test
        void mockWasInvokedOnce() {
            cut.foo();
            verify(serviceB, times(1)).bar();
        }

        @Test
        void mockWasInvokedTwice() {
            cut.foo();
            cut.foo();
            verify(serviceB, times(2)).bar();
        }

    }

}

I wonder if we can do something when calculating the context cache key for NestedTest that would allow us to reuse the context from FailingTests without having to declare the mock for serviceB in both places.

@slu-it
Copy link
Author

slu-it commented Mar 14, 2018

In the meantime I checked whether or not the Spring REST Docs issue still exists. Tested it with Spring Boot 2.0.1-BUILD-SNAPSHOT in this repository: https://github.com/slu-it/bug-spring-restdocs-nested-tests
Failing with @Nested tests as well. Might be related, or it might just be coincidence.. I'll create a separate issue for that in the Spring REST Docs repository and link it here.

@wilkinsona
Copy link
Member

wilkinsona commented Mar 14, 2018

Here's a possible fix. Can you take a look please, @philwebb?

With the fix in place, these failing tests are now actually working tests:

package example;

import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.test.context.junit.jupiter.SpringExtension;


@SpringBootTest
@ExtendWith(SpringExtension.class)
class FailingTests {

    @Autowired
    ServiceA cut;

    @MockBean
    ServiceB serviceB;

    @Nested
    @SpringBootTest
    class NestedTest {

        @Test
        void mockWasInvokedOnce() {
            cut.foo();
            verify(serviceB, times(1)).bar();
        }

        @Test
        void mockWasInvokedTwice() {
            cut.foo();
            cut.foo();
            verify(serviceB, times(2)).bar();
        }

    }

}

Adding a unit test for this is a challenge. As soon as I add an optional dependency on the JUnit Jupiter API to spring-boot-test, Eclipse defaults to using the JUnit 5 runner for all the tests in that project and you have to manually tell it to use JUnit 4 instead. I think that's too high a price to pay. We could add some use of @Nested to the JUnit 5 sample instead.

@slu-it
Copy link
Author

slu-it commented Mar 14, 2018

@wilkinsona is the second @SpringBootTest on the @Nested still necessary?

@wilkinsona
Copy link
Member

Yes. Without it, you get a second application context created for NestedTest. That second context is created as its configuration doesn't have anything to do with Spring Boot. Adding @SpringBootTest means that both NestedTest and FailingTests use SpringBootTestContextBootstrapper which finds example.Application as the basis for the test context's configuration in both cases.

@slu-it
Copy link
Author

slu-it commented Mar 15, 2018

Ok, makes sense.

@sbrannen
Copy link
Member

I'm rather surprised that there's a second application context created for NestedTest.

I'm actually very surprised by that. I thought that a @Nested would by default not have an ApplicationContext loaded for it.

In fact, I'm nearly 100% certain that an ApplicationContext will not be loaded for such a @Nested class when using the Spring TestContext Framework (TCF) without Spring Boot Test support.

So, perhaps it's just the magic of Spring Boot Test that is automagically finding the configuration for the test ApplicationContext.

But I'd have to debug that to see what's really going on.

My hunch was that it would share the context that was created for FailingTests.

Many people actually assume that would be the case, since the TCF supports configuration inheritance within a test class hierarchy.

However, Spring does not yet have any support for "inheriting" configuration from an enclosing class.

See SPR-15366 for details.

Unfortunately, resolving SPR-15366 is not so easy (if even feasible) since the entire annotation processing infrastructure within the core Spring Framework (and Spring portfolio projects for that matter) has zero support for searching for annotations on an enclosing class. 😞

There's no mention of @Nested in the relevant section of Spring Framework's reference documentation so I'm not sure what is the right way to use @nested with Spring Framework's testing support.

The "right way" for the time being is simply to redeclare all of the annotations declared on the enclosing class. A custom composed annotation can alleviate the pain here, but in general I admit that it is quite a nuisance.

@sbrannen
Copy link
Member

Yes. Without it, you get a second application context created for NestedTest. That second context is created as its configuration doesn't have anything to do with Spring Boot.

Hmmm... what is the second context created from?

For example, what @Configuration class(es) is/are used to create that context?

@sbrannen
Copy link
Member

sbrannen commented Mar 15, 2018

Here's a possible fix. Can you take a look please, @philwebb?

With the fix in place, these failing tests are now actually working tests:

That of course sounds nice for now, but I'm wondering what the implications would be if SPR-15366 is resolved at a later date. Maybe the result of SPR-15366 would be an opt-in feature for turning on "inheritance from enclosing class" in order to remain backwards compatible.

Will have to ponder it... 😉

@sbrannen
Copy link
Member

sbrannen commented Mar 15, 2018

@wilkinsona,

Without it, you get a second application context created for NestedTest.

If you have personally confirmed that behavior with Boot, I would appreciate if you could add a corresponding comment to SPR-15366.

TIA

@wilkinsona
Copy link
Member

wilkinsona commented Mar 15, 2018

Hmmm... what is the second context created from?

For example, what @Configuration class(es) is/are used to create that context?

I'm not sure as I didn't dig that far. I can do though.

If you have personally confirmed that behavior with Boot, I would appreciate if you could add a corresponding comment to SPR-15366.

I have. I'll do a bit more digging so I can provide some more details and comment on SPR-15366.

@sbrannen
Copy link
Member

Thanks, @wilkinsona! 👏

@sbrannen
Copy link
Member

sbrannen commented Mar 15, 2018

I'm not sure as I didn't dig that far. I can do though.

No need to do that: your input in SPR-15366 is sufficient.

@philwebb philwebb self-assigned this Mar 15, 2018
@wilkinsona wilkinsona removed their assignment Apr 30, 2018
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 15, 2018
@philwebb philwebb added this to the Backlog milestone Jun 15, 2018
@mbhave mbhave added the theme: testing Issues related to testing label Sep 11, 2018
@philwebb philwebb removed their assignment Sep 11, 2018
@philwebb philwebb added the status: blocked An issue that's blocked on an external project change label Oct 5, 2018
@philwebb philwebb modified the milestones: 2.1.x, General Backlog Oct 5, 2018
@philwebb
Copy link
Member

philwebb commented Oct 5, 2018

We're going to wait for the SPR-15366 fix before looking at this.

@szymonslosarczyk
Copy link

Hello, I'm having similar issue with BDDMockito.willThrow

willThrow(SomeException.class).given(service).someMethod(anyString());
It throws this exception in other nested tests, that use this method.

@hantsy
Copy link

hantsy commented Jul 5, 2020

Same problem here, add reset to reset all mocks in afterEach to make it works again. https://github.com/hantsy/spring-reactive-jwt-sample/blob/master/src/test/java/com/example/demo/PostControllerTest.java#L85-L91

@jehanzebqayyum
Copy link

Adding springbootcontext in nested classes doesnt work for me. I had to reset mock after each test

@bclozel bclozel added for: team-attention An issue we'd like other members of the team to review and removed for: team-attention An issue we'd like other members of the team to review labels Oct 12, 2020
@sbrannen
Copy link
Member

Please note that spring-projects/spring-framework#19930 has been resolved in time for Spring Framework 5.3 RC2.

See the "Deliverables" section of that issue (and the following commits) for details on what's included in RC2.

@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review and removed status: blocked An issue that's blocked on an external project change labels Oct 13, 2020
@philwebb philwebb modified the milestones: General Backlog, 2.4.x Oct 14, 2020
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Oct 14, 2020
@wilkinsona
Copy link
Member

wilkinsona commented Oct 27, 2020

I've tried a slightly updated version of the original sample with Spring Boot 2.4.0-M4 which uses Framework 5.3.0-RC2. The problem still occurs, however I think that's to be expected as we're not making use of any of the changes made in Framework to find test configuration in an enclosing class. This results in a cache miss as the merged context configurations for FailingTests and FailingTests.NestedTest differ. I think we need to make some changes to use searchEnclosingClass() and perhaps also update the @MockBean support to find annotated fields on an enclosing class. The latter would be a revision of this commit, updated to consider the @NestedTestConfiguration setting.

@sbrannen
Copy link
Member

Spring Framework 5.3 is now GA, and the searchEnclosingClass() method resides in the new org.springframework.test.context.TestContextAnnotationUtils class.

@wilkinsona, let me know if you run into any issues getting everything working with the new nested configuration support.

@wimdeblauwe
Copy link
Contributor

I just tried this with Spring Boot 2.4.0-RC1, but the @MockBean is not reset between tests like if I don't use nesting. You can view my example at https://github.com/wimdeblauwe/blog-example-code/tree/feature/nestedtests/nestedtests/src/test/java/com/wimdeblauwe/examples/nestedtests/music/web

There are 2 tests there: MusicRestControllerTest and MusicRestControllerNestedTest which only differ in the fact that nesting is used or not. The normal one succeeds, the nested one fails because the mock bean is not reset.

If you want me to create a separate issue, I can also do that.

@wilkinsona
Copy link
Member

Thanks for trying out the RC, @wimdeblauwe. Please do open a new issue. It's not immediately apparent to me what the difference is between this test class that was part of the commit that closed this issue and your example. It may be the use of @WebMvcTest vs @SpringBootTest.

@wimdeblauwe
Copy link
Contributor

Issue #23984 created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: testing Issues related to testing type: bug A general bug
Projects
None yet
Development

No branches or pull requests