Improve vararg handling #2807

big-andy-coates · 2022-11-28T19:57:56Z

Fixes: #2796
Fixes: #567
Fixes: #584
Fixes: #1222
Fixes: #1498

An alternative approach to #2805.

Key difference:

This approach adds the new type() method to the main ArgumentMatcher interface and leaves the, IMHO broken, VarargsMatcher interface alone.

Benefit:

Because this approach leaves VarargsMatcher untouched, it does not require additional existing matchers to implement VarargsMatcher to fix issues such as #567. Where as the first PR would require Null and NotNull to be marked VarargsMatcher.

This PR creates new variants of isNotNull and isNull to address #567.
Having InstanceOf override type() provides a workable solution to #1593.
Having equals override type addresses #1222.

If we want to go with this approach I'll implement type() for all the other existing matchers.

Downside

The obvious downside is that this changes the public ArgumentMatcher interface, though in a backwards compatible way.

The default implementation could be dropped in the next major version bump.

Known limitation

The main limitation I'm aware of, is not a new limitation. It is that it is not possible to assert only a single parameter is passed to the vararg parameter, when using a VarargMatcher, e.g. any(). (ref: #1593). For example:

// Given method:
int vararg(String... args);

// I want to mock this invocation:
mock.vararag("one param");

// ...but not these:
mock.vararg();
mock.vararg("more than", "one param");

There is no current way to do this. This is because in the following intuitive mocking:

given(mock.vararg(any(String.class))).willReturn(1);

... matches zero or more vararg parameters, as the any() method is using VarargMatcher. It seems to me that VarargMatcher is... a little broken! This is maybe something that should be consider a candiate for fixing in the next major version bump.

While it is not possible to fix any VarargMatcher based matchers in a backwards compatible way, this the approach in this PR it is possible to mock/verify exactly one vararg param using isA, rather than any:

    @Test
    public void shouldMatchExactlyOnParam() {
        mock.varargs("one param");

        verify(mock).varargs(isA(String.class));
    }

    @Test
    public void shouldNotMatchMoreParams() {
        mock.varargs("two", "params");

        verify(mock, never()).varargs(isA(String.class));
    }

    @Test
    public void shouldMatchAnyNumberOfParams() {
        mock.varargs("two", "params");

        verify(mock).varargs(isA(String[].class));
    }

... because isA does not implement VarargsMatcher, and so can work as expected once it implements 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

Fixes: mockito#2796 Add an optional method to `VarargMatcher`, which implementations can choose to override to return the type of object the matcher is matching. This is used by `MatcherApplicationStrategy` to determine if the type of matcher used to match a vararg parameter is of a type compatible with the vararg parameter. Where a vararg compatible matcher is found, the matcher is used to match the _raw_ parameters.

...add the `type()` method to the `ArgumentMatcher` interface, rather than the `VarargMatcher` interface. This would allow the, IMHO _broken_, `VarargMatcher` interface to be removed in a later major release.

big-andy-coates · 2022-11-28T19:59:21Z

@TimvdLippe Personally, I prefer this approach.

codecov-commenter · 2022-11-28T20:39:54Z

Codecov Report

Base: 86.20% // Head: 84.80% // Decreases project coverage by -1.39% ⚠️

Coverage data is based on head (63193a8) compared to base (e2f26d8).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2807      +/-   ##
============================================
- Coverage     86.20%   84.80%   -1.40%     
- Complexity     2833     2866      +33     
============================================
  Files           320      327       +7     
  Lines          8587     8744     +157     
  Branches       1060     1057       -3     
============================================
+ Hits           7402     7415      +13     
- Misses          904     1052     +148     
+ Partials        281      277       -4

Impacted Files	Coverage Δ
...ito/internal/hamcrest/HamcrestArgumentMatcher.java	`100.00% <ø> (ø)`
src/main/java/org/mockito/ArgumentCaptor.java	`100.00% <100.00%> (ø)`
src/main/java/org/mockito/ArgumentMatcher.java	`100.00% <100.00%> (ø)`
src/main/java/org/mockito/ArgumentMatchers.java	`99.01% <100.00%> (+0.05%)`	⬆️
...nternal/invocation/MatcherApplicationStrategy.java	`100.00% <100.00%> (ø)`
...c/main/java/org/mockito/internal/matchers/And.java	`88.88% <100.00%> (+5.55%)`	⬆️
...c/main/java/org/mockito/internal/matchers/Any.java	`100.00% <100.00%> (ø)`
...rg/mockito/internal/matchers/CapturingMatcher.java	`100.00% <100.00%> (ø)`
...ain/java/org/mockito/internal/matchers/Equals.java	`87.50% <100.00%> (+0.83%)`	⬆️
...java/org/mockito/internal/matchers/InstanceOf.java	`100.00% <100.00%> (ø)`
... and 40 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

big-andy-coates

Added some review notes...

src/main/java/org/mockito/ArgumentMatcher.java

src/main/java/org/mockito/internal/invocation/MatcherApplicationStrategy.java

src/main/java/org/mockito/internal/matchers/InstanceOf.java

src/main/java/org/mockito/internal/matchers/NotNull.java

src/main/java/org/mockito/internal/matchers/Null.java

fixes: mockito#1222

big-andy-coates · 2022-11-30T22:30:27Z

OK @TimvdLippe , I think that's all the implementations of ArgumentMatcher it makes sense to add the type() method to.

This should be just about complete now.

Look forward to a review when you have time.

src/main/java/org/mockito/ArgumentMatcher.java

TimvdLippe · 2022-12-19T19:53:47Z

I haven't done a thorough review yet, as I mostly want to discuss the change to ArgumentMatcher first. Ideally, I want to keep that interface as it is, so that the experience for our users remains the same and they can keep on using lambdas. We can then make our internal matchers smarter for varargs, but avoid too many breakages. Does that sound feasible to you or would you like me to look at other parts of the PR first?

TimvdLippe

I think we are almost there 😄

src/main/java/org/mockito/ArgumentMatcher.java

src/main/java/org/mockito/internal/matchers/VarargMatcher.java

src/test/java/org/mockito/internal/matchers/AndTest.java

src/test/java/org/mockitousage/matchers/VarargsTest.java

big-andy-coates · 2022-12-22T02:56:38Z

@TimvdLippe I've updated the since to reflect the new target release of 5.0.0 and I've marked ArgumentMatcher interface as @FunctionalInterface.

If possible, I'd prefer we merge this, and I look to remove the VarargMatcher interface as a separate PR. Is that OK, or would you rather I remove VarargMatcher in this PR?

TimvdLippe · 2022-12-22T16:00:31Z

@big-andy-coates Yes that's okay to me. It might still be a good idea to include the removal of the interface into Mockito 5.0.0 no? Since it removes an interface, even if it is internal, I think it is cleanest to include in a major release.

TimvdLippe

Woohoo, great work!

big-andy-coates · 2022-12-23T02:58:35Z

@big-andy-coates Yes that's okay to me. It might still be a good idea to include the removal of the interface into Mockito 5.0.0 no? Since it removes an interface, even if it is internal, I think it is cleanest to include in a major release.

Yes, I'll get this done asap to include in the 5.0.0 release.

big-andy-coates · 2022-12-24T00:34:43Z

See #2835 for pr removing VarargMatcher

@test

Using the new `type()`, we can differentiate between matching all varargs or only one argument of the varargs. # Benefits: Because this approach leaves `VarargsMatcher` untouched, it does not require additional existing matchers to implement `VarargsMatcher` to fix issues such as #567. Where as the first PR would require `Null` and `NotNull` to be marked `VarargsMatcher`. This PR creates new variants of `isNotNull` and `isNull` to address #567. Having `InstanceOf` override `type()` provides a workable solution to #1593. Having `equals` override `type` addresses #1222. # Downsides The obvious downside is that this changes the public `ArgumentMatcher` interface, though in a backwards compatible way. ## Known limitation The main limitation I'm aware of, is not a new limitation. It is that it is not possible to assert only a single parameter is passed to the vararg parameter, when using a `VarargMatcher`, e.g. `any()`. (ref: #1593). For example: ```java // Given method: int vararg(String... args); // I want to mock this invocation: mock.vararag("one param"); // ...but not these: mock.vararg(); mock.vararg("more than", "one param"); ``` There is no current way to do this. This is because in the following intuitive mocking: ```java given(mock.vararg(any(String.class))).willReturn(1); ``` ... matches zero or more vararg parameters, as the `any()` method is using `VarargMatcher`. It seems to me that `VarargMatcher` is... a little broken! This is maybe something that should be consider a candiate for fixing in the next major version bump. While it is not possible to fix any `VarargMatcher` based matchers in a backwards compatible way, this the approach in this PR it is possible to mock/verify exactly one vararg param using `isA`, rather than `any`: ```java @test public void shouldMatchExactlyOnParam() { mock.varargs("one param"); verify(mock).varargs(isA(String.class)); } @test public void shouldNotMatchMoreParams() { mock.varargs("two", "params"); verify(mock, never()).varargs(isA(String.class)); } @test public void shouldMatchAnyNumberOfParams() { mock.varargs("two", "params"); verify(mock).varargs(isA(String[].class)); } ``` ... because `isA` does not implement `VarargsMatcher`, and so can work as expected once it implements `type()`. Fixes #2796 Fixes #567 Fixes #584 Fixes #1222 Fixes #1498

cpovirk · 2022-12-29T15:59:49Z

This PR does end up introducing an incompatible change to CapturingMatcher: It changes the constructor from CapturingMatcher() to CapturingMatcher(Class). We don't appear to have very many CapturingMatcher subclasses inside Google, so it looks like we'll be able to update the ones that we do have atomically when we pull in this update.

I gather that this change might not land until the 5.0.0 major release, in which case that's obviously the time to make incompatible changes :) You may want to note the change in a @since tag (and perhaps release notes, as you might already be planning to do). You could go even further by introducing the new constructor in a release before 5.0.0 so that people at least don't have to update their code atomically. But unless an organization is a much heavier user of CapturingMatcher than we are, that's not necessarily worth the trouble.

cpovirk · 2022-12-29T16:04:16Z

Oh, sorry, scratch all that: CapturingMatcher is under internal, so we are Doing It Wrong :) (I got it mixed up with the changes to the classes in org.mockito itself.)

TimvdLippe · 2022-12-29T17:04:47Z

Ah yeah we don't consider org.mockito.internal as part of SemVer, so ideally users don't use these internal matchers. Can you port these implementations to use only public API? If not, please file a GitHub issue so we can consider the use case.

cpovirk · 2022-12-29T17:20:07Z

Thanks. I have to imagine they're possible to port. They might actually mostly be implementations of isA from before that existed, and I imagine a plain capture will suffice for the rest.

That's not to say that we'll actually do it anytime soon. (I'm sure you of all people are shocked... :)) But I should at least more readily remember how much this is our fault next time.

TimvdLippe · 2022-12-29T17:21:35Z

Haha yeah all good. I am sure you can find some of internal tickets with more explanation on how to tackle that problem. Feel free to message me if you need more information from my side.

cpovirk · 2022-12-29T17:52:46Z

Thanks, I had already come across one :)

I will also report that people really like writing ArgumentCaptor.forClass(null), which now throws NPE. But that looks manageable, too. (example)

…table*` methods and to `ArgumentCaptor.forClass`. - The `Immutable*` methods of course already throw `NullPointerException`. And while they're already annotated for nullness, they've been ignored by this checker because there are [technical challenges](https://bugs.openjdk.org/browse/JDK-8225377) with identifying the nullness of type-variable types loaded from `.class` files. - `ArgumentCaptor.forClass` soon [will throw `NullPointerException`](mockito/mockito@fe1cb2d#diff-8d274a9bda2d871524d15bbfcd6272bd893a47e6b1a0b460d82a8845615f26daR31). See mockito/mockito#2807 (comment). To match `ArgumentCaptor.forClass`, we have to start checking test-only code. But for now, we check such code _only_ for calls to `ArgumentCaptor.forClass`. (I suppose that we still don't handle the _varargs_ parameters of the various `Immutable*` methods, since this entire check skips varargs, in part because varargs are confusing in general (b/232103314) and in part because we can't see annotations on a varargs-element type that is loaded from a `.class` file. We could make an exception for these methods if we really wanted, but I doubt it's worth it.) PiperOrigin-RevId: 499237604

…table*` methods and to `ArgumentCaptor.forClass`. - The `Immutable*` methods of course already throw `NullPointerException`. And while they're already annotated for nullness, they've been ignored by this checker because there are [technical challenges](https://bugs.openjdk.org/browse/JDK-8225377) with identifying the nullness of type-variable types loaded from `.class` files. - `ArgumentCaptor.forClass` soon [will throw `NullPointerException`](mockito/mockito@fe1cb2d#diff-8d274a9bda2d871524d15bbfcd6272bd893a47e6b1a0b460d82a8845615f26daR31). See mockito/mockito#2807 (comment). To match `ArgumentCaptor.forClass`, we have to start checking test-only code. But for now, we check such code _only_ for calls to `ArgumentCaptor.forClass`. (I suppose that we still don't handle the _varargs_ parameters of the various `Immutable*` methods, since this entire check skips varargs, in part because varargs are confusing in general (b/232103314) and in part because we can't see annotations on a varargs-element type that is loaded from a `.class` file. We could make an exception for these methods if we really wanted, but I doubt it's worth it.) PiperOrigin-RevId: 499326542

dacianf · 2023-03-10T11:26:00Z

Hey @big-andy-coates @TimvdLippe,

I'm trying Mockito 5.2 in Android using Kotlin and I got over a problem with varg.

Let's say we have the following interface for getting strings:

interface TestVargsMock {
    fun getString(res: Int, vararg args: String): String
}

When I try to mock it using:

whenever(testVargsMock.getString(anyInt(), any()))
            .thenAnswer {
            "It works"
        }

works just fine, as expected; it checks if only one argument was used

but if I try whenever(testVargsMock.getString(anyInt(), any(Array<out String>::class.java))) it doesn't compile because of a type mismatch error: Type mismatch: inferred type is Array<out String>! but String was expected

if I try to mock any of string whenever(testVargsMock.getString(anyInt(), any(String::class.java))) mocking is failing with java.lang.NullPointerException: any(String::class.java) must not be null

I don't know how should I express String[].class in kotlin so the mocking will work. Is there a way to do it or this is a bug that needs to be fixed?

TimvdLippe · 2023-03-10T18:20:40Z

@dacianf For Kotlin related questions, please ask it over at mockito-kotlin: mockito/mockito-kotlin#474 Any help appreciated there to fix the Kotlin support!

mockito/mockito#2807

big-andy-coates added 4 commits November 28, 2022 17:51

Formatting

054665b

Checkstyle

b2b8122

Alternative way of skinning the same cat.

0e111ae

...add the `type()` method to the `ArgumentMatcher` interface, rather than the `VarargMatcher` interface. This would allow the, IMHO _broken_, `VarargMatcher` interface to be removed in a later major release.

big-andy-coates mentioned this pull request Nov 28, 2022

Improve vararg handling #2805

Closed

8 tasks

Address mockito#1593

0ddedaa

big-andy-coates commented Nov 28, 2022

View reviewed changes

big-andy-coates added a commit to big-andy-coates/mockito that referenced this pull request Nov 28, 2022

Add same test cases as already added to mockito#2807

87dbe8c

big-andy-coates added 7 commits November 30, 2022 15:16

Mark @SInCE

b7f5b5a

Enhance Equals

0b8e97c

fixes: mockito#1222

Harden testing

519a4c1

Enhance Not

de111f7

Enhance Same

8e1658b

Enhance Or

c636b40

Enhance And

73e47ac

big-andy-coates added 3 commits November 30, 2022 23:36

Code coverage

93e84de

Improve JavaDocs

57634ff

Merge branch 'main' into vararg3

2400d55

big-andy-coates mentioned this pull request Dec 19, 2022

Release Mockito 5 #2802

Closed

4 tasks

TimvdLippe reviewed Dec 19, 2022

View reviewed changes

src/main/java/org/mockito/ArgumentMatcher.java Show resolved Hide resolved

TimvdLippe reviewed Dec 21, 2022

View reviewed changes

Mark functional & update since

63193a8

TimvdLippe approved these changes Dec 22, 2022

View reviewed changes

TimvdLippe merged commit fe1cb2d into mockito:main Dec 22, 2022

big-andy-coates deleted the vararg3 branch December 23, 2022 02:58

big-andy-coates mentioned this pull request Dec 25, 2022

Remove VarargMatcher #2835

Merged

8 tasks

big-andy-coates changed the title ~~Improve vararg handling: approach 2~~ Improve vararg handling Dec 29, 2022

timtebeek mentioned this pull request Jan 2, 2023

Handle breaking changes in Mockito 5 openrewrite/rewrite-testing-frameworks#295

Open

5 tasks

copybara-service bot mentioned this pull request Jan 3, 2023

Extend NullArgumentForNonNullParameter to match calls to some Immutable* methods and to ArgumentCaptor.forClass. google/error-prone#3680

Closed

lukas-krecan added a commit to lukas-krecan/mockito-kotlin that referenced this pull request May 19, 2023

Fix vararg handling

bf2d54e

mockito/mockito#2807

lukas-krecan added a commit to lukas-krecan/mockito-kotlin that referenced this pull request May 19, 2023

Fix vararg handling

760b61f

mockito/mockito#2807

lukas-krecan added a commit to lukas-krecan/mockito-kotlin that referenced this pull request May 19, 2023

Fix vararg handling

0362c19

mockito/mockito#2807

lukas-krecan added a commit to lukas-krecan/mockito-kotlin that referenced this pull request May 19, 2023

Fix vararg handling

08830c7

mockito/mockito#2807

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve vararg handling #2807

Improve vararg handling #2807

big-andy-coates commented Nov 28, 2022 •

edited

Loading

big-andy-coates commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Nov 28, 2022 •

edited

Loading

big-andy-coates left a comment

big-andy-coates commented Nov 30, 2022 •

edited

Loading

TimvdLippe commented Dec 19, 2022

TimvdLippe left a comment

big-andy-coates commented Dec 22, 2022 •

edited

Loading

TimvdLippe commented Dec 22, 2022

TimvdLippe left a comment

big-andy-coates commented Dec 23, 2022

big-andy-coates commented Dec 24, 2022

cpovirk commented Dec 29, 2022

cpovirk commented Dec 29, 2022

TimvdLippe commented Dec 29, 2022

cpovirk commented Dec 29, 2022

TimvdLippe commented Dec 29, 2022

cpovirk commented Dec 29, 2022 •

edited

Loading

dacianf commented Mar 10, 2023

TimvdLippe commented Mar 10, 2023

Improve vararg handling #2807

Improve vararg handling #2807

Conversation

big-andy-coates commented Nov 28, 2022 • edited Loading

Key difference:

Benefit:

Downside

Known limitation

Checklist

big-andy-coates commented Nov 28, 2022 • edited Loading

codecov-commenter commented Nov 28, 2022 • edited Loading

Codecov Report

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates commented Nov 30, 2022 • edited Loading

TimvdLippe commented Dec 19, 2022

TimvdLippe left a comment

Choose a reason for hiding this comment

big-andy-coates commented Dec 22, 2022 • edited Loading

TimvdLippe commented Dec 22, 2022

TimvdLippe left a comment

Choose a reason for hiding this comment

big-andy-coates commented Dec 23, 2022

big-andy-coates commented Dec 24, 2022

cpovirk commented Dec 29, 2022

cpovirk commented Dec 29, 2022

TimvdLippe commented Dec 29, 2022

cpovirk commented Dec 29, 2022

TimvdLippe commented Dec 29, 2022

cpovirk commented Dec 29, 2022 • edited Loading

dacianf commented Mar 10, 2023

TimvdLippe commented Mar 10, 2023

big-andy-coates commented Nov 28, 2022 •

edited

Loading

big-andy-coates commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Nov 28, 2022 •

edited

Loading

big-andy-coates commented Nov 30, 2022 •

edited

Loading

big-andy-coates commented Dec 22, 2022 •

edited

Loading

cpovirk commented Dec 29, 2022 •

edited

Loading