Fix issue #1222 and #584 #1224

paulduffin · 2017-10-26T13:57:45Z

Adds a new method varargsAsArray(...) that indicates that the varargs
should be matched/captured as a single array rather than separate
values.

Fixes #1222
Fixes #584

codecov-io · 2017-10-26T14:03:46Z

Codecov Report

Merging #1224 into release/2.x will increase coverage by 0.07%.
The diff coverage is 97.67%.

@@                Coverage Diff                @@
##             release/2.x    #1224      +/-   ##
=================================================
+ Coverage          87.27%   87.34%   +0.07%     
- Complexity          2305     2321      +16     
=================================================
  Files                287      288       +1     
  Lines               5869     5905      +36     
  Branches             703      709       +6     
=================================================
+ Hits                5122     5158      +36     
  Misses               556      556              
  Partials             191      191

Impacted Files	Coverage Δ	Complexity Δ
.../internal/progress/ArgumentMatcherStorageImpl.java	`100% <ø> (ø)`	`15 <0> (ø)`	⬇️
...ockito/internal/matchers/text/MatcherToString.java	`90% <0%> (ø)`	`4 <0> (ø)`	⬇️
...nternal/invocation/MatcherApplicationStrategy.java	`100% <100%> (ø)`	`21 <0> (+5)`	⬆️
...mockito/internal/matchers/TreatVarargsAsArray.java	`100% <100%> (ø)`	`10 <10> (?)`
src/main/java/org/mockito/ArgumentMatchers.java	`99.1% <100%> (+0.02%)`	`60 <1> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e10c540...be0cd33. Read the comment docs.

TimvdLippe

Ah cool that you were able to figure it out. Thanks a lot for the investigation and effort to create a fix @paulduffin! The implementation SFTM, but I would like either @mockitoguy or @bric3 to sign off on this logic, as they were involved in earlier changes in this code part.

paulduffin · 2017-10-26T14:15:43Z

I am working on fixing the code coverage problem and adding some more tests for TreatsVarargsAsArray.

TimvdLippe · 2017-10-26T14:17:11Z

Code coverage seems mostly concerned regarding the boilerplate toString and friends. Not a huge dealbreaker, but adding more tests for to get full coverage is 💯

Adds a new method varargsAsArray(...) that indicates that the varargs should be matched/captured as a single array rather than separate values.

ChristianSchwarz · 2017-10-26T20:48:05Z

I would like to see some test cases too, to get a feel for the API in corner cases

ArgumentCaptor<int[]>  captor;
when(mock).varargInts(varargsAsArray(captor.capture())) //varargInts(int...values)

when(mock).varargInts(varargsAsArray(eq(5))) //varargInts(int...values)

when(mock).stringVarargInts(eq("a"),varargsAsArray(123)) //stringVarargInts(String s,int...values)

when(mock).consume(varargsAsArray(eq("a"))) //consume(String s);
mock.consume("a");

when(mock).consumeInt(varargsAsArray(eq(1))) //consumeInt(int i);
mock.consumeInt(1);

paulduffin · 2017-10-27T12:57:23Z

I did add some more tests after the first upload. Do you want even more?

ChristianSchwarz · 2017-10-27T20:23:24Z

In the end the mockito-core members or you decide! I am as you a contributor and can only give the advices to add more non-happy-path tests.

Last year I looked into the this issue and other related varargs, capturing and matching issues. Which leaded to some refactorings, regarding the whole capturing and matching process. The MatcherApplicationStrategy were the most of your changes were made are a result of it. I never found a good solution to handle / prevent API misuses. From that POV I would like to know 2 things:

How does the API handle/prevent misuses and communicate them?
Can we ensure this API solves the problem and introduces no new ones.

Here is some other interesting test case:

  mock.varargsInt(1,2,3);
  verify(mock).varargInts(varargsAsArray(or(eq(null),eq(new int[]{1,2,3})))) 
  //what is the result: success, assertion failure or an exception indicating a bug

mockitoguy · 2017-10-30T15:26:14Z

Looking at it!

ChristianSchwarz · 2017-10-30T16:30:52Z

Somewhere it should be checked that this new matcher is only allowed to be placed as last-arg of a vararg call.

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 · 2017-10-31T04:29:18Z

Hey guys! This is a great discussion, thank you @TimvdLippe for bringing me in!

@paulduffin, this PR is conceivable! Before we merge it let's make an attempt to solve the use case more conveniently for our users.

@ChristianSchwarz, can you comment on #1235? I prototyped/hacked the implementation so that the use case is handled by Mockito out of the box. Can you point out gaps / add tests that demonstrate gaps?

PR #1235 makes both types of syntax work (and all existing Mockito tests are passing):

mock.mixedVarargs("1", "2", "3");

//traditional verify with varargs, same number of args
verify(mock).mixedVarargs(any(), eq("2"), eq("3"));

//new matching, using an array
verify(mock).mixedVarargs(any(), eq(new String[] {"2", "3"}));
verify(mock).mixedVarargs(any(), (String[]) any());

Thanks everyone for working on this use case!

ChristianSchwarz · 2017-10-31T21:43:26Z

@paulduffin Some puzzelz, but they are solvable...

    @Test
    public void shouldNoWorkOnNonVarargParameter_argOfTypeString()   {
        exception.expect(MockitoException.class);
        exception.expectMessage("varargsAsArray(..) is only allowed on vararg parameters");
        when(mock.simpleMethod(varargsAsArray(eq("")))).thenReturn("");
    }

    @Test
    public void shouldNoWorkOnNonVarargParameter_argOfTypeStringArray()   {
        exception.expect(MockitoException.class);
        exception.expectMessage("varargsAsArray(..) is only allowed on vararg parameters, actual parameter is of type: String");
        when(mock.simpleMethod("",varargsAsArray(eq(new String[0])))).thenReturn("");
    }

    @Test
    public void shouldNoWorkOnNonVarargParameter_argOfTypeStringArray2()   {
        exception.expect(MockitoException.class);
        exception.expectMessage("varargsAsArray(..) is only allowed on vararg parameters, actual parameter is of type: String[]");
        when(mock.simpleMethod("",varargsAsArray(new String[0]))).thenReturn("");
    }

    @Test
    public void shouldNotCompile()  {
        when(mock.intArgumentReturningInt(varargsAsArray(0))).thenReturn(1);
    }

    @Test
    public void shouldConvertRawValuesToEqualsMatcher()  {
        when(mock.varargsReturningString(varargsAsArray(0))).thenReturn("");
    }

paulduffin · 2017-11-01T13:33:52Z

They could all cause a compile error as long as the code was compiled with Error Prone and a suitable Error Prone checker was written for it.

Otherwise, the only choice is to have them as runtime errors.

Preventing it being used with non-vararg and non-array parameters could be done by having it return T[] instead of T. Unfortunately, that will not work with primitive varargs. e.g. if the parameter was int... then varargsAsArray(eq(new int[] {1})) would have to return int[] but T[] cannot be assigned to int[] because T cannot be primitive. The closest it can be is Integer and Integer[] is not assignable to int[].

That would require primitive specializations of varargsAsArray, e.g. varargsAsIntArray, etc.

mockitoguy · 2017-11-03T04:02:26Z

Thank you for explaining the PR here and in #1235. It is immensely helpful. My feedback:

+1 to adding explicit "varargs" matcher
what is the best name? What about shorter "vararg"?
with the current behavior, there are opportunities to improve usability (if we believe it is important enough, let's put together a ticket):
- more context to the verification error message when vararg is involved and number of args don't match. We could hint to the new matcher.
- improve behavior for the use cases we can solve unambiguously.

I'm doing the review of the code now.

mockitoguy

I love it. Can you consider my feedback? If you can improve the PR using small, separate commits it will be really easy to review the delta.

I suggested improvements but the change is really good! Looking forward to merging!!!

mockitoguy · 2017-11-03T04:03:15Z

src/main/java/org/mockito/ArgumentMatchers.java

@@ -1317,6 +1320,24 @@ public static double doubleThat(ArgumentMatcher<Double> matcher) {
        return 0;
    }

+    /**
+     * Indicates that the varargs should be matched as an array rather than individual values.


Nit: "individual values" -> "individual value"

mockitoguy · 2017-11-03T04:49:45Z

src/main/java/org/mockito/ArgumentMatchers.java

+     *
+     * The value must be specified as a call to a method for matching an argument, e.g.
+     * {@link #eq(Object)} or {@link ArgumentCaptor#capture()}. Failure to do so will result in
+     * either an {@link EmptyStackException} or an {@link InvalidUseOfMatchersException}.


Can you add a unit test for this behavior? Let's see how the unit test looks like and what is the user experience. If the user experience is not great, can we offer a clean exception describing API misuse?

mockitoguy · 2017-11-03T04:51:47Z

src/main/java/org/mockito/ArgumentMatchers.java

+     * {@code captor.capture()}
+     * @param <T> the type of the vararg or array of varargs
+     * @return {@code null}
+     */


This feature is really cool and it deserves new minor version. Can you add "since" tag to the Javadoc with the next minor version? Also, let's update the "version.properties" file accordingly!

mockitoguy · 2017-11-03T04:52:08Z

src/main/java/org/mockito/ArgumentMatchers.java

+     * The value must be specified as a call to a method for matching an argument, e.g.
+     * {@link #eq(Object)} or {@link ArgumentCaptor#capture()}. Failure to do so will result in
+     * either an {@link EmptyStackException} or an {@link InvalidUseOfMatchersException}.
+     *


Can you add code sample to the Javadoc in a similar way we do it for other main public methods?

mockitoguy · 2017-11-03T05:16:51Z

src/main/java/org/mockito/internal/invocation/MatcherApplicationStrategy.java

@@ -91,21 +93,35 @@ private static MatcherApplicationType getMatcherApplicationType(Invocation invoc
        final int expandedArguments = invocation.getArguments().length;
        final int matcherCount = matchers.size();

+        boolean treatVarargsAsArray = false;


Wow. I'm very impressed that you were able to inject the new algorithm here. The code is pretty complex around here. I'd love to see it refactored at some point in the future.

It is complex but you have added fantastic test coverage that I was not able to break down :)

mockitoguy · 2017-11-03T05:19:36Z

src/test/java/org/mockitousage/verification/BasicVerificationTest.java

@@ -115,4 +115,49 @@ public void shouldDetectWhenOverloadedMethodCalled() throws Exception {
            fail();
        } catch(WantedButNotInvoked e) {}
    }
+
+    @Test
+    public void shouldVerifyVarargsAsArray_nullVarargsArray() throws Exception {


Can you move those tests to a better place? "BasicVerification" is for basic operations. I suggest to move them to existing VarargTest or create a new one like VarargAsArrayTest.

BTW. thank you very much for adding truly comprehensive test coverage!!!

mockitoguy · 2017-11-03T05:21:12Z

src/test/java/org/mockitousage/verification/BasicVerificationTest.java

+
+        mock.mixedVarargs(null, (String[]) null);
+
+        verify(mock).mixedVarargs(any(), varargsAsArray(eq((String[]) null)));


Can you add coverage for negative cases, e.g. when verification error is thrown for use cases with "varargsAsArray"? This would also help us see the exception message emitted from Mockito, and if the message is handsome.

mockitoguy · 2017-11-03T05:24:21Z

src/main/java/org/mockito/ArgumentMatchers.java

@@ -1317,6 +1320,24 @@ public static double doubleThat(ArgumentMatcher<Double> matcher) {
        return 0;
    }

+    /**


Can you add a new point to the main Mockito Javadoc with a short description of the feature? This is a really cool feature and it deserves a note in main Javadoc. That doc acts as Mockito user guide and also shows notable features. Feel free to bump new minor version.

fransflippo · 2018-02-15T05:55:58Z

@paulduffin @mockitoguy Would love to get this into a release version of Mockito! Can I help address some of the issues that @mockitoguy identified perhaps so we can finish it off?

ChristianSchwarz · 2018-02-26T09:24:14Z

@mockitoguy
@TimvdLippe
Can you create a mockito branch of this PR, so other can work on it and fix the remaining issues?

TimvdLippe · 2018-02-26T11:34:19Z

@ChristianSchwarz https://github.com/mockito/mockito/tree/capture-vararg-as-array published

paulduffin · 2018-02-26T15:57:12Z

Apologies for not getting back sooner on this. Things are crazy at work at the minute. If someone else wants to address the issues that would be really helpful.

…

On 26 February 2018 at 11:34, Tim van der Lippe ***@***.***> wrote: @ChristianSchwarz <https://github.com/christianschwarz> https://github.com/mockito/mockito/tree/capture-vararg-as-array published — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEtcQ1CRM51GfoNG8LxKpozcsRvGWGwoks5tYpbIgaJpZM4QHnXV> .

TungDoanDuong · 2019-03-04T23:35:58Z

@mockitoguy
Any update on this issue?

TimvdLippe · 2019-05-02T17:03:58Z

Closing this PR as stale. Feel free to open a new PR if you are still interested in landing these changes.

TimvdLippe reviewed Oct 26, 2017

View reviewed changes

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.

paulduffin force-pushed the capture-vararg-as-array branch from 590eed7 to be0cd33 Compare October 26, 2017 14:37

mockitoguy mentioned this pull request Oct 31, 2017

Experimental flexible matching of varargs #1235

Closed

mockitoguy requested changes Nov 3, 2017

View reviewed changes

TimvdLippe assigned paulduffin Nov 5, 2017

TungDoanDuong mentioned this pull request Mar 5, 2019

ArgumentCaptor with varArgs-arrays returning unexpected behavior when using getAllValues #1652

Open

TimvdLippe closed this May 2, 2019

This was referenced Nov 23, 2022

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

Closed

Hack about to fix #2796 #2797

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #1222 and #584 #1224

Fix issue #1222 and #584 #1224

paulduffin commented Oct 26, 2017 •

edited by TimvdLippe

codecov-io commented Oct 26, 2017 •

edited

TimvdLippe left a comment

paulduffin commented Oct 26, 2017

TimvdLippe commented Oct 26, 2017

ChristianSchwarz commented Oct 26, 2017 •

edited

paulduffin commented Oct 27, 2017

ChristianSchwarz commented Oct 27, 2017

mockitoguy commented Oct 30, 2017

ChristianSchwarz commented Oct 30, 2017

mockitoguy commented Oct 31, 2017

ChristianSchwarz commented Oct 31, 2017 •

edited

paulduffin commented Nov 1, 2017

mockitoguy commented Nov 3, 2017

mockitoguy left a comment •

edited

mockitoguy Nov 3, 2017

mockitoguy Nov 3, 2017

mockitoguy Nov 3, 2017

mockitoguy Nov 3, 2017

mockitoguy Nov 3, 2017

mockitoguy Nov 3, 2017

mockitoguy Nov 3, 2017

mockitoguy Nov 3, 2017

fransflippo commented Feb 15, 2018

ChristianSchwarz commented Feb 26, 2018

TimvdLippe commented Feb 26, 2018

paulduffin commented Feb 26, 2018 via email

TungDoanDuong commented Mar 4, 2019 •

edited

TimvdLippe commented May 2, 2019


		mock.mixedVarargs(null, (String[]) null);

		verify(mock).mixedVarargs(any(), varargsAsArray(eq((String[]) null)));

Fix issue #1222 and #584 #1224

Fix issue #1222 and #584 #1224

Conversation

paulduffin commented Oct 26, 2017 • edited by TimvdLippe

codecov-io commented Oct 26, 2017 • edited

Codecov Report

TimvdLippe left a comment

Choose a reason for hiding this comment

paulduffin commented Oct 26, 2017

TimvdLippe commented Oct 26, 2017

ChristianSchwarz commented Oct 26, 2017 • edited

paulduffin commented Oct 27, 2017

ChristianSchwarz commented Oct 27, 2017

mockitoguy commented Oct 30, 2017

ChristianSchwarz commented Oct 30, 2017

mockitoguy commented Oct 31, 2017

ChristianSchwarz commented Oct 31, 2017 • edited

paulduffin commented Nov 1, 2017

mockitoguy commented Nov 3, 2017

mockitoguy left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fransflippo commented Feb 15, 2018

ChristianSchwarz commented Feb 26, 2018

TimvdLippe commented Feb 26, 2018

paulduffin commented Feb 26, 2018 via email

TungDoanDuong commented Mar 4, 2019 • edited

TimvdLippe commented May 2, 2019

paulduffin commented Oct 26, 2017 •

edited by TimvdLippe

codecov-io commented Oct 26, 2017 •

edited

ChristianSchwarz commented Oct 26, 2017 •

edited

ChristianSchwarz commented Oct 31, 2017 •

edited

mockitoguy left a comment •

edited

TungDoanDuong commented Mar 4, 2019 •

edited