Unified logic of argument matching and capturing

[ChristianSchwarz]

The logic of argument matching and capturing is the same but can be found in ArgumentsComparator and InvocationMatcher. This PR unifies the decision logic how matchers are applied to argument+varargs by introducing a MatcherApplicationStrategy. This strategy can apply an action to an pair of ArgumentMatcher and Argument. One action is used to perform the ArgumentMatcher.matches(arg) call and an other is used to perfrom the capturing.

• fixes ArgumentCaptor and ArgumentMatcher can't be mixed in varargs #439 ArgumentCaptor and ArgumentMatcher's can now be mixed in varargs
• fixes ArgumentCaptor: NPE when an null array is passed to a varargs method #583 No more NPE when null is passed to an varargs
• added 25 regression tests in VarargsTest (5 are ignored cause they belong to other issues and fail currently)
• reduced logic duplication

[codecov-io]

Current coverage is 87.33% (diff: 97.72%)

No coverage report found for release/2.x at 0fcad43.

Powered by Codecov. Last update 0fcad43...968fea6

[bric3]

on hold as we need to address the beta release thing

[ChristianSchwarz]

Since these 2 bugfixes will not be included in the 2.1.0 RC phase, should I send a new PR against release/2.x ?

[mockitoguy]

@ChristianSchwarz, yes, but let's wait until 2.1.0 is out. This way, nobody gets confused :)

[bric3]

@ChristianSchwarz I've changed the base branch to release/2.x

[ChristianSchwarz]

[..] let's wait until 2.1.0 is out. This way, nobody gets confused :)

@mockito-team
Now I am confused! Who can mockito users be confused by bugfixes?

In the meantime on the master branch...

• refactorings were merged like Fixing the typo in JUnitDetecter class name #656 and Renamed FailureDetecter to Failure Detector. #654
• new features are going to be merged too Added InlineByteBuddyMockMaker which uses the instrumentation API for redefining classes and inlining the mocking logic. #648
• but bugfixed won't be merged ?

[TimvdLippe]

We wanted to freeze the release branch while waiting for the RC. #648 is a special case which originates from JavaOne, a major selling point for Mockito. We dont plan to add other changes in the RC.

[bric3]

@ChristianSchwarz this thing will go in 2.2, I mean it ;)
I think Capturing matcher deserves an overfaul, and I like the enhancement you've made, but feature freeze is essential, sorry for the longer RC phase, but I think it's for the better ;)

[bric3]

@ChristianSchwarz By the way could you rebase your commits on top of release/2x, because git seems to see unrelated commits form @TimvdLippe

[ChristianSchwarz]

@bric3 This PR is now rebased on release/2x. It the master now used for mockito 3.x ?
@ALL If you have some time besided the numerous CI-tasks, please review this PR.

[bric3]

Great thanks. It may be delayed again due to some disagreement on the CI-CD...

Otherwise yes the master is for mockito 3.x. I should probably update the contributing guide and the template as well

[bric3]

Some adjustments can be made e.g. getArgumentType can be simplified to read the type arg on the class rather than looking at the method. But it's a nice code improvement.

Other improvements can be made later. Thanks again @ChristianSchwarz !

[mockitoguy]

@ChristianSchwarz, this is an impressive code change, improving not only the behavior but also code and test coverage!!! THANKS!

Thanks @bric3 for reviewing and merging the PR!

[ChristianSchwarz]

@bric3

Some adjustments can be made e.g. getArgumentType can be simplified to read the type arg on the class rather than looking at the method.

The mechanic remained unchanged in this RP, I introduced it with #463. At first I tried to retrieve the Type-Argument from the class but this turned out to be more complex. E.g. when the type-argument is bound in an upper class or interface or additional type-arguments were introduced on a matcher class.
Anyway if you have an idea how to improve or simplify it let me now!

Other improvements can be made later.

Sounds good, can you open a new ticket, so I can take look.

[ChristianSchwarz]

@mockito-core
#606 can be closed now

[bric3]

Other improvements can be made later.

Sounds good, can you open a new ticket, so I can take look.

At the moment this works quite well! So no hurry, especially given your above remark. On mockito 3 (on master atm) we could change the interface to allow reporting the type handled by the matcher and maybe use default method for that.

[bric3]

@ChristianSchwarz I have seen an interesting problem with VarargsTest fail on master, which is JDK 8 only, while the same test passes on release/2.x which is compiled and run against 3 JDK 6/7/8. I'm not sure why at the moment this behavior is different . Plus my dev laptop is being fixed for a few days, so I'm limited in my debug right now.

This happened after a merge of release/2.x to master (52df10a).

I noticed that verify(mock).varargs(isNull()); is compiled to invoke IMethods.varargs(String ... string). I'm not sure other JDKs behave/compile differently though.

One of the failing test can be fixed by adding the cast statement

    @Test
    public void shouldMatchVarArgs_nullArrayArg() {
        Object[] argArray = null;
        mock.varargs(argArray);

        verify(mock).varargs((Object[]) isNull()); // otherwise it is linked against String[]
    }

Could it be that something went wrong with the merge regarding this PR ?

[bric3]

@ChristianSchwarz Actually the investigation continue, see #717 (comment)