Support local parameters in test methods with JUnit Jupiter

[TimvdLippe]

Note that this implementation differs from the prototype implementation of the JUnitTeam (https://github.com/junit-team/junit5-samples/blob/7bf40178345d5ca837579c8ddb8c025401a98788/junit5-mockito-extension/src/main/java/com/example/mockito/MockitoExtension.java#L41-L73). Instead, it will do not do any parameter resolution between test methods.

Fixes #1348

[codecov-io]

Codecov Report

Merging #1350 into release/2.x will increase coverage by 0.08%.
The diff coverage is 100%.

@@                Coverage Diff                @@
##             release/2.x    #1350      +/-   ##
=================================================
+ Coverage          88.54%   88.62%   +0.08%     
+ Complexity          2391     2359      -32     
=================================================
  Files                296      292       -4     
  Lines               6005     5951      -54     
  Branches             727      719       -8     
=================================================
- Hits                5317     5274      -43     
+ Misses               507      497      -10     
+ Partials             181      180       -1

Impacted Files	Coverage Δ	Complexity Δ
...nternal/configuration/MockAnnotationProcessor.java	100% <100%> (ø)	7 <1> (+1)	⬆️
...mockito/internal/stubbing/OngoingStubbingImpl.java	90% <0%> (-1.67%)	5% <0%> (-1%)
...ava/org/mockito/internal/stubbing/StubberImpl.java	86.36% <0%> (-0.6%)	19% <0%> (ø)
...ito/internal/stubbing/InvocationContainerImpl.java	94.54% <0%> (-0.46%)	28% <0%> (-3%)
.../internal/junit/DefaultStubbingLookupListener.java	95.65% <0%> (-0.19%)	11% <0%> (ø)
...rg/mockito/internal/creation/MockSettingsImpl.java	92.77% <0%> (-0.18%)	43% <0%> (-1%)
...o/internal/creation/settings/CreationSettings.java	97.77% <0%> (-0.1%)	21% <0%> (-1%)
src/main/java/org/mockito/Mockito.java	96.55% <0%> (-0.06%)	39% <0%> (-1%)
...rc/main/java/org/mockito/internal/MockitoCore.java	97.7% <0%> (-0.06%)	33% <0%> (-2%)
.../org/mockito/internal/handler/MockHandlerImpl.java	100% <0%> (ø)	12% <0%> (ø)	⬇️
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5ebb9c...68be01f. Read the comment docs.

[sbrannen]

Instead, it will do not do any parameter resolution between test methods.

I always thought that was a cool feature of the original prototype, but I can understand your rationale for omitting that feature; however, I'd make sure it's very well documented in the user guide, etc. for Mockito.

Otherwise, users will likely get confused quite quickly.

[sbrannen]

In contrast to field names -- which are always user-defined -- I would also make a special note in the documentation that the default name for a mock created for a parameter will be something very ugly like arg0 instead of the user-defined name... unless the code has been compiled on Java 8+ with the -parameters flag.

[sbrannen]

Regarding that latter, in the prototype we added explicit support for determining the mock name here:

https://github.com/junit-team/junit5-samples/blob/7bf40178345d5ca837579c8ddb8c025401a98788/junit5-mockito-extension/src/main/java/com/example/mockito/MockitoExtension.java#L64

... and we did not create the mock with a custom name if the user did not provide a custom name.

So I'd suggest Mockito do something similar.

[TimvdLippe]

I'd make sure it's very well documented in the user guide, etc. for Mockito

I think the javadoc of MockitoExtension is quite explicit about this, do you have any suggestions to extend it?

I would also make a special note in the documentation that the default name

Good call, will do

we did not create the mock with a custom name if the user did not provide a custom name

We can use the type name indeed.

[sbrannen]

I think the javadoc of MockitoExtension is quite explicit about this, do you have any suggestions to extend it?

Well, as the code is currently implemented, supportsParameter() will return true for any parameter annotated with @Mock, whether such a parameter is in a constructor, @BeforeAll/@AfterAll, @BeforeEach/@AfterEach, @Test method etc.

And it sounds like you wish to restrict usage to @Test, @TestFactory, @ParameterizedTest, and @RepeatedTest methods.

Or maybe you don't want to restrict it?

But it's unclear what is supported and what isn't.

I think your goal is to restrict sharing of parameter-injected mock instances between setup and test methods. So you might want to restrict mock injection for lifecycle callback methods.

On the other hand, it would be a perfectly valid use case to have a mock injected into a constructor and then store it in a private final field (i.e., immutability).

In summary, you'll need to think through the possible use cases and document which ones you actually support, and... the implementation should only return true from supportsParameter() if Mockito actually supports the use case.

[TimvdLippe]

Oh I was not aware of all these cases, I thought it was only for test methods. Have to think about what we do want to support. (Maybe it all magically works already with this implementation)

[sbrannen]

Sure.... I'd recommend you play around with it a bit to get a feel for what's possible.

Then you'll be better equipped to make an informed decision. 😉

[TWiStErRob]

+1 for sharing, I think @BeforeEach void f(@Mock x) @Test void f(@Mock x) could be the same object. @TimvdLippe as I read you're clearing the session after each test, so sharing between methods would affect a single test method execution.

[TimvdLippe]

@TWiStErRob Please see #1348 (comment) for the full explanation

[sbrannen]

@TimvdLippe,

Maybe it all magically works already with this implementation

Yes, that should be the case.

But I still think the Mockito Team should provide guidance for end users demonstrating best practices when using the MockitoExtension for JUnit Jupiter.

For example, if it were my project, I'd make sure that there are tests in place and documented examples demonstrating parameter injection for constructors, lifecycle methods, testable methods (e.g., @Test, @TestFactory, etc.), and I'd cover the use of @TestInstance(PER_CLASS) semantics as well.

I'm sure that either I or someone else from the JUnit team would be happy to review your work before you publish it. 😉

[TimvdLippe]

For reference junit-team/junit5#1345 is the issue that is blocking this PR for now, but the upstream issue is actively being worked on atm.

[mockitoguy]

Thank you guys for working on this!

[TimvdLippe]

The JUnit issue has been resolved on their master (junit-team/junit5#1345 (comment)) so we have to wait for a new release and then I can continue working on this PR.

[sbrannen]

@TimvdLippe, I would't hold off on working on this PR just to wait on JUnit Jupiter 5.2 to be released.

That will literally take months. Plus it would prevent Mockito users from using the feature with JUnit Jupiter 5.1.x (unless we backport the convenience methods to 5.1.x).

For example, in Spring Framework 5.0.5 (likely being released today), I already added custom support for the JDK bug so that Spring users aren't affected by the issue even on JUnit Jupiter 5.0.x.

[TimvdLippe]

unless we backport the convenience methods to 5.1.x

As a user, I would strongly appreciate a backport. Have to think whether we would like to maintain this code ourselves (like Spring does) or whether we simply note the limitation that this will be addressed in the next release of JUnit.

[sbrannen]

Sure... maintaining the "workaround" is a personal decision you have to make.

As for appreciating a backport... feel free to voice your opinion on the related JUnit Jupiter issue (which I reopened for that purpose). 😉

[TimvdLippe]

Per junit-team/junit5#1345 (comment) this will be backported to JUnit 5.1.1. We just have to wait till that release is out and then I can finish up this PR 🎉

[thekalinga]

@TimvdLippe Looks like 5.1.1 released couple of hours back :)

https://search.maven.org/#artifactdetails%7Corg.junit.jupiter%7Cjunit-jupiter-engine%7C5.1.1%7Cjar

[TimvdLippe]

Awesome, constructor parameter resolution has been fixed! @mockito/core PTAL

[sbrannen]

👍

[ammerritt]

Will this be getting released soon? Thank you!

[TimvdLippe]

This is still awaiting review of @mockito/core

[mockitoguy]

I finished reviewing and I'm happy to merge! There's some issue with our Travis build at the moment but we should merge soon!

[mockitoguy]

I rebased and will wait for Travis feedback before merging.