-
-
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
Mockito confuses varargs and single-param methods (regression) #1593
Comments
I can fix it. |
try this as workaround: |
the problem is matches method from internal class VarArgAware in InstanceOf. It need to be override. |
Hi @jjDevPL Thanks for tracking the version that changed the behavior ! This was done as part of #635. For other here's the code in the archive : interface Good {
Long call(String... values);
}
interface Bad {
Long call(String value);
Long call(String... values);
}
private Good good = Mockito.mock(Good.class);
private Bad bad = Mockito.mock(Bad.class);
private String value = "abc";
// Expectation with `String[]` and call vararg method with a single-element outer-array.
// This passes in Mockito 2.2.4 and earlier
// This fails in Mockito 2.2.5 and later
@Test
public void testMockitoVarArgs1() {
Mockito.when(good.call(Mockito.any(String[].class))).thenReturn(1L);
Long count = good.call(new String[]{value});
assertEquals(1, (long) count);
}
// The failure above can be fixed by changing the expectation to `String` (with the exception of the case below)
// This passes all Mockito versions
@Test
public void testMockitoVarArgs2() {
Mockito.when(good.call(Mockito.any(String.class))).thenReturn(1L);
Long count = good.call(new String[]{value});
assertEquals(1, (long) count);
}
// However, with `bad` having the single-arg method, the fix doesn't work, and there is no way I can find to make
// this pass.
// This fails in all Mockito versions
@Test
public void testMockitoVarArgs3() {
Mockito.when(bad.call(Mockito.any(String.class))).thenReturn(1L);
Long count = bad.call(new String[]{value});
assertEquals(1, (long) count);
} Both
@jjDevPL This doesn't happen if the API of the type to mock is designed this way (which is better) :
I don't know if you own this API, but this could solves you issue faster than us. |
well, the issue got bug label. There are work around but nevertheless it should be fixed. My changes are located in InstanceOf.java
and under VarArgAware
small changes but it fixes Bad example. #635 was meged long time ago and I think Bad example is still bad. |
I am trying to mock querydsl orderBy method:
the project is on java 1.8 yet, and the workaround using deprecated Mockito api worked, but java version change will break it. |
This PR contains changes NOT intended to be committed 'as-is', but as a showcase for a potential solution to: * mockito#2796 * mockito#1593 And potentially other vararg related issues. The crux of the issue is that Mockito needs to handle the last matcher passed to a method, when that matcher aligns with a vararg parameter and has the same type as the vararg parameter. For example, ```java public interface Foo { String m1(String... args); // Vararg param at index 0 and type String[] } @test public void shouldWork2() throws Exception { // Last matcher at index 0, and with type String[]: needs special handling! given(foo.m1(any(String[].class))).willReturn("var arg method"); ... } ``` In such situations that code needs to match the raw argument, _not_ the current functionality, which is to use the last matcher to match the last _non raw_ argument. Unfortunately, I'm not aware of a way to get at the type of the matcher without adding a method to `VarargMatcher` to get this information. This is the downside of this approach.
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
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)`.
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
With #2835: when(good.call(any(String[].class))).thenReturn(1L); ...now works, matching any number of values passed to the varargs parameter. Where as: when(good.call(any(String.class))).thenReturn(1L); ... will match exactly one value passed to the varargs parameter. |
BREAKING CHANGE: This changes the default behaviour of the `any()` matcher, argument captors and `MockitoHamcrest` matchers when passed to a varargs parameter. Previously, these matcher would match each element in the varargs parameter, matching any number of elements 0...n. From Mockito v5 onwards, 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 the matcher. For example, given a `String...` varargs parameter, use `any(String[].class)`. Fixes #2836 Fixes #1593 Fixes #585
In Mockito 2.2.5 and later, when setting expectations for mocking a varargs method, using an array doesn't work:
And instead the contained type must be used:
However in the face of an overload of the varargs method that takes a single argument (of the same type):
This results in being unable to set an expectation for the varargs call.
This worked previously in Mockito 2.2.4 and earlier.
Reproducer: mockito-bug.tar.gz
The text was updated successfully, but these errors were encountered: