-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fixes #2331 #2369
Fixes #2331 #2369
Conversation
Has conditionally on basis of whether default answer is pass to actual methods made hash code and equals pass to actual methods.
Has conditionally on basis of whether default answer is pass to actual methods made hash code and equals pass to actual methods. Intellij seems to had only pushed import and not the test
Codecov Report
@@ Coverage Diff @@
## main #2369 +/- ##
============================================
- Coverage 84.90% 84.77% -0.14%
+ Complexity 2794 2792 -2
============================================
Files 328 328
Lines 8480 8490 +10
Branches 1025 1026 +1
============================================
- Hits 7200 7197 -3
- Misses 1004 1015 +11
- Partials 276 278 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not ideal to have another branch, but I think it is the best way forward. Only 1 question, but the rest LGTM
@@ -77,7 +77,7 @@ public String toString() { | |||
|
|||
@Override | |||
public boolean matches(Invocation candidate) { | |||
return invocation.getMock().equals(candidate.getMock()) | |||
return invocation.getMock() == candidate.getMock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I follow why this line was changed. Could you elaborate as to why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the test PartialMockingWithSpiesTest.shouldAllowStubbingOfMethodsThatDelegateToOtherMethods.
Now the call to getName is handled by MockMethodInterceptor.DispatcherDefaultingToRealMethod.interceptSuperCallable here the interceptor will not be null and it will go to interceptor.doIntercept. This finally leads to InvocationMatcher.matches
Now with changes in this pull request the call to equals is again delegated to MockMethodInterceptor.DispatcherDefaultingToRealMethod and it will come at same place. And this way it leads to stackoverflow. Earlier equals of spy was this==that. So i just replced equals with ==.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the explanation! That makes sense to me 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha Thanks. Had missed to put links to the classes.
Has conditionally on basis of whether default answer is pass to actual methods made hash code and equals pass to actual methods.
Checklist
including project members to get a better picture of the change
commit is meaningful and help the people that will explore a change in 2 years
Fixes #<issue number>
in the description if relevantFixes #<issue number>
if relevant