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.

Sign up for GitHub

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

Jump to bottom

ArgumentCaptor can't capture varargs-arrays #584

Closed
ChristianSchwarz opened this issue Aug 23, 2016 · 4 comments · Fixed by #2807
Closed

ArgumentCaptor can't capture varargs-arrays #584

ChristianSchwarz opened this issue Aug 23, 2016 · 4 comments · Fixed by #2807
Labels
ready
Milestone
2.x

Comments

@ChristianSchwarz
Copy link
Contributor

ChristianSchwarz commented Aug 23, 2016 •
edited
Loading

Relates to: #439, #565, #567, #583

ArgumentCaptor can't capture varargs-arrays.

@Test
public void shouldCaptureVarArgsAsArray() throws Exception {
    ArgumentCaptor<String[]> varargCaptor = ArgumentCaptor.forClass(String[].class);

    mock.varargs("1","2");   

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

    assertThat(varargCaptor).containsExactly(new String[]{"1","2"});
}

Expected: A ArgumentCaptor<String[]> should be able to capture the varargs-array as a whole.
Actual: The ArgumentCaptor captures no arrays but every single argument passed to the varargs method. The following exception is thrown:

java.lang.AssertionError: 
Actual and expected should have same size but actual size was:
  <2>
while expected size was:
  <1>
Actual was:
  <["1", "2"]>
Expected was:
  <[["1", "2"]]>
@bric3
Copy link
Contributor

bric3 commented Aug 23, 2016

For reference this issue is not new ; see #74, #201, #211

@bric3 bric3 added this to the 2.1 milestone Aug 24, 2016
@ChristianSchwarz ChristianSchwarz mentioned this issue Aug 29, 2016
Closed
@paulduffin
Copy link
Contributor

Can you not just use varargCaptor.getAllValues() to access the list of arguments passed as varargs and then compare that?

@paulduffin paulduffin mentioned this issue Oct 26, 2017
Closed
@ChristianSchwarz
Copy link
Contributor Author

@paulduffin

Can you not just use varargCaptor.getAllValues() to access the list of arguments passed as varargs and then compare that?

This is not always possible cause getAllValues() returns all ever captured values.

when(mock).addVararg(captor.capture());
mock.addVararg(1,2);
mock.addVararg(3,4);
mock.addVararg(5,6);

captor.getAllValues() // {1,2,3,4,5,6}

vs.

when(mock).addVararg(captor.capture());
mock.addVararg(1,2,3);
mock.addVararg(4,5,6);

captor.getAllValues() // {1,2,3,4,5,6}

paulduffin added a commit to paulduffin/mockito that referenced this issue Oct 26, 2017
@paulduffin
Fix issue mockito#1222 and mockito#584
590eed7 
Adds a new method varargsAsArray(...) that indicates that the varargs
should be matched/captured as a single array rather than separate
values.
@paulduffin paulduffin mentioned this issue Oct 26, 2017
Closed
paulduffin added a commit to paulduffin/mockito that referenced this issue Oct 26, 2017
@paulduffin
Fix issue mockito#1222 and mockito#584
be0cd33 
Adds a new method varargsAsArray(...) that indicates that the varargs
should be matched/captured as a single array rather than separate
values.
mockitoguy added a commit that referenced this issue Oct 31, 2017
@mockitoguy
Experimental flexible matching of varargs
c5cae24 
Not intended for check-in but demonstrates that we can match varargs flexibly.

See #1224

Can we follow down this path and fix problem #1222 and #584 in the way the API is most convenient to use?
@mockitoguy mockitoguy mentioned this issue Oct 31, 2017
Closed
@big-andy-coates big-andy-coates mentioned this issue Nov 23, 2022
Closed
@big-andy-coates big-andy-coates mentioned this issue Nov 30, 2022
Merged
8 tasks
TimvdLippe pushed a commit that referenced this issue Dec 22, 2022
@big-andy-coates
Add type() method to ArgumentMatcher (#2807)
fe1cb2d 
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
TimvdLippe pushed a commit that referenced this issue Dec 28, 2022
@big-andy-coates @TimvdLippe
Add type() method to ArgumentMatcher (#2807)
483e15f 
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
@big-andy-coates
Copy link
Contributor

With #2807 merged, argumentCaptor(String.class) will capture a single value passed to a varargs parameter, where as argumentCaptor(String[].class) will capture any number of values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ready
Projects
None yet
Milestone
2.x
Development

Successfully merging a pull request may close this issue.

Improve vararg handling big-andy-coates/mockito
Fix issue #1222 and #584 paulduffin/mockito
5 participants
@bric3 @ChristianSchwarz @paulduffin @TimvdLippe @big-andy-coates