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

Improved sensitivity of potential stubbing problem #1539

Merged
merged 2 commits into from Nov 25, 2018

Conversation

mockitoguy
Copy link
Member

@mockitoguy mockitoguy commented Nov 22, 2018

This change improves the developer experience with strict stubbing. It is now possible to stub the same method with different argument multiple times in the test. Previously, we threw PotentialStubbingProblem exception in this scenario:

when(mock.foo(1)).thenReturn(1);
when(mock.foo(2)).thenReturn(2); // <- no longer throws PotentialStubbingProblem (false negative)

This reduces the number of false negatives reported by strict stubbing.

Fixes #1522, #1496, partially #769, #720

@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #1539 into release/2.x will decrease coverage by 0.04%.
The diff coverage is 63.63%.

Impacted file tree graph

@@                Coverage Diff                @@
##             release/2.x    #1539      +/-   ##
=================================================
- Coverage          88.58%   88.53%   -0.05%     
- Complexity          2402     2406       +4     
=================================================
  Files                299      299              
  Lines               6044     6053       +9     
  Branches             734      737       +3     
=================================================
+ Hits                5354     5359       +5     
- Misses               511      513       +2     
- Partials             179      181       +2
Impacted Files Coverage Δ Complexity Δ
.../internal/junit/DefaultStubbingLookupListener.java 96% <100%> (+0.16%) 12 <0> (+1) ⬆️
...a/org/mockito/internal/debugging/LocationImpl.java 89.47% <50%> (-10.53%) 8 <1> (+1)
...ternal/exceptions/stacktrace/StackTraceFilter.java 84.61% <50%> (-15.39%) 7 <2> (+2)

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 30d3127...90c89c3. Read the comment docs.

&& s.getInvocation().getMethod().getName().equals(invocation.getMethod().getName())
//If stubbing and invocation are in the same source file we assume they are in the test code,
// and we don't flag it as mismatch:
&& !s.getInvocation().getLocation().getSourceFile().equals(invocation.getLocation().getSourceFile())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not work if the setup is performed in a base class or helper class. Can we figure out a solution for these situations as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is improving the current behavior a lot already. We can always improve it further without blocking this PR. Feel free to suggest a solution!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimvdLippe, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not been able to come up with a proper solution, other than using a different stubbing mechanism. I fear that, no matter how hard we try, we will keep on receiving issues on this feature, where users will run into all the edge-cases.

That said, this does improve the current situation, so we can push forward. I just think we have to solve the fundamental problem first to be able to prevent these kind of issues popping up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for review!

@TimvdLippe TimvdLippe dismissed their stale review November 24, 2018 17:42

This change is an improvement to the status quo.

This change improves the developer experience when stubbing same method with different argument, in the same test class. Example:

when(mock.foo(1)).thenReturn(1);
when(mock.foo(2)).thenReturn(2); // <- no longer throws PotentialStubbingProblem (false negative)

This reduces the number of false negatives reported by strict stubbing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants