Cannot verify varargs parameter as an array #1222
Comments
|
You upgraded from 1.9.5. Are you upgrading to Mockito 2 straight away or
1.10 first?
…On Thu, 26 Oct 2017, 12:05 paulduffin, ***@***.***> wrote:
Cannot verify the varargs by comparing with an array.
@test
public void shouldVerifyVarargsAsArray() throws Exception {
IMethods mock = mock(IMethods.class);
mock.mixedVarargs("1", "2", "3");
verify(mock).mixedVarargs(any(), eq(new String[] {"2", "3"}));
}
*Expected:* test runs successfully
*Actual:* the test fails with an argument mismatch error.
org.mockitousage.verification.BasicVerificationTest > shouldVerifyVarargsAsArray FAILED
Argument(s) are different! Wanted:
iMethods.mixedVarargs(<any>, ["2", "3"]);
-> at org.mockitousage.verification.BasicVerificationTest.shouldVerifyVarargsAsArray(BasicVerificationTest.java:125)
Actual invocation has different arguments:
iMethods.mixedVarargs("1", "2", "3");
-> at org.mockitousage.verification.BasicVerificationTest.shouldVerifyVarargsAsArray(BasicVerificationTest.java:123)
...
This was found while upgrading from 1.9.5. In that version it was possible
to achieve something similar by creating a custom ArgumentMatcher that
implemented VarargMatcher. In that case the ArgumentMatcher was given the
varargs as an array. However, in the latest version the same matcher is
passed each vararg separately which prevents it from matching.
It is not possible to inline the array as follows because in the actual
code the contents of the array are passed as an array into the method that
does the verification.
verify(mock).mixedVarargs(any(), eq("1"), eq("2"));
I tried doing something like this:
verify(mock).mixedVarargs(any(), inline(new String[] {"1", "2"}));
}
public static <T> T inline(T[] array) {
for (Object o : array) {
eq(o);
}
return null;
}
But that failed in MatchersBinder because the number of matchers provided
(3) did not match the number of arguments passed in the invocation (2).
I also tried to remove the VarargMatcher from the custom ArgumentMatcher
but that failed too as the number of arguments did not match in
MatcherApplicationStrategy.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1222>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFrDb7ES8J-kMmrB6K7FyOPgdDZNZTVpks5swFlVgaJpZM4QHUK9>
.
|
|
I'm actually going from 1.9.5 to 1.10.19 but 1.10.19 removed the special support for varargs in ArgumentCaptor (captureVargarg() and getVarargsValues()) which broke some tests. I fixed the tests to use capture() and getAllValues() respectively but they did not work because vararg capturing was broken. I then patched in changes from 2.2.5 (026ba1d) but had to undo some of the work done to avoid depending on hamcrest (i.e. 7f20e63, ca68963, 35786f3). I then had to patch MatcherApplicationStrategy.isLastMatcherVarargMatcher to handle That left me with this issue (which is hopefully the last). |
|
The only way that I can see that this can be easily and reliably achieved is to have a special method for matching against the varargs as an array. e.g. Something like Attempting to automatically infer when it should be treated as a vararg and when it should not is almost certainly going to have some edge cases when it cannot be detected properly. e.g. if we use the number of arguments (raw and expanded) against the number of matchers then we cannot differentiate between them when a single vararg matcher is provided, e.g. in the following we cannot determine whether the eq(...) matcher should be provided the single vararg directly, or wrapped in an array. If the matchers provided the type of object that they were going to accept then it would be possible to tell whether to pass the single vararg directly or wrapped in an array in order to make it pass but that would result in false positives when the matcher is expecting an array of a single null value but the method is called with a null array. e.g. Adding varargsAsArray would also address #584 (although as I have commented there is a work around using getAllValues()). e.g. |
|
Sadly I was not involved in the project back then. Hopefully @mockitoguy can shine some light on this issue and has an idea to tackle it. |
|
Duplicates: #584 Solution:
|
|
Why do you say it cannot be solved in mockito 2 due top compatibility? I have a solution that appears to work and will upload soon. |
Adds a new method varargsAsArray(...) that indicates that the varargs should be matched/captured as a single array rather than separate values.
Adds a new method varargsAsArray(...) that indicates that the varargs should be matched/captured as a single array rather than separate values.
This was related to the the proposal I described above. I see we had pretty similar ideas how to solve it.
Yeah I looked into your PR, it looks great. Your solution is more generic/better than the idea I had in mind 👍. |
|
This is still an issue. |
|
It looks like https://github.com/mockito/mockito/pull/1461/files still fixes it, and can just be included as needed in test code. But it would be a nice to have. |
fixes: mockito#1222
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
|
Thank you! |
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 #2807 merged, this works as expected:L @Test
public void shouldCaptureVarArgsAsArray() throws Exception {
ArgumentCaptor<String[]> varargCaptor = ArgumentCaptor.forClass(String[].class);
mock.varargs("1","2");
verify(mock).varargs(varargsAsArray(varargCaptor.capture()));
assertThat(varargCaptor).containsExactly(new String[]{"1","2"});
}... passes... |
Cannot verify the varargs by comparing with an array.
Expected: test runs successfully
Actual: the test fails with an argument mismatch error.
This was found while upgrading from 1.9.5. In that version it was possible to achieve something similar by creating a custom ArgumentMatcher that implemented VarargMatcher. In that case the ArgumentMatcher was given the varargs as an array. However, in the latest version the same matcher is passed each vararg separately which prevents it from matching.
It is not possible to inline the array as follows because in the actual code the contents of the array are passed as an array into the method that does the verification.
verify(mock).mixedVarargs(any(), eq("1"), eq("2"));I tried doing something like this:
But that failed in MatchersBinder because the number of matchers provided (3) did not match the number of arguments passed in the invocation (2).
I also tried to remove the VarargMatcher from the custom ArgumentMatcher but that failed too as the number of arguments did not match in MatcherApplicationStrategy.
The text was updated successfully, but these errors were encountered: