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.

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

Closed
lewisd32 opened this issue Jan 17, 2019 · 7 comments · Fixed by #2835
Closed

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

lewisd32 opened this issue Jan 17, 2019 · 7 comments · Fixed by #2835

Comments

@lewisd32
Copy link

In Mockito 2.2.5 and later, when setting expectations for mocking a varargs method, using an array doesn't work:

Mockito.when(good.call(Mockito.any(String[].class))).thenReturn(1L);

And instead the contained type must be used:

Mockito.when(good.call(Mockito.any(String.class))).thenReturn(1L);

However in the face of an overload of the varargs method that takes a single argument (of the same type):

    Long call(String value);
    Long call(String... values);

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

@jjDevPL
Copy link

jjDevPL commented Mar 13, 2019

I can fix it.

@jjDevPL
Copy link

jjDevPL commented Apr 29, 2019

try this as workaround:
Mockito.when(good.call((String[])Mockito.any())).thenReturn(1L);

@jjDevPL
Copy link

jjDevPL commented Apr 29, 2019

the problem is matches method from internal class VarArgAware in InstanceOf. It need to be override.

@bric3
Copy link
Contributor

bric3 commented May 7, 2019

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 testMockitoVarArgs1 and testMockitoVarArgs3 are failing.

  1. Reagrding testMockitoVarArgs1, this is somehow expected, as it is a part of the enhancement of Unified logic of argument matching and capturing #635, however we probably missed to document this case. The stub should now be configured as :

    Mockito.when(good.call(Mockito.any(String.class))).thenReturn(1L);

    Note the any(String.class) not any(String[].class).

  2. The issue with testMockitoVarArgs3 is that the stub is resolved to Long call(String value) because any(String.class) returns a single argument. But the production code is compiled with the vararg signature Long call(String... values). Forcing the cast to String[] don't work as well.

    The only option is to use

    Mockito.when(bad.call((String[]) Mockito.anyVararg())).thenReturn(1L);

    Unfortunately this API is deprecated since 2.1.0, also the current return type do not indicate an array and thus forces the cast. Tweaking the vararg matcher signature can solve the issue

    <T> T[] anyVararg() // enough for Java 8 as compiler infers the type
    <T> T[] vararg(Class<T> type) // for earlier javac version

    In this case deprecation of this matcher should be lifted, as it this is the only solution that allows to work with ambiguous method declarations.


@jjDevPL This doesn't happen if the API of the type to mock is designed this way (which is better) :

Long call(String value, String..., moreValues)

I don't know if you own this API, but this could solves you issue faster than us.

@bric3 bric3 added the bug label May 7, 2019
@jjDevPL
Copy link

jjDevPL commented May 19, 2019

well, the issue got bug label. There are work around but nevertheless it should be fixed. My changes are located in InstanceOf.java

protected Class getClazz() {
        return clazz;
    }

and under VarArgAware

  public boolean matches(Object actual) {
            return (actual != null) &&
                    (Primitives.isAssignableFromWrapper(actual.getClass(), getClazz())
                            || (getClazz().getComponentType() != null && getClazz().getComponentType().isAssignableFrom(actual.getClass()))
                            || getClazz().isAssignableFrom(actual.getClass()));
        }

small changes but it fixes Bad example. #635 was meged long time ago and I think Bad example is still bad.

@bric3 bric3 added enhancement and removed bug labels May 22, 2019
@padisah
Copy link

padisah commented Apr 25, 2022

I am trying to mock querydsl orderBy method:
public api methods on com.querydsl.core.support.QueryBase

public Q orderBy(OrderSpecifier<?>... o) 
public Q orderBy(OrderSpecifier<?> o)

the project is on java 1.8 yet, and the workaround using deprecated Mockito api worked, but java version change will break it.

big-andy-coates added a commit to big-andy-coates/mockito that referenced this issue Nov 23, 2022
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 added a commit to big-andy-coates/mockito that referenced this issue Nov 28, 2022
TimvdLippe pushed a commit that referenced this issue Dec 22, 2022
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 added a commit to big-andy-coates/mockito that referenced this issue Dec 23, 2022
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)`.
TimvdLippe pushed a commit that referenced this issue Dec 28, 2022
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 #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.

TimvdLippe pushed a commit that referenced this issue Dec 30, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants