Skip to content

Fixes #974: Fix to get correct stubbing location with inline mocking #979

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 19, 2017
Merged

Fixes #974: Fix to get correct stubbing location with inline mocking #979

merged 4 commits into from
Mar 19, 2017

Conversation

tmurakami
Copy link
Contributor

  • Fix StackTraceFilter to support inline mocking
  • Add one test to StackTraceFilterTest
  • Add StubbingLocationTest into inline subproject

This PR fixes #974.

@codecov-io
Copy link

codecov-io commented Mar 6, 2017 •
edited
Loading

Codecov Report

Merging #979 into release/2.x will increase coverage by 0.01%.
The diff coverage is 95%.

@@                Coverage Diff                @@
##             release/2.x     #979      +/-   ##
=================================================
+ Coverage           86.8%   86.82%   +0.01%     
- Complexity          2275     2281       +6     
=================================================
  Files                287      287              
  Lines               5769     5783      +14     
  Branches             675      677       +2     
=================================================
+ Hits                5008     5021      +13     
  Misses               571      571              
- Partials             190      191       +1
Impacted Files Coverage Δ Complexity Δ
...rnal/creation/bytebuddy/InterceptedInvocation.java 86% <100%> (ø) 22 <0> (ø) ⬇️
...a/org/mockito/internal/debugging/LocationImpl.java 100% <100%> (ø) 6 <3> (+2) ⬆️
...rnal/creation/bytebuddy/MockMethodInterceptor.java 77.77% <100%> (+0.85%) 7 <2> (+1) ⬆️
.../internal/creation/bytebuddy/MockMethodAdvice.java 62.61% <88.88%> (+2.41%) 14 <3> (+3) ⬆️

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 5815d4c...33b601e. Read the comment docs.

mockitoguy
mockitoguy approved these changes Mar 7, 2017
View reviewed changes
Copy link
Member

@mockitoguy mockitoguy left a comment

Choose a reason for hiding this comment

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

Very nice change, deep in the internals of Mockito. Thank you very much for fixing it!!!

I feel that it would be cleaner if we could put this logic in the StackTraceCleaner implementation somehow. StackTraceCleaner is a public API and we cannot just change it. Given that we don't need to change this code often I'm ok with a little bit overlap between cleaner and filter.

Can you describe how did you test this change? (does the unstubbed method finder work ok now, does filtering work ok in general - verification errors).

Approved. Thanks again!

@tmurakami
Copy link
Contributor Author

To solve the issue #974, I thought that if one stack trace element is the method frame representation of MockMethodAdvice#handle, the next element must be skipped.
Since the StackTraceCleaner API cannot handle the two related elements, I abandoned to move this logic into DefaultStackTraceCleaner.

Can you describe how did you test this change?

I ran ./gradlew clean build with this PR and confirmed that there was no problem.
Please point out me if you need additional tests.

TimvdLippe
TimvdLippe reviewed Mar 18, 2017
View reviewed changes
Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Changes seems fine, the refactoring of the ClassMethodPair made it a bit hard to review. In the future, if you can do without such an unrelated refactor, please do. It is easier to review 2 separate pull requests than 1 big one :P

In the tests I don't see a usage of the inline mockmaker. Per #752 this might not be tested. Could you explicitly try it with the inline mockmaker?

Also I would like @raph to sign of this change, but he is fairly busy right now. Thanks for your patience!

@tmurakami
Copy link
Contributor Author

@TimvdLippe, thank you for your review.

I did not know that mock maker can be switched by environmental variables.
Tests failed with inline-mock-maker, so I'm working on it.

@tmurakami
Copy link
Contributor Author

tmurakami commented Mar 18, 2017 •
edited
Loading

I've just updated.

I ran the following command on release/2.x and this PR.

$ git branch > log.txt;
$ MOCK_MAKER=mock-maker-inline ./gradlew -Dorg.gradle.parallel=false -x removeTestResources --continue clean ciBuild 2>&1 | tee -a ~/log.txt;
$ ./gradlew removeTestResources

On release/2.x (log):

1830 tests completed, 8 failed, 60 skipped

On this PR (log):

1830 tests completed, 8 failed, 60 skipped

The same tests failed.
(This is difference of two logs)
There is no failure caused by this PR.

In the previous code PartialMockingWithSpiesTest#shouldStackTraceGetFilteredOnUserExceptions failed.
I gave up on fixing StackTraceFilter and moved the skip logic to MockMethodAdvice.

TimvdLippe
TimvdLippe approved these changes Mar 19, 2017
View reviewed changes
Copy link
Contributor

@TimvdLippe TimvdLippe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This seems fine to me. I would like to have @raphw sign this of as well before merging

@TimvdLippe TimvdLippe requested a review from raphw March 19, 2017 14:10
raphw
raphw approved these changes Mar 19, 2017
View reviewed changes
Copy link
Member

@raphw raphw left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

@TimvdLippe TimvdLippe merged commit ccd5e85 into mockito:release/2.x Mar 19, 2017
@tmurakami tmurakami deleted the fix_issue_974 branch March 20, 2017 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockitoguy mockitoguy mockitoguy approved these changes

@raphw raphw raphw approved these changes

@TimvdLippe TimvdLippe TimvdLippe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MockitoJUnitRunner.StrictStubs does not detect 'Unnecessary Stubbing' when inline mock maker is enabled
5 participants
@tmurakami @codecov-io @mockitoguy @raphw @TimvdLippe