-
-
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
Improve Varargs handling in AdditionalAnswers #2664
Conversation
@TimvdLippe any chance of a quick review to check I'm going in the right direction with this before I flesh out all the testing and clean up the code? |
src/main/java/org/mockito/internal/stubbing/answers/ReturnsArgumentAt.java
Show resolved
Hide resolved
src/main/java/org/mockito/internal/stubbing/answers/ReturnsArgumentAt.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #2664 +/- ##
============================================
+ Coverage 86.22% 86.26% +0.03%
- Complexity 2763 2776 +13
============================================
Files 314 315 +1
Lines 8290 8334 +44
Branches 1031 1037 +6
============================================
+ Hits 7148 7189 +41
- Misses 873 874 +1
- Partials 269 271 +2
Continue to review full report at Codecov.
|
057c795
to
7cbc653
Compare
Fixes: mockito#2644 Fixes issues around vararg handling for the following methods in `AdditionalAnswers`: * `returnsFirstArg` * `returnsSecondArg` * `returnsLastArg` * `returnsArgAt` * `answer` * `answerVoid` These methods were not correctly handling varargs. For example, ```java doAnswer(answerVoid( (VoidAnswer2<String, Object[]>) logger::info )).when(mock) .info(any(), (Object[]) any()); mock.info("Some message with {} {} {}", "three", "parameters", ""); ``` Would previously have resulted in a `ClassCastException` being thrown from the `mock.info` call. This was because the `answerVoid` method was not taking into account that the second parameter was a varargs parameter and was attempting to pass the second actual argument `"three"`, rather than the second _raw_ argument `["three", "parameters", ""]`.
7cbc653
to
459efad
Compare
avoiding having to cast to `Invocation`
@TimvdLippe any chance of a review please? |
I have a very busy week this week. I will likely get to this early next week. |
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.
Solid work and great test coverage! Only two small nits, but other than this it is ready to go 🎉
src/test/java/org/mockitousage/stubbing/StubbingWithAdditionalAnswersTest.java
Show resolved
Hide resolved
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.
Thanks!
No worries. Good to give something back. I've used Mockito for YEARS! |
@big-andy-coates I found a similar semi-related issue I think... With 4.7.0, this now works correctly: doAnswer(answerVoid((VoidAnswer2<String, Object[]>)
logger::info
)).when(mock)
.info(any(), (Object[]) any()); However, if I try to use this form which is slightly more ergonomic: doAnswer( answerVoid( (VoidAnswer2<String, Object[]>)
logger::info
) ).when( mock )
.info( any(), any( Object[].class ) ); ...the mocking will fail to intercept these method calls. 🤔 No error when creating the mock, but it just won't detect the method calls as expected. I presume this has again something to do with the varargs parsing. Do you want me to create a separate issue/repro repo for this? I'm not even sure if it's something that can be (easily) fixed, I just happened to see it now while trying to "clean up our code"... 😅 |
Fixes: #2644
Fixes issues around vararg handling for the following methods in
AdditionalAnswers
:returnsFirstArg
returnsSecondArg
returnsLastArg
returnsArgAt
answer
answerVoid
These methods were not correctly handling varargs. For example,
Would previously have resulted in a
ClassCastException
being thrown from themock.info
call. This was because theanswerVoid
method was not taking into account that the second parameter was a varargs parameter and was attempting to pass the second actual argument"three"
, rather than the second raw argument["three", "parameters", ""]
.The fix is to add checks for varargs methods and, when encountered, check to see if the required parameter type is the raw varargs type or the its component type.
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