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

Improve Varargs handling in AdditionalAnswers #2664

Merged
merged 3 commits into from Jun 11, 2022

Conversation

big-andy-coates
Copy link
Contributor

@big-andy-coates big-andy-coates commented Jun 2, 2022

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,

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", ""].

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

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

@big-andy-coates big-andy-coates marked this pull request as draft June 2, 2022 00:20
@big-andy-coates
Copy link
Contributor Author

@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?

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2022

Codecov Report

Merging #2664 (7ed9b26) into main (1fbef57) will increase coverage by 0.03%.
The diff coverage is 90.16%.

@@             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     
Impacted Files Coverage Δ
...l/stubbing/answers/AnswerFunctionalInterfaces.java 94.79% <88.09%> (-5.21%) ⬇️
...o/internal/stubbing/answers/ReturnsArgumentAt.java 98.21% <94.44%> (-1.79%) ⬇️
...l/stubbing/defaultanswers/ForwardsInvocations.java 100.00% <100.00%> (ø)
...rg/mockito/internal/creation/MockSettingsImpl.java 88.11% <0.00%> (ø)
src/main/java/org/mockito/Mock.java 100.00% <0.00%> (ø)
...nternal/configuration/MockAnnotationProcessor.java 97.14% <0.00%> (+0.08%) ⬆️
.../mockito/internal/stubbing/StrictnessSelector.java 100.00% <0.00%> (+60.00%) ⬆️

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 1fbef57...7ed9b26. Read the comment docs.

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", ""]`.
avoiding having to cast to `Invocation`
@big-andy-coates big-andy-coates marked this pull request as ready for review June 6, 2022 17:06
@big-andy-coates
Copy link
Contributor Author

@TimvdLippe any chance of a review please?

@TimvdLippe
Copy link
Contributor

I have a very busy week this week. I will likely get to this early next week.

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.

Solid work and great test coverage! Only two small nits, but other than this it is ready to go 🎉

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.

Thanks!

@TimvdLippe TimvdLippe merged commit ce4e64d into mockito:main Jun 11, 2022
@big-andy-coates
Copy link
Contributor Author

No worries. Good to give something back. I've used Mockito for YEARS!

@perlun
Copy link
Contributor

perlun commented Nov 18, 2022

@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. javac resolves those two calls to the correct/same logger.info() overload, so so far so good.

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"... 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Varargs methods cause ClassCastException in AnswerFunctionalInterfaces
4 participants