Remove `VarargMatcher` #2835

big-andy-coates · 2022-12-23T06:14:58Z

fixes: #2836
fixes: #1593
fixes: #585

Remove broken VarargMatcher internal interface.

`any()` behaviour change

BREAKING CHANGE: The behaviour of any() when passed to a varargs parameter has changed. Prior to Mockito v5, the any() matcher would match each element in the varargs parameter, matching any number of elements 0...n. From Mockito v5 onwards any(), when passed to a varargs parameter, will match invocations where a single value is passed to the varargs parameter. To match any number of values passed to the varargs parameter, pass the type of the varargs parameter to any(). For example, given a String... varargs parameter, use any(String[].class).

Example:

String varargsMethod(String... arg);

when(mock.varargsMethod(any()).thenReturn("matched!");

// Mockito 4.x.x: the following would match invocations with _any number_ of values passed to arg:
// i.e. all of the following invocations will return "matched!":
mock.varargsMethod();
mock.varargsMethod("1");
mock.varargsMethod("1", "2");

// Mockito 5.x.x: now a single `any()` will match a single value passed to varargs parameter:
mock.varargsMethod("1");

// To match any number of values passed to varargs parameter use `any(String[].class):
when(mock.varargsMethod(any(String[].class)).thenReturn("varargs!");

// then all of the following invocations will return "varargs!":
mock.varargsMethod();
mock.varargsMethod("1");
mock.varargsMethod("1", "2");

`ArgumentCaptor.forClass()` behaviour change

BREAKING CHANGE: The behaviour of argument captors when passed to a varargs parameter has changed. Prior to Mockito v5, passing an argument captor to a varargs parameter would capture any number of values passed to the varargs parameter. From Mockito v5 onwards, the same call will match only a single value passed to the varargs parameter. To capture any number of values, change to the type of the argument captor to match the array type of the varargs parameter. For example, given a String... varargs parameter, change the argument captor from type String to type String[].

Example:

String varargsMethod(String... arg);

@Captor private ArgumentCaptor<String> captor;

verify(mock).varargs(captor.capture());

// Mockito 4.x.x: the above captor call would match all of the following invocations:
mock.varargsMethod();    // captor.getAllValues() would return List.of();
mock.varargsMethod("1");    // captor.getAllValues() would return List.of("1");
mock.varargsMethod("1", "2");     // captor.getAllValues() would return List.of("1", "2");

// After all invocations  captor.getAllValues() would return List.of("1", "1", "2");

// Mockito 5.x.x: the avove captor call would only match:
mock.varargsMethod("1");    // captor.getAllValues() would return List.of("1");

// To match any number of values passed to the varargs parameter use `ArgumentCaptor<String[]>`:
@Captor private ArgumentCaptor<String[]> captor;

// Which will then match all of the following invocations:
mock.varargsMethod();    // captor.getValue() would return List.of();
mock.varargsMethod("1");    // captor.getValue() would return List.of("1");
mock.varargsMethod("1", "2");     // captor.getValue() would return List.of("1", "2");

// After all invocations  captor.getAllValues() would return List.of({}, {"1"}, {"1", "2"});

`argThat()` behaviour change

BREAKING CHANGE: The behaviour of all methods in MockitoHamcrest, such as argThat(), when passed to a varargs parameter has changed. Prior to Mockito v5, these matchers would match each element in the varargs parameter, matching any number of elements 0...n. From Mockito v5 onwards these matchers, when passed to a varargs parameter, will match invocations where a single value is passed to the varargs parameter. To match any number of values passed to the varargs parameter, pass use suitable Hamcrest Matcher and the varargs array type to argThat. For example, given a String... varargs parameter, use: argThat(isA(String[].class), String[].class).

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#1593 Remove broken `VarargMatcher` internal interface. BREAKING CHANGE: This changes the default behaviour of the `any()` matcher when passed to a varargs parameter. Previously, the `any()` matcher would match each element in the varargs parameter, matching any number of elements 0...n. From Mockito v5 onwards `any()`, when passed to a varargs parameter, will match invocations where a single value is passed to the varargs parameter. To match any number of values passed to the varargs parameter, pass the type of the varargs parameter to `any()`. For example, given a `String...` varargs parameter, use `any(String[].class)`.

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

codecov-commenter · 2022-12-23T06:22:45Z

Codecov Report

Base: 84.90% // Head: 84.88% // Decreases project coverage by -0.02% ⚠️

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

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2835      +/-   ##
============================================
- Coverage     84.90%   84.88%   -0.03%     
+ Complexity     2877     2875       -2     
============================================
  Files           327      327              
  Lines          8773     8761      -12     
  Branches       1063     1060       -3     
============================================
- Hits           7449     7437      -12     
  Misses         1047     1047              
  Partials        277      277

Impacted Files	Coverage Δ
src/main/java/org/mockito/ArgumentMatcher.java	`100.00% <ø> (ø)`
...rg/mockito/internal/matchers/CapturingMatcher.java	`100.00% <ø> (ø)`
...java/org/mockito/internal/matchers/InstanceOf.java	`100.00% <ø> (ø)`
src/main/java/org/mockito/ArgumentMatchers.java	`99.01% <100.00%> (ø)`
...ain/java/org/mockito/hamcrest/MockitoHamcrest.java	`92.30% <100.00%> (+1.39%)`	⬆️
...ito/internal/hamcrest/HamcrestArgumentMatcher.java	`100.00% <100.00%> (ø)`
...nternal/invocation/MatcherApplicationStrategy.java	`100.00% <100.00%> (ø)`
...c/main/java/org/mockito/internal/matchers/Any.java	`100.00% <100.00%> (ø)`

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 · 2022-12-23T06:28:20Z

@TimvdLippe I'm working through current implementations of VarargsMatcher, with a commit for each. First commit is for any() which is probably the most contentious, as it's a breaking change and widely used. However, it's only a breaking change when passed to a varargs parameter.

FYI: I can't get a green build locally on main branch. Seeing loads of errors about like:

java.lang.IllegalStateException: Could not initialize plugin: interface org.mockito.plugins.MockMaker (alternate: null)
...
Could not initialize inline Byte Buddy mock maker.

It appears as if your JDK does not supply a working agent attachment mechanism.
Java               : 15
JVM vendor name    : AdoptOpenJDK
JVM vendor version : 15.0.2+7
JVM name           : OpenJDK 64-Bit Server VM
JVM version        : 15.0.2+7
JVM info           : mixed mode, sharing
OS name            : Mac OS X
OS version         : 10.16
...

What JDK do you recommend using with Mockito?

...tests duplicate others in `VarargsTest`

BREAKING CHANGE: The behaviour of argument captors when passed to a varargs parameter has changed. Prior to Mockito v5, passing an argument captor to a varargs parameter would capture any number of values passed to the varargs parameter. From Mockito v5, the same call will match only a single value passed to the varargs parameter. To capture any number of values, change to the type of the argument captor to match the array type of the varargs parameter. For example, given a `String...` varargs parameter, change the argument captor from type `String` to type `String[]`.

src/test/java/org/mockitousage/bugs/varargs/VarargsNotPlayingWithAnyTest.java

... fix tests!

BREAKING CHANGE: The behaviour of all methods in `MockitoHamcrest` when passed to a varargs parameter has changed. Prior to Mockito v5, these matchers would match each element in the varargs parameter, matching any number of elements 0...n. From Mockito v5 onwards these matchers, when passed to a varargs parameter, will match invocations where a single value is passed to the varargs parameter. To match any number of values passed to the varargs parameter, pass use suitable Matcher that matches the vararg's array type. For example, given a `String...` varargs parameter, use a Matcher that handles `String[].class`.

... required to support matchers that match a vararg parameter, rather than values in a varargs parameter.

big-andy-coates · 2022-12-24T00:33:59Z

All done. Over to you @TimvdLippe :)

big-andy-coates · 2022-12-24T13:11:45Z

It's only a breaking change for `any()` where: * it’s used for a varargs parameter, and * no additional marchers occur after this matcher, and * it’s used to match either zero or more than two values being passed to the vararg parameter. My previous PR added support for `any(String[].class)`, and added a `isA(String[].class)` as well. This is the new preferred way of matching a varargs parameter of any length. However, without this PR it still won’t be possible to use `any()` to match exactly one value passed to the varargs parameter. Even using something like `any(String.class)` would match zero or more values. Fixing that has to be a breaking change. That said, you can not use `isA(String.class)` to match exactly one value. We could release 5,0 workout this change, but that means waiting until 6.0 before fixing this issue. Which is… a year away? IMHO, there is no point in holding off unless users know they need to migrate to the new solution. We can't mark `any()` as deprecated, as it's not. But we could add a log message when `any()` is being used in a deprecated way. Is this something Mockito has done before:? The log message could also include details of how to use `isA` for matching exactly one value. Up to you.

…

On Fri, 23 Dec 2022 at 18:22, Tim van der Lippe ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/test/java/org/mockitousage/matchers/VarargsTest.java <#2835 (comment)>: > @@ -363,20 +363,6 @@ public void shouldMockVarargsInvocation_multiple_vararg_matcher() { assertThat(mock.methodWithVarargAndNonVarargVariants("a", "b", "c")).isNull(); } - @test - public void shouldMockVarargsInvocationUsingCasts() { Hm, breaking any() sounds a bit scary. Is it possible to write code that works both before and after? E.g. is it possible to use any(String[].class) before this change as well? Ideally these are not mutually exclusive, so our users can prepare for the breaking change up front. — Reply to this email directly, view it on GitHub <#2835 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5EE3UZZG5MA3WD2VKEVYLWOWDQTANCNFSM6AAAAAATHNWRR4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

big-andy-coates · 2022-12-25T06:08:20Z

@TimvdLippe I'm working through current implementations of VarargsMatcher, with a commit for each. First commit is for any() which is probably the most contentious, as it's a breaking change and widely used. However, it's only a breaking change when passed to a varargs parameter.

FYI: I can't get a green build locally on main branch. Seeing loads of errors about like:
java.lang.IllegalStateException: Could not initialize plugin: interface org.mockito.plugins.MockMaker (alternate: null)
...
Could not initialize inline Byte Buddy mock maker.

It appears as if your JDK does not supply a working agent attachment mechanism.
Java               : 15
JVM vendor name    : AdoptOpenJDK
JVM vendor version : 15.0.2+7
JVM name           : OpenJDK 64-Bit Server VM
JVM version        : 15.0.2+7
JVM info           : mixed mode, sharing
OS name            : Mac OS X
OS version         : 10.16
...
What JDK do you recommend using with Mockito?

Thoughts @TimvdLippe ?

TimvdLippe · 2022-12-25T13:12:56Z

@big-andy-coates with regards to the breakage: it seems like users have alternatives that will work on both 4.10.0 and 5.0.0. This is essentially my only requirement: it should be possible to write that will work before and after this change. That way, users can first update their usage on 4.10.0 to get it to work, then upgrade to 5.0.0 and then potentially update their code again if they so desire. As I understand your comment, this would be possible with isA? In any case, if it wasn't possible to write these kind of matchers before and this PR enables more use cases, then that's fine by me as well.

With regards to your JDK issue: you are using JDK 15. Can you try out either 11 or 17? That should hopefully work. If that doesn't work, maybe @reta can share their local setup? They have been contributing lately as well, so I am assuming their local setup works.

big-andy-coates · 2022-12-25T13:50:21Z

@TimvdLippe #2807 is not released yet, so there is no way users can express ‘any number of varargs’ in 4.10.0, except the broken `any()` method. Using `isA(String[].class)` would not work without #2807. I’m guessing it’s not (easily) possible to release #2807 as 4.11.0…?

…

On Sun, 25 Dec 2022 at 20:13, Tim van der Lippe ***@***.***> wrote: @big-andy-coates <https://github.com/big-andy-coates> with regards to the breakage: it seems like users have alternatives that will work on both 4.10.0 and 5.0.0. This is essentially my only requirement: it should be possible to write that will work before and after this change. That way, users can first update their usage on 4.10.0 to get it to work, then upgrade to 5.0.0 and then potentially update their code again if they so desire. As I understand your comment, this would be possible with isA? In any case, if it wasn't possible to write these kind of matchers before and this PR enables more use cases, then that's fine by me as well. With regards to your JDK issue: you are using JDK 15. Can you try out either 11 or 17? That should hopefully work. If that doesn't work, maybe @reta <https://github.com/reta> can share their local setup? They have been contributing lately as well, so I am assuming their local setup works. — Reply to this email directly, view it on GitHub <#2835 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5EE3V55VZEJ47DUHHJOPDWPBB6FANCNFSM6AAAAAATHNWRR4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

reta · 2022-12-25T19:38:19Z

What JDK do you recommend using with Mockito?

@big-andy-coates the error message could be a bit misleading (at least so far the issues I've run into have not been related to JDK), happy to help here.

big-andy-coates · 2022-12-26T03:55:20Z

What JDK do you recommend using with Mockito?

@big-andy-coates the error message could be a bit misleading (at least so far the issues I've run into have not been related to JDK), happy to help here.

Hi @reta,

Yes, please. Do you need more info?

It was all running fine until recently when work started on v5...

reta · 2022-12-26T17:35:43Z

What JDK do you recommend using with Mockito?

@big-andy-coates the error message could be a bit misleading (at least so far the issues I've run into have not been related to JDK), happy to help here.

Hi @reta,

Yes, please. Do you need more info?

It was all running fine until recently when work started on v5...

Hi @big-andy-coates , could you please share the command line you use to run the tests and which module or test(s) fail with java.lang.IllegalStateException, I have just run locally builds with JDK-15/16, cannot see the problem so far, thank you.

big-andy-coates · 2022-12-27T04:49:23Z

Ah, I think I know what's going in. There's an error in the build script. PR inbound...

See #2839

big-andy-coates · 2022-12-27T05:54:32Z

@TimvdLippe #2807 is not released yet, so there is no way users can express ‘any number of varargs’ in 4.10.0, except the broken any() method. Using isA(String[].class) would not work without #2807. I’m guessing it’s not (easily) possible to release #2807 as 4.11.0…?

@TimvdLippe, incase this was lost in the chatter about build failures...

TimvdLippe · 2022-12-27T19:27:09Z

@big-andy-coates If I understand correctly, #2807 is not a breaking change, is it? If so, I can publish it as part of 4.11.0. That will require some tinkering on my end, but I think that is better for our users. Then we have a feasible alternative to the breaking change here, so that they can always have code running on both 4.11.0 and 5.0.0, without needing a big bang approach.

big-andy-coates · 2022-12-28T01:52:16Z

That is correct @TimvdLippe

TimvdLippe · 2022-12-28T12:53:17Z

I created https://github.com/mockito/mockito/tree/release/4.11.0 and cherry-picked #2807 onto there. Then I pushed tag v4.11.0 which is being published as we speak: https://github.com/mockito/mockito/actions/runs/3794091624

TimvdLippe · 2022-12-28T13:16:15Z

https://repo1.maven.org/maven2/org/mockito/mockito-core/4.11.0/ has been published successfully. I will now do a full review of this PR so that we can get it in 😄

TimvdLippe

This all looks great to me! I only have one question with regards to a test you changed. I would like to know if that is a required regression or if we can keep the pre-existing behavior.

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

...demonstrating how exactly two, or any number, of varargs values can be matched with argument captors.

...for methods released in 4.11.0

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

TimvdLippe

This all LGTM. Unfortunately I didn't realize this would clash with the other PR I just merged, so we have to fix the merge conflicts 😢

big-andy-coates · 2022-12-30T10:28:07Z

I wasn't expecting the other PR to be merged until after 5.0.0 was released, but ... no worries.

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

Release Mockito 5 #2802

Closed

4 tasks

big-andy-coates commented Dec 23, 2022

View reviewed changes

src/test/java/org/mockitousage/matchers/VarargsTest.java Show resolved Hide resolved

big-andy-coates added 3 commits December 23, 2022 13:28

Remove duplicate test file

7fe9688

...tests duplicate others in `VarargsTest`

InstanceOf

c6eb9e6

big-andy-coates commented Dec 23, 2022

View reviewed changes

src/test/java/org/mockitousage/bugs/varargs/VarargsNotPlayingWithAnyTest.java Show resolved Hide resolved

big-andy-coates added 7 commits December 23, 2022 17:49

CapturingMatcher part II

a1221dd

... fix tests!

CapturingMatcher part III

873294f

... fix tests!

Checkstyle

2a13caf

add argThat(matcher, type)

7117387

... required to support matchers that match a vararg parameter, rather than values in a varargs parameter.

Update MatcherApplicationStrategy

eaafc3e

Remove VarargMatcher

17f97a1

big-andy-coates marked this pull request as ready for review December 24, 2022 00:18

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

Improve vararg handling #2807

Merged

8 tasks

TimvdLippe reviewed Dec 28, 2022

View reviewed changes

src/test/java/org/mockitousage/matchers/CapturingArgumentsTest.java Show resolved Hide resolved

Add primitive captor tests

fbf8929

...demonstrating how exactly two, or any number, of varargs values can be matched with argument captors.

This was referenced Dec 29, 2022

ArgumentCaptor A single capture() call captures all vararg elements. #585

Closed

Mockito confuses varargs and single-param methods (regression) #1593

Closed

Correct @SInCE

7a87dd6

...for methods released in 4.11.0

big-andy-coates commented Dec 29, 2022

View reviewed changes

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

TimvdLippe approved these changes Dec 29, 2022

View reviewed changes

Merge from main

b939591

TimvdLippe merged commit 064fe90 into mockito:main Dec 30, 2022

big-andy-coates deleted the remove_vararg_matcher branch January 1, 2023 07:07

timtebeek mentioned this pull request Jan 2, 2023

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

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `VarargMatcher` #2835

Remove `VarargMatcher` #2835

big-andy-coates commented Dec 23, 2022 •

edited

codecov-commenter commented Dec 23, 2022 •

edited

big-andy-coates commented Dec 23, 2022

big-andy-coates commented Dec 24, 2022

big-andy-coates commented Dec 24, 2022 via email •

edited

big-andy-coates commented Dec 25, 2022 •

edited

TimvdLippe commented Dec 25, 2022

big-andy-coates commented Dec 25, 2022 via email •

edited

reta commented Dec 25, 2022

big-andy-coates commented Dec 26, 2022 •

edited

reta commented Dec 26, 2022

big-andy-coates commented Dec 27, 2022 •

edited

big-andy-coates commented Dec 27, 2022 •

edited

TimvdLippe commented Dec 27, 2022

big-andy-coates commented Dec 28, 2022

TimvdLippe commented Dec 28, 2022

TimvdLippe commented Dec 28, 2022

TimvdLippe left a comment

TimvdLippe left a comment

big-andy-coates commented Dec 30, 2022

Remove VarargMatcher #2835

Remove VarargMatcher #2835

Conversation

big-andy-coates commented Dec 23, 2022 • edited

any() behaviour change

ArgumentCaptor.forClass() behaviour change

argThat() behaviour change

Checklist

codecov-commenter commented Dec 23, 2022 • edited

Codecov Report

big-andy-coates commented Dec 23, 2022

big-andy-coates commented Dec 24, 2022

big-andy-coates commented Dec 24, 2022 via email • edited

big-andy-coates commented Dec 25, 2022 • edited

TimvdLippe commented Dec 25, 2022

big-andy-coates commented Dec 25, 2022 via email • edited

reta commented Dec 25, 2022

big-andy-coates commented Dec 26, 2022 • edited

reta commented Dec 26, 2022

big-andy-coates commented Dec 27, 2022 • edited

big-andy-coates commented Dec 27, 2022 • edited

TimvdLippe commented Dec 27, 2022

big-andy-coates commented Dec 28, 2022

TimvdLippe commented Dec 28, 2022

TimvdLippe commented Dec 28, 2022

TimvdLippe left a comment

Choose a reason for hiding this comment

TimvdLippe left a comment

Choose a reason for hiding this comment

big-andy-coates commented Dec 30, 2022

Remove `VarargMatcher` #2835

Remove `VarargMatcher` #2835

big-andy-coates commented Dec 23, 2022 •

edited

`any()` behaviour change

`ArgumentCaptor.forClass()` behaviour change

`argThat()` behaviour change

codecov-commenter commented Dec 23, 2022 •

edited

big-andy-coates commented Dec 24, 2022 via email •

edited

big-andy-coates commented Dec 25, 2022 •

edited

big-andy-coates commented Dec 25, 2022 via email •

edited

big-andy-coates commented Dec 26, 2022 •

edited

big-andy-coates commented Dec 27, 2022 •

edited

big-andy-coates commented Dec 27, 2022 •

edited