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

Mocking varargs method with any(String[].class) doesn't work as expected #2796

Closed
big-andy-coates opened this issue Nov 22, 2022 · 3 comments · Fixed by #2807
Closed

Mocking varargs method with any(String[].class) doesn't work as expected #2796

big-andy-coates opened this issue Nov 22, 2022 · 3 comments · Fixed by #2807

Comments

@big-andy-coates
Copy link
Contributor

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

This issue is related to a resolved issue #2644, which was resolved in 4.7.0 by #2664.

As per the comment by @perlun in the above PR: #2664 (comment)

@big-andy-coates I found a similar semi-related issue I think... With 4.7.0, this now works correctly:

        doAnswer(answerVoid((VoidAnswer2<String, Object[]>)
                logger::info
        )).when(mock)
                .info(any(), (Object[]) any());

However, if I try to use this form which is slightly more ergonomic:

        doAnswer( answerVoid( (VoidAnswer2<String, Object[]>)
                logger::info
        ) ).when( mock )
                .info( any(), any( Object[].class ) );

...the mocking will fail to intercept these method calls. 🤔 No error when creating the mock, but it just won't detect the method calls as expected.

I presume this has again something to do with the varargs parsing. javac resolves those two calls to the correct/same logger.info() overload, so so far so good.

Do you want me to create a separate issue/repro repo for this? I'm not even sure if it's something that can be (easily) fixed, I just happened to see it now while trying to "clean up our code"... 😅

Originally posted by @perlun in #2664 (comment)
@big-andy-coates
Copy link
Contributor Author

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

I've done some digging and the reason the above fails is that Mockito is using the last matcher to match all varags parameters.

Let's simplify the example to make it easier to explain:

For example,

Given:

   String varargMethod(int x, String... args);

With setup:

when(theMock.varargMethod(eq(1), any(String[].class)).thenReturn("whatever");

When the mock encounters a call like:

theMock.varargMethod(1, "a", "b");

It uses the first eq(1) matcher to match the first argument, which passes. It then see's there are more parameters to the call than matchers, so it uses the last matcher for any additional arguments, i.e. it tries to match any(String[].class) against "a" and against "b". Of course, both of these fail and hence the method returns null, not "whatever".

If the test code is changed to:

when(theMock.varargMethod(eq(1), any(String.class)).thenReturn("whatever");

...then it works. Indeed, any of the following work:

// Matches any call with one _or more_ String parameters
when(theMock.varargMethod(eq(1), any(String.class)).thenReturn("whatever");
when(theMock.varargMethod(eq(1), any()).thenReturn("whatever");

// Matches any call with two _or more_ String parameters
when(theMock.varargMethod(eq(1), any(String.class), any(String.class)).thenReturn("whatever");
when(theMock.varargMethod(eq(1), any(), any()).thenReturn("whatever");

Conclusion

This could be seen as a bug: the code code see that the last parameter is vararg, and effectively treat the any(String[].class) as any(String.class). However, we'd need to thing through the edge cases of this and how such a change would affect existing usages.

or...

This could be seen as intentional behaviour.

Releated

Interestingly, there used to be a anyVargs method, but it was removed.

There was also a PRs that got close to merging that might have solved this and other varargs related issues... (#1224, #1235, #1236, #1461)

There are also a whole host of other issues pertaining to issues with varargs, (#1222, #584, #1593)

Unfortunately, I personally don't have time to sift through them all to distill the underlying issues and a suitable solution...

@TimvdLippe
Copy link
Contributor

Given our shenanigans with var args, I feel link our current solution is the most pragmatic and least-surprising. As you noted, we have had loads of back-and-forths on the issue of varargs, with numerous competing implementations and surprising behavior with each one. Acknowledging that there are still edge cases that are annoying, I think what we have today is the best we got. So therefore I think I would qualify this as

This could be seen as intentional behaviour.

@big-andy-coates
Copy link
Contributor Author

I'd kind of agree, given I don't have time to fix what looks to be a complicated issue, and there are workarounds.

But then I got looking into this closely related #1593, and was thinking about a fix, and I may have a solution.

I'll throw up a hack PR to see if we think the approach can work.

big-andy-coates added a commit to big-andy-coates/mockito that referenced this issue Nov 23, 2022
@big-andy-coates
Hack about to fix mockito#2796
4b1d533 
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.
@big-andy-coates big-andy-coates mentioned this issue Nov 23, 2022
Closed
big-andy-coates added a commit to big-andy-coates/mockito that referenced this issue Nov 28, 2022
@big-andy-coates
Simplify MatcherApplicationStrategy
2c6a341 
Prep work for mockito#2796

The class is overly complex, with the precomputed `matchingType` adding to value.
@big-andy-coates big-andy-coates mentioned this issue Nov 28, 2022
Merged
8 tasks
TimvdLippe pushed a commit that referenced this issue Nov 28, 2022
@big-andy-coates
Simplify MatcherApplicationStrategy (#2803)
8f4af18 
Prep work for #2796

The class is overly complex, with the precomputed `matchingType` adding to value.
big-andy-coates added a commit to big-andy-coates/mockito that referenced this issue Nov 28, 2022
@big-andy-coates
Improve vararg handling
48a3014 
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.
This was referenced Nov 28, 2022
Closed
Merged
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
@DaveTryon DaveTryon mentioned this issue Jan 11, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve vararg handling big-andy-coates/mockito
Improve vararg handling big-andy-coates/mockito
2 participants
@TimvdLippe @big-andy-coates