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

Offer a way for MockitoTestExecutionListener to enable strict stubbing #33318

Closed
jschmied opened this issue Dec 16, 2019 · 30 comments
Closed

Offer a way for MockitoTestExecutionListener to enable strict stubbing #33318

jschmied opened this issue Dec 16, 2019 · 30 comments
Assignees
Labels
in: test Issues in the test module type: enhancement A general enhancement
Milestone

Comments

@jschmied
Copy link

jschmied commented Dec 16, 2019

Running tests with SpringRunner does not report unused stubs like MockitoJUnitRunner does.

In MockitoTestExecutionListener.initMocks it should use

Mockito.mockitoSession()
  .initMocks(this)
  .strictness(Strictness.STRICT_STUBS)
  .startMocking();

to enable strict mocking.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 16, 2019
@bclozel
Copy link
Member

bclozel commented Dec 16, 2019

This feature is still being debated by the community in mockito/mockito#769, and I'm not sure we're in a position to force that choice right now on the Spring Boot community. Looking at the comments in that issue, it seems that some developers had issues while applying this to their projects; arguable, some of those might be linked to their setup or even bad practices.

If we do consider that change, we should do that in a proper milestone and document that change properly. I'm marking this issue for team attention and we'll discuss this.

In the meantime @jschmied, don't hesitate to point us to official Mockito documentation/best practices about this feature.

@jschmied
Copy link
Author

jschmied commented Dec 17, 2019

The discussion you are refering ist ourdated. Since Mockito 2.+, strict stubbing is used by default when initializing our mocks using either of:

MockitoJUnitRunner
MockitoJUnit.rule()

Spring ist using Mockito.initMocks() instead of Mockito.mockitoSession().initMocks(this).strictness(Strictness.STRICT_STUBS).startMocking(); what is causing this problem.

See: https://www.baeldung.com/mockito-unnecessary-stubbing-exception

So by default (We are at Mockito 2.23 in Spring Boot 2.1) all plain JUnit test use strict stubbing by default but all Sprint tests do not.

This behaviour is inconstent and anoying.

Of course, enabling it would possibly force users to clean up their spring tests. All plain JUnit test had to be cleaned up with switch to spring boot 2.x already.

It would be nice to be able it at least by some property or annotation (or in my oppinon, better switch it on and give user the possibility to disable it easily)

@wilkinsona
Copy link
Member

As far as I can tell, the issue to which @bclozel linked remains accurate. MockitoJUnit.rule() calls JUnitRule(Plugins.getMockitoLogger(), Strictness.WARN) in Mockito 2.28 and continues to do so in Mockito 3.1.

@jschmied
Copy link
Author

jschmied commented Dec 17, 2019

I have seen nobody using MockitoJUnit.rule(), anybody uses MockitoJUnitRunner.

Fact is: if you have two JUnit test, one with

@RunWith(MockitoJUnitRunner.class)

one with

@RunWith(SpringRunner.class)
@SpringBootTest()

the first one will fail when there are unnecessary stubs (default is STRICT), the second one will pass.

You can test easily.

@wilkinsona
Copy link
Member

wilkinsona commented Dec 17, 2019

On further inspection, as far as I can tell, Mockito does not use STRICT_STUBS by default in any situation. As with the rule, the runner's default behaviour is to report problems to the console. This is the equivalent of Strictness.WARN. Strictness.STRICT_STUBS is "planned as default for Mockito v4".

I think we could safely switch to Strictness.WARN. It will result in some additional console output containing hints on how to improve the tests' usage of Mockito but, unlike STRICT_STUBS, it will not cause test failures. I think there's even an argument to be made to do this in a Spring Boot maintenance release so that we're aligned with Mockito's default behaviour. We would then switch to Strictness.STRICT_STUBS by default when we upgrade to a version of Mockito that does so.

@jschmied
Copy link
Author

This would be nice. I usually run JUnit from eclipse first and unused stubs make the tests failing in the IDE. I did not test this as part of maven build on Jenkins. So you might be right.

@wilkinsona
Copy link
Member

@jschmied Could you share some example tests with us please? It would be useful to see exactly what you are seeing and to confirm that Mockito's behaviour matches its documentation.

@jschmied
Copy link
Author

jschmied commented Dec 17, 2019

Sure.
This one fails with UnnecessaryStubbingException:

@RunWith(MockitoJUnitRunner.class)
public class TestJUnit {
	@Test
	public void test1() {
		List<Object> list = mock(List.class);
		when(list.get(anyInt())).thenReturn(new Object());
	}
}

This one passes:

@RunWith(SpringRunner.class)
@SpringBootTest
public class TestSpring {

	@Test
	public void test1() {
		List<Object> list = mock(List.class);
		when(list.get(anyInt())).thenReturn(new Object());
	}

}

Only the annotations on top differ

@wilkinsona
Copy link
Member

Thank you, @jschmied. I can see now that I was wrong before. Sorry. Mockito does fail by default when using MockitoJUnitRunner. It does not fail by default when using MockitoRule but does output some hints to the console.

The concept of strictness differs between the runner and the rule. There are three variants of the runner:

  • Silent
  • Strict
  • StrictStubs

There are three variants of Strictness used by the rule:

  • LENIENT
  • WARN
  • STRICT_STUBS

The Silent runner is equivalent to the LENIENT rule. The StrictStubs runner is equivalent to the STRICT_STUBS rule. The Strict runner and the WARN rule are not equivalent. The former fails when there is unnecessary stubbing while the latter only outputs hints to the console.

An equivalent of the Strict runner doesn't appear to be available to us as there's no equivalent Strictness. STRICT_STUBS would be too much as it's stricter than Mockito is by default even when using the runner. Using WARN by default in Boot feels reasonable to me. It matches the default of the rule and is no more strict than the default of the runner.

@philwebb
Copy link
Member

I think there's currently too much risk in changing our defaults at this point. The MockitoTestExecutionListener.initMocks method currently just delegates to MockitoAnnotations.initMocks (from the org.mockito package).

As soon as the Mockto team decide that MockitoAnnotations.initMocks should switch to a stricter form, we'll pick it up by virtue of an upgrade.

The only problem that we see with our current approach is that we don't offer a good way to change the defaults for an individual test. I think we should investigate offering a way to switch the strictness for an individual test.

@philwebb philwebb changed the title MockitoTestExecutionListener should enable strict stubbing Offer a way for MockitoTestExecutionListener to enable strict stubbing Dec 20, 2019
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 20, 2019
@JoaaoVerona
Copy link

JoaaoVerona commented May 1, 2020

+1. It would be nice if we could change the strictness used in a test. Even better if you could support @MockitoSettings(strictness = STRICT_STUBS).

@mminot-yseop
Copy link

mminot-yseop commented Oct 23, 2020

Indeed, it took me a while to understand why my unit and integration tests were behaving differently, and noticing that @MockitoSettings(strictness = STRICT_STUBS) didn’t help with @SpringBootTest was quite a letdown. For now I’ll have to resort to something like…

@AfterEach
void afterEach() {
    // Only stubbed (or otherwise expected) calls allowed.
    ignoreStubs(/* explicit list of mockbeans here */);
    verifyNoMoreInteractions(/* the same list */);
}

This is not really ideal (and does not provide the usual nice helpful errors for unnecessary stubbings).

@iekdosha
Copy link

iekdosha commented Nov 23, 2020

A workaround to manually set the strictness would help.

@enepomnyaschih
Copy link

@mminot-yseop A couple of remarks:

  1. Probably reset(/* the same list */) is necessary, or the test should be annotated with @DirtiesContext. Otherwise mocks get preserved between tests (or maybe I did something wrong when tested it).
  2. This workaround doesn't verify unnecessary stubbings.

Due to these issues, I switched back to vanilla Mockito and its @mock and @Injectmocks annotations.

@mminot-yseop
Copy link

Probably reset(/* the same list */) is necessary, or the test should be annotated with @DirtiesContext. Otherwise mocks get preserved between tests (or maybe I did something wrong when tested it).

Hum I was using this in conjunction with @Mock and co., and it seemed fine. I virtually never had to use reset in my life.

This workaround doesn't verify unnecessary stubbings.

I said so in my message, you know 😁 ↓

does not provide the usual nice helpful errors for unnecessary stubbings

Yeah probably not perfect at all.
I kinda gave up on this since that first message and bit the bullet of “no strictness”. 😢

@robwbruder
Copy link

A workaround to enable "strict stubs" in @SpringBootTest tests can be found here: https://stackoverflow.com/a/75388020/20670189

@vpavic
Copy link
Contributor

vpavic commented Feb 11, 2023

Since Spring Boot usually aligns with the defaults of technologies it integrates (and in some cases even maintains the tests that verify the alignment), what's the reason not to do the same here?

Having substantially different behavior of mocks between tests set up with Mockito's own facilities (for example, @ExtendWith(MockitoExtension.class)) and ones set up by Spring Boot is quite annoying.

@wilkinsona
Copy link
Member

There are three different ways to do things with Mockito. In addition to @ExtendWith(MockitoExtension.class), there's also a JUnit runner and MockitoAnnotations.initMocks. Each has different default behavior. Boot aligns with MockitoAnnotations.initMocks as that's what it calls under the covers.

@mminot-yseop
Copy link

There are three different ways to do things with Mockito. In addition to @ExtendWith(MockitoExtension.class), there's also a JUnit runner and MockitoAnnotations.initMocks. Each has different default behavior. Boot aligns with MockitoAnnotations.initMocks as that's what it calls under the covers.

That’s an interesting point. However, this issue, I think, is not exactly about the default behavior but mostly about the lack of simple, built-in way to switch to the “strict” mode in a Boot test, and therefore remains valid IMHO.

Sidenote: I kinda fail to see in which contexts it can be interesting not to be strict. 🤔 (But I’m no expert of course, just your average user.) The most extreme cases I saw so far were tests with at most one or two annoying mocks that are not-so-relevant to the things being tested (while still being relied upon here and there), but in such cases there’s still lenient().when(…), which I generally use (sparingly) along with code comments justifying its presence.

@wilkinsona
Copy link
Member

@mminot-yseop Apologies for the confusion. I was responding to @vpavic. We agree that this issue remains valid. It would have been closed if we thought otherwise.

@mminot-yseop
Copy link

@mminot-yseop Apologies for the confusion. I was responding to @vpavic. We agree that this issue remains valid. It would have been closed if we thought otherwise.

Sure 😄 No real confusion, I guess I was just being talkative.
Now that I think about it:

there's also a JUnit runner

Isn’t that the JUnit 4 way of doing things, though? I got the impression that nearly all modern-ish projects rely on v5, and therefore use @ExtendWith(MockitoExtension.class). As for manual calls to initMocks, there seem to be like 3–4 occurrences of that in my company’s codebase so far (including one in a terribly old and dead project, and some really dubious uses), But perhaps it’s way more common in some other, slightly esoteric contexts and projects, I dunno. (Ran ag initMocks --java ~/workspace/)

@vpavic
Copy link
Contributor

vpavic commented Feb 13, 2023

@wilkinsona Sorry, but I disagree with your reasoning here. Mockito clearly documents (section 40. Improved productivity and cleaner tests with "stricter" Mockito) that:

Starting with version 2.1 Mockito has been getting new features that nudge the framework towards "strictness".

From end user perspective, it shouldn't matter which way of configuring Mockito does Spring Boot use internally, but rather what Mockito's user-facing configuration facilities do.

These days, JUnit Platform is the dominant unit testing framework and @ExtendWith(MockitoExtension.class) is the preferred method of setting up Mockito there, which results in strict mocks. As this is what most users will end up with when using Mockito in tests that don't involve Spring Boot's testing support, IMO that's what Spring Boot should align with as well.

@wilkinsona
Copy link
Member

We're at risk of going round in circles here.

but rather what Mockito's user-facing configuration facilities do.

MockitoAnnotations.openMocks (the non-deprecated replacement for initMocks) is also a user-facing configuration facility. It's documented on the page to which you have already linked.

As this is what most users will end up with when using Mockito in tests that don't involve Spring Boot's testing support, IMO that's what Spring Boot should align with as well.

This ignores the risk that @philwebb has already described above. Minimising that risk requires a setting that allows the strictness to be configured. That's what this issue is tracking.

@vpavic
Copy link
Contributor

vpavic commented Feb 13, 2023

Both this issue and the comment stating risks are 3+ years old now. In the meanwhile there has been no evidence that Mockito will stop favoring the strictness by default, quite the opposite. I think that should warrant re-evaluation of the goals of this ticket, which IMO should be to flip the default behavior and offer a way to disable strict stubbing.

I'm aware that MockitoAnnotations.openMocks is documented (and also preferred by the team in Mockito-based tests within this project) but since JUnit Platform has become dominant in organizations I've worked with I literally haven't seen anyone prefer this style over using MockitoExtension. The initMocks approach was popular with JUnit 4 due to limitations of @RunWith however JUnit Platform's @ExtendWith being both repeatable and accepting an array of extensions (in contrast to @RunWith accepting a single runner) it's simply too low-level and verbose to be attractive to the developers these days.

You don't have to take my word on this, since a very simple GitHub code search can easily show which approach is the more popular one:

That's an order of magnitude difference and I'd argue that the figures are even tilted to benefit the openMocks approach here, since some of the code was definitely migrated from JUnit 4 to JUnit Platform in simplest possible fashion and is not utilizing @ExtendWith(MockitoExtension.class) for that reason.

@snicoll
Copy link
Member

snicoll commented Aug 5, 2024

With @MockBean and friends being moved over framework, I am going to move this issue as well.

@simonbasle can you please review the history of this issue? Perhaps a chance for us to consider this in some form for Spring Framework 6.2 still.

@snicoll snicoll added the in: test Issues in the test module label Aug 5, 2024
@snicoll snicoll transferred this issue from spring-projects/spring-boot Aug 5, 2024
@snicoll snicoll added this to the General Backlog milestone Aug 5, 2024
@simonbasle
Copy link
Contributor

simonbasle commented Aug 6, 2024

I think we can use 6.2 as an opportunity to change Spring Framework's newly introduced @MockitoBean default to the strict style, with an additional class-level annotation that can be used to switch to a lenient style.

I was not able to find a way to account for the @MockitoSettings annotation since it is meta-annotated with @ExtendWith(MockitoExtension.class), but the usage pattern would be pretty similar:

@SpringJUnitConfig(Config.class)
@MockitoBeanSettings(Strictness.LENIENT)
public class MyIntegrationTests {
	@Test
	public void unusedStubbingNotReported() {
		var list = Mockito.mock(List.class);
		Mockito.when(list.get(Mockito.anyInt())).thenReturn(new Object());
	}
}

@simonbasle simonbasle self-assigned this Aug 6, 2024
@simonbasle simonbasle modified the milestones: General Backlog, 6.2.0-M7 Aug 6, 2024
@sbrannen
Copy link
Member

sbrannen commented Aug 7, 2024

I was not able to find a way to account for the @MockitoSettings annotation since it is meta-annotated with @ExtendWith(MockitoExtension.class),

It's indeed unfortunate that @MockitoSettings auto-registers the MockitoExtension, but then again... @MockitoSettings resides in the mockito-junit-jupiter module. So it's somewhat expected.

but the usage pattern would be pretty similar:

Yes, your proposal looks good. 👍

@sbrannen sbrannen changed the title Offer a way for MockitoTestExecutionListener to enable strict stubbing Offer a way for MockitoTestExecutionListener to enable strict stubbing Oct 13, 2024
simonbasle added a commit that referenced this issue Oct 16, 2024
This change remove the support for Mockito annotations, `MockitoSession`
and opening/closing of mocks that was inherited from Boot's `@MockBean`
support, as well as the switch to `MockitoSession` made in 1c893e6.

Attempting to take responsability for things Mockito's own JUnit
Jupiter extension does better is not ideal, and we found it leads to
several corner cases which make `SpringExtension` and `MockitoExtension`
incompatible in the current approach.

Instead, this change refocuses our Mockito bean overriding support
exclusively on aspects specific to the Framework. `MockitoExtension`
will thus be usable in conjunction with `SpringExtension` if one needs
to use `@Captor`/`@InitMocks`/`@Mock`/`@Spy` or other Mockito utilities.

See gh-33318
Closes gh-33692
@simonbasle
Copy link
Contributor

Unfortunately, we had to backtrack on this in 6.2.0-RC2 due to various blocking issues with MockitoSession and MockitoExtension integration in a SpringExtension-enabled test suite. We'll reach out to the Mockito team to explore possible improvements in the Spring Framework 7 timeframe.

@vpavic
Copy link
Contributor

vpavic commented Feb 13, 2025

Is there an issue that tracks the re-introduction of strict stubbing as the default behavior?

@sbrannen
Copy link
Member

Is there an issue that tracks the re-introduction of strict stubbing as the default behavior?

There wasn't one, but there is now. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests