From 4b1d5335de7935ff837d86a6bf407fe2b5fbf657 Mon Sep 17 00:00:00 2001 From: Big Andy <8012398+big-andy-coates@users.noreply.github.com> Date: Wed, 23 Nov 2022 11:50:21 +0000 Subject: [PATCH] Hack about to fix #2796 This PR contains changes NOT intended to be committed 'as-is', but as a showcase for a potential solution to: * https://github.com/mockito/mockito/issues/2796 * https://github.com/mockito/mockito/issues/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. --- src/main/java/org/mockito/ArgumentCaptor.java | 3 +- .../hamcrest/HamcrestArgumentMatcher.java | 6 + .../MatcherApplicationStrategy.java | 81 ++++++------- .../org/mockito/internal/matchers/Any.java | 6 + .../internal/matchers/CapturingMatcher.java | 11 ++ .../mockito/internal/matchers/InstanceOf.java | 8 +- .../internal/matchers/VarargMatcher.java | 7 +- .../invocation/InvocationMatcherTest.java | 4 +- .../MatcherApplicationStrategyTest.java | 6 + .../matchers/CapturingMatcherTest.java | 8 +- .../StubbingWithAdditionalAnswersTest.java | 110 +++++++++++++++++- 11 files changed, 193 insertions(+), 57 deletions(-) diff --git a/src/main/java/org/mockito/ArgumentCaptor.java b/src/main/java/org/mockito/ArgumentCaptor.java index afb3add2ba..ed9e398f91 100644 --- a/src/main/java/org/mockito/ArgumentCaptor.java +++ b/src/main/java/org/mockito/ArgumentCaptor.java @@ -62,11 +62,12 @@ @CheckReturnValue public class ArgumentCaptor { - private final CapturingMatcher capturingMatcher = new CapturingMatcher(); + private final CapturingMatcher capturingMatcher; private final Class clazz; private ArgumentCaptor(Class clazz) { this.clazz = clazz; + this.capturingMatcher = new CapturingMatcher(clazz); } /** diff --git a/src/main/java/org/mockito/internal/hamcrest/HamcrestArgumentMatcher.java b/src/main/java/org/mockito/internal/hamcrest/HamcrestArgumentMatcher.java index 99869faf73..a39b9159ea 100644 --- a/src/main/java/org/mockito/internal/hamcrest/HamcrestArgumentMatcher.java +++ b/src/main/java/org/mockito/internal/hamcrest/HamcrestArgumentMatcher.java @@ -9,6 +9,8 @@ import org.mockito.ArgumentMatcher; import org.mockito.internal.matchers.VarargMatcher; +import java.util.Optional; + public class HamcrestArgumentMatcher implements ArgumentMatcher { private final Matcher matcher; @@ -26,6 +28,10 @@ public boolean isVarargMatcher() { return matcher instanceof VarargMatcher; } + public Optional varargMatcher() { + return isVarargMatcher() ? Optional.of((VarargMatcher) matcher) : Optional.empty(); + } + @Override public String toString() { // TODO SF add unit tests and integ test coverage for toString() diff --git a/src/main/java/org/mockito/internal/invocation/MatcherApplicationStrategy.java b/src/main/java/org/mockito/internal/invocation/MatcherApplicationStrategy.java index acc7382e2c..eb1c0f4410 100644 --- a/src/main/java/org/mockito/internal/invocation/MatcherApplicationStrategy.java +++ b/src/main/java/org/mockito/internal/invocation/MatcherApplicationStrategy.java @@ -4,12 +4,10 @@ */ package org.mockito.internal.invocation; -import static org.mockito.internal.invocation.MatcherApplicationStrategy.MatcherApplicationType.ERROR_UNSUPPORTED_NUMBER_OF_MATCHERS; -import static org.mockito.internal.invocation.MatcherApplicationStrategy.MatcherApplicationType.MATCH_EACH_VARARGS_WITH_LAST_MATCHER; -import static org.mockito.internal.invocation.MatcherApplicationStrategy.MatcherApplicationType.ONE_MATCHER_PER_ARGUMENT; - +import java.lang.reflect.Type; import java.util.ArrayList; import java.util.List; +import java.util.Optional; import org.mockito.ArgumentMatcher; import org.mockito.internal.hamcrest.HamcrestArgumentMatcher; @@ -21,21 +19,12 @@ public class MatcherApplicationStrategy { private final Invocation invocation; private final List> matchers; - private final MatcherApplicationType matchingType; private MatcherApplicationStrategy( Invocation invocation, - List> matchers, - MatcherApplicationType matchingType) { + List> matchers) { this.invocation = invocation; - if (matchingType == MATCH_EACH_VARARGS_WITH_LAST_MATCHER) { - int times = varargLength(invocation); - this.matchers = appendLastMatcherNTimes(matchers, times); - } else { - this.matchers = matchers; - } - - this.matchingType = matchingType; + this.matchers = matchers; } /** @@ -53,8 +42,7 @@ private MatcherApplicationStrategy( public static MatcherApplicationStrategy getMatcherApplicationStrategyFor( Invocation invocation, List> matchers) { - MatcherApplicationType type = getMatcherApplicationType(invocation, matchers); - return new MatcherApplicationStrategy(invocation, matchers, type); + return new MatcherApplicationStrategy(invocation, matchers); } /** @@ -74,11 +62,35 @@ public static MatcherApplicationStrategy getMatcherApplicationStrategyFor( * */ public boolean forEachMatcherAndArgument(ArgumentMatcherAction action) { - if (matchingType == ERROR_UNSUPPORTED_NUMBER_OF_MATCHERS) { - return false; + final boolean isVararg = invocation.getMethod().isVarArgs() + && invocation.getRawArguments().length == matchers.size() + && getLastVarargMatcher(matchers).isPresent(); + + if (isVararg) { + final Type type = getLastVarargMatcher(matchers).get().type(); + final Class varArgType = invocation.getMethod().getParameterTypes()[invocation.getMethod().getParameterTypes().length - 1]; + if (type.equals(varArgType)) { + return argsMatch(invocation.getRawArguments(), matchers, action); + } + } + + if (invocation.getArguments().length == matchers.size()) { + return argsMatch(invocation.getArguments(), matchers, action); + } + + if (isVararg) { + int times = varargLength(invocation); + final List> matchers = appendLastMatcherNTimes(this.matchers, times); + return argsMatch(invocation.getArguments(), matchers, action); } - Object[] arguments = invocation.getArguments(); + return false; + } + + private boolean argsMatch(final Object[] arguments, + final List> matchers, + final ArgumentMatcherAction action) { + for (int i = 0; i < arguments.length; i++) { ArgumentMatcher matcher = matchers.get(i); Object argument = arguments[i]; @@ -90,29 +102,12 @@ public boolean forEachMatcherAndArgument(ArgumentMatcherAction action) { return true; } - private static MatcherApplicationType getMatcherApplicationType( - Invocation invocation, List> matchers) { - final int rawArguments = invocation.getRawArguments().length; - final int expandedArguments = invocation.getArguments().length; - final int matcherCount = matchers.size(); - - if (expandedArguments == matcherCount) { - return ONE_MATCHER_PER_ARGUMENT; - } - - if (rawArguments == matcherCount && isLastMatcherVarargMatcher(matchers)) { - return MATCH_EACH_VARARGS_WITH_LAST_MATCHER; - } - - return ERROR_UNSUPPORTED_NUMBER_OF_MATCHERS; - } - - private static boolean isLastMatcherVarargMatcher(final List> matchers) { + private static Optional getLastVarargMatcher(final List> matchers) { ArgumentMatcher argumentMatcher = lastMatcher(matchers); if (argumentMatcher instanceof HamcrestArgumentMatcher) { - return ((HamcrestArgumentMatcher) argumentMatcher).isVarargMatcher(); + return ((HamcrestArgumentMatcher) argumentMatcher).varargMatcher(); } - return argumentMatcher instanceof VarargMatcher; + return argumentMatcher instanceof VarargMatcher ? Optional.of((VarargMatcher) argumentMatcher) : Optional.empty(); } private static List> appendLastMatcherNTimes( @@ -135,10 +130,4 @@ private static int varargLength(Invocation invocation) { private static ArgumentMatcher lastMatcher(List> matchers) { return matchers.get(matchers.size() - 1); } - - enum MatcherApplicationType { - ONE_MATCHER_PER_ARGUMENT, - MATCH_EACH_VARARGS_WITH_LAST_MATCHER, - ERROR_UNSUPPORTED_NUMBER_OF_MATCHERS; - } } diff --git a/src/main/java/org/mockito/internal/matchers/Any.java b/src/main/java/org/mockito/internal/matchers/Any.java index 7ad113feed..c977ae2135 100644 --- a/src/main/java/org/mockito/internal/matchers/Any.java +++ b/src/main/java/org/mockito/internal/matchers/Any.java @@ -5,6 +5,7 @@ package org.mockito.internal.matchers; import java.io.Serializable; +import java.lang.reflect.Type; import org.mockito.ArgumentMatcher; @@ -21,4 +22,9 @@ public boolean matches(Object actual) { public String toString() { return ""; } + + @Override + public Type type() { + return Object.class; + } } diff --git a/src/main/java/org/mockito/internal/matchers/CapturingMatcher.java b/src/main/java/org/mockito/internal/matchers/CapturingMatcher.java index 5138839ddb..0dc688da12 100644 --- a/src/main/java/org/mockito/internal/matchers/CapturingMatcher.java +++ b/src/main/java/org/mockito/internal/matchers/CapturingMatcher.java @@ -7,6 +7,7 @@ import static org.mockito.internal.exceptions.Reporter.noArgumentValueWasCaptured; import java.io.Serializable; +import java.lang.reflect.Type; import java.util.ArrayList; import java.util.List; import java.util.concurrent.locks.Lock; @@ -19,12 +20,17 @@ public class CapturingMatcher implements ArgumentMatcher, CapturesArguments, VarargMatcher, Serializable { + private final Class clazz; private final List arguments = new ArrayList<>(); private final ReadWriteLock lock = new ReentrantReadWriteLock(); private final Lock readLock = lock.readLock(); private final Lock writeLock = lock.writeLock(); + public CapturingMatcher(final Class clazz) { + this.clazz = clazz; + } + @Override public boolean matches(Object argument) { return true; @@ -66,4 +72,9 @@ public void captureFrom(Object argument) { writeLock.unlock(); } } + + @Override + public Type type() { + return clazz; + } } diff --git a/src/main/java/org/mockito/internal/matchers/InstanceOf.java b/src/main/java/org/mockito/internal/matchers/InstanceOf.java index 1b4b30e875..ee7de83bd6 100644 --- a/src/main/java/org/mockito/internal/matchers/InstanceOf.java +++ b/src/main/java/org/mockito/internal/matchers/InstanceOf.java @@ -5,13 +5,14 @@ package org.mockito.internal.matchers; import java.io.Serializable; +import java.lang.reflect.Type; import org.mockito.ArgumentMatcher; import org.mockito.internal.util.Primitives; public class InstanceOf implements ArgumentMatcher, Serializable { - private final Class clazz; + final Class clazz; private final String description; public InstanceOf(Class clazz) { @@ -44,5 +45,10 @@ public VarArgAware(Class clazz) { public VarArgAware(Class clazz, String describedAs) { super(clazz, describedAs); } + + @Override + public Type type() { + return clazz; + } } } diff --git a/src/main/java/org/mockito/internal/matchers/VarargMatcher.java b/src/main/java/org/mockito/internal/matchers/VarargMatcher.java index 43a27596f8..9e086743db 100644 --- a/src/main/java/org/mockito/internal/matchers/VarargMatcher.java +++ b/src/main/java/org/mockito/internal/matchers/VarargMatcher.java @@ -5,9 +5,14 @@ package org.mockito.internal.matchers; import java.io.Serializable; +import java.lang.reflect.Type; /** * Internal interface that informs Mockito that the matcher is intended to capture varargs. * This information is needed when mockito collects the arguments. */ -public interface VarargMatcher extends Serializable {} +public interface VarargMatcher extends Serializable { + + // Todo: default impl to avoid compatability issues? + Type type(); +} diff --git a/src/test/java/org/mockito/internal/invocation/InvocationMatcherTest.java b/src/test/java/org/mockito/internal/invocation/InvocationMatcherTest.java index 0b3d09d5f1..ab53be545f 100644 --- a/src/test/java/org/mockito/internal/invocation/InvocationMatcherTest.java +++ b/src/test/java/org/mockito/internal/invocation/InvocationMatcherTest.java @@ -136,7 +136,7 @@ public void should_be_similar_if_is_overloaded_but_used_with_different_arg() thr public void should_capture_arguments_from_invocation() throws Exception { // given Invocation invocation = new InvocationBuilder().args("1", 100).toInvocation(); - CapturingMatcher capturingMatcher = new CapturingMatcher(); + CapturingMatcher capturingMatcher = new CapturingMatcher(List.class); InvocationMatcher invocationMatcher = new InvocationMatcher(invocation, (List) asList(new Equals("1"), capturingMatcher)); @@ -167,7 +167,7 @@ public void should_capture_varargs_as_vararg() throws Exception { // given mock.mixedVarargs(1, "a", "b"); Invocation invocation = getLastInvocation(); - CapturingMatcher m = new CapturingMatcher(); + CapturingMatcher m = new CapturingMatcher(List.class); InvocationMatcher invocationMatcher = new InvocationMatcher(invocation, Arrays.asList(new Equals(1), m)); diff --git a/src/test/java/org/mockito/internal/invocation/MatcherApplicationStrategyTest.java b/src/test/java/org/mockito/internal/invocation/MatcherApplicationStrategyTest.java index 285bfecc66..0383cf7755 100644 --- a/src/test/java/org/mockito/internal/invocation/MatcherApplicationStrategyTest.java +++ b/src/test/java/org/mockito/internal/invocation/MatcherApplicationStrategyTest.java @@ -12,6 +12,7 @@ import static org.mockito.internal.invocation.MatcherApplicationStrategy.getMatcherApplicationStrategyFor; import static org.mockito.internal.matchers.Any.ANY; +import java.lang.reflect.Type; import java.util.ArrayList; import java.util.List; @@ -230,6 +231,11 @@ public boolean matches(Object o) { } public void describeTo(Description description) {} + + @Override + public Type type() { + return Integer.class; + } } private Invocation mixedVarargs(Object a, String... s) { diff --git a/src/test/java/org/mockito/internal/matchers/CapturingMatcherTest.java b/src/test/java/org/mockito/internal/matchers/CapturingMatcherTest.java index a4c6b59149..768d08d0b8 100644 --- a/src/test/java/org/mockito/internal/matchers/CapturingMatcherTest.java +++ b/src/test/java/org/mockito/internal/matchers/CapturingMatcherTest.java @@ -19,7 +19,7 @@ public class CapturingMatcherTest extends TestBase { @Test public void should_capture_arguments() throws Exception { // given - CapturingMatcher m = new CapturingMatcher(); + CapturingMatcher m = new CapturingMatcher(String.class); // when m.captureFrom("foo"); @@ -32,7 +32,7 @@ public void should_capture_arguments() throws Exception { @Test public void should_know_last_captured_value() throws Exception { // given - CapturingMatcher m = new CapturingMatcher(); + CapturingMatcher m = new CapturingMatcher(String.class); // when m.captureFrom("foo"); @@ -45,7 +45,7 @@ public void should_know_last_captured_value() throws Exception { @Test public void should_scream_when_nothing_yet_captured() throws Exception { // given - CapturingMatcher m = new CapturingMatcher(); + CapturingMatcher m = new CapturingMatcher(String.class); try { // when @@ -59,7 +59,7 @@ public void should_scream_when_nothing_yet_captured() throws Exception { @Test public void should_not_fail_when_used_in_concurrent_tests() throws Exception { // given - final CapturingMatcher m = new CapturingMatcher(); + final CapturingMatcher m = new CapturingMatcher(String.class); // when m.captureFrom("concurrent access"); diff --git a/src/test/java/org/mockitousage/stubbing/StubbingWithAdditionalAnswersTest.java b/src/test/java/org/mockitousage/stubbing/StubbingWithAdditionalAnswersTest.java index d037cfc0a0..464c6f0f40 100644 --- a/src/test/java/org/mockitousage/stubbing/StubbingWithAdditionalAnswersTest.java +++ b/src/test/java/org/mockitousage/stubbing/StubbingWithAdditionalAnswersTest.java @@ -28,6 +28,8 @@ import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.exceptions.base.MockitoException; import org.mockito.junit.MockitoJUnitRunner; @@ -56,12 +58,116 @@ public void can_return_arguments_of_invocation() throws Exception { given(iMethods.objectArgMethod(any())).will(returnsFirstArg()); given(iMethods.threeArgumentMethod(eq(0), any(), anyString())).will(returnsSecondArg()); given(iMethods.threeArgumentMethod(eq(1), any(), anyString())).will(returnsLastArg()); - given(iMethods.mixedVarargsReturningString(eq(1), any())).will(returnsArgAt(2)); assertThat(iMethods.objectArgMethod("first")).isEqualTo("first"); assertThat(iMethods.threeArgumentMethod(0, "second", "whatever")).isEqualTo("second"); assertThat(iMethods.threeArgumentMethod(1, "whatever", "last")).isEqualTo("last"); - assertThat(iMethods.mixedVarargsReturningString(1, "a", "b")).isEqualTo("b"); + } + + public interface Foo { + String m1(String arg); + String m1(String... args); + } + + @Mock private Foo foo; + + @Test + public void shouldWorkWithCasts() throws Exception { + given(foo.m1((String)any())).willReturn("single arg method"); + given(foo.m1((String[])any())).willReturn("var arg method"); + + assertThat(foo.m1( "a")).isEqualTo("single arg method"); + assertThat(foo.m1()).isEqualTo("var arg method"); + assertThat(foo.m1( new String[] {"a" })).isEqualTo("var arg method"); + assertThat(foo.m1("a", "b")).isEqualTo("var arg method"); + } + + @Test + public void shouldWork() throws Exception { + given(foo.m1(any(String.class))).willReturn("single arg method"); + + assertThat(foo.m1( "a")).isEqualTo("single arg method"); + assertThat(foo.m1( new String[] {"a"})).isNull(); + assertThat(foo.m1( "a", "b")).isNull(); + assertThat(foo.m1( "a", "b", "c")).isNull(); + } + + @Test + public void shouldWork2() throws Exception { + given(foo.m1(any(String[].class))).willReturn("var arg method"); + + assertThat(foo.m1( "a")).isNull(); + assertThat(foo.m1( new String[] {"a"})).isEqualTo("var arg method"); + assertThat(foo.m1( "a", "b")).isEqualTo("var arg method"); + assertThat(foo.m1( "a", "b", "c")).isEqualTo("var arg method"); + } + + @Test + public void shouldWork3() throws Exception { + given(foo.m1(any(String.class), any(String.class))).willReturn("var arg method"); + + assertThat(foo.m1( "a")).isNull(); + assertThat(foo.m1( new String[] {"a"})).isNull(); + assertThat(foo.m1( "a", "b")).isEqualTo("var arg method"); + assertThat(foo.m1( "a", "b", "c")).isNull(); + } + + @Test + public void shouldCaptureVarArgs_nullArrayArg1() { + iMethods.varargs((String[]) null); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String[].class); + verify(iMethods).varargs(captor.capture()); + } + + @Test + public void shouldCaptureVarArgs_nullArrayArg2() { + iMethods.varargs((String[]) null); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(iMethods).varargs(captor.capture()); + } + + + @Test + public void shouldVerifyVarArgs_any_nullArrayArg() { + iMethods.varargs((String[]) null); + + verify(iMethods).varargs(ArgumentMatchers.any()); + } + + @Test + public void shouldVerifyVarArgs_eq_NullArrayArg() { + iMethods.varargs((String[]) null); + + verify(iMethods).varargs(ArgumentMatchers.eq(null)); + } + + + + @Test + public void shouldVerifyVarArgs_eq_nullArrayArg() { + + iMethods.varargs((String)null); + + verify(iMethods).varargs(ArgumentMatchers.eq(null)); + } + + @Test + public void shouldVerifyVarArgs_isNull_nullArrayArg() { + + iMethods.varargs((String)null); + + verify(iMethods).varargs(ArgumentMatchers.isNull()); + } + + @Test + public void shouldVerifyVarArgs_isNotNull_nullArrayArg() { + + iMethods.varargs(); + + // Todo: still fails. Would require `NotNull` to be `VarargMatcher` I think + //verify(iMethods).varargs(ArgumentMatchers.isNotNull()); } @Test