Hack about to fix #2796

[big-andy-coates]

This PR contains changes NOT intended to be committed 'as-is', but as a showcase for a potential solution to:

• Mocking varargs method with any(String[].class) doesn't work as expected #2796
• Mockito confuses varargs and single-param methods (regression) #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,

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.

The PR includes the tests suggested in #1236, though one still fails (and is commented out with explanation).

Crux of the change is in MatcherApplicationStrategy. Obviously, it needs some refactoring. Not sure if you'd want the refactor as a separate PR or as the first commit of the main PR????

Alternatively, I could resurrect #1224 and take it through to merging.

[TimvdLippe]

Is this the only stub that would now start failing?

[big-andy-coates]

Unintentional removal - I'll add it back in.

[TimvdLippe]

Ah great! In that case, if all existing tests pass and these additional cases also pass, I am up for this! I have not reviewed your implementation yet, but as long as we can keep all existing regression tests passing I am happy 😄

[codecov-commenter]

Codecov Report

Base: 86.20% // Head: 86.20% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (7f89900) compared to base (3faa002).
Patch coverage: 97.05% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2797      +/-   ##
============================================
- Coverage     86.20%   86.20%   -0.01%     
- Complexity     2833     2839       +6     
============================================
  Files           320      320              
  Lines          8596     8602       +6     
  Branches       1060     1064       +4     
============================================
+ Hits           7410     7415       +5     
  Misses          905      905              
- Partials        281      282       +1     

Impacted Files	Coverage Δ
...ito/internal/hamcrest/HamcrestArgumentMatcher.java	85.71% <0.00%> (-14.29%)	⬇️
src/main/java/org/mockito/ArgumentCaptor.java	100.00% <100.00%> (ø)
...nternal/invocation/MatcherApplicationStrategy.java	100.00% <100.00%> (ø)
...c/main/java/org/mockito/internal/matchers/Any.java	100.00% <100.00%> (ø)
...rg/mockito/internal/matchers/CapturingMatcher.java	100.00% <100.00%> (ø)
...java/org/mockito/internal/matchers/InstanceOf.java	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[big-andy-coates]

Questions for me are:

1. Are we happy with the API change to VarargMatcher? This could be either:
   1. with a default implementation that returns Object.class, which would NOT be a breaking change for existing implementations, but the default return value is a bit ... meh!
   2. without a default implementation, which IS a breaking change...
2. Are we happy with the API change to CapturingMatcher? i.e. the addition of the constructor taking the type.
   If this type is considered part of the public API, then probably best to add a (deprecated?) default constructor as well, which sets clazz to Object.class.
3. What to do about shouldVerifyVarArgs_isNotNull_nullArrayArg test? It currently fails. It fails because NotNull doesn't implement VarargMatcher. Even if it did, it would need a variant that took the type, i.e.String[].class, for the new code to work.

Everything else seems internal implementation details.

My personal view would be:

1. Have default implementation that returns Object.class, avoiding breaking change.
2. Add default constructor to maintain existing API.
3. Not sure...

Would be great if anyone out there with more context / experience can raise any edge cases that may have been missed. I can see @ChristianSchwarz, @mockitoguy, @paulduffin have all worked in this area...)

[TimvdLippe]

1. Are we happy with the API change to VarargMatcher? This could be either:

   1. with a default implementation that returns Object.class, which would NOT be a breaking change for existing implementations, but the default return value is a bit ... meh!
   2. without a default implementation, which IS a breaking change...

These matchers are in org.mockito.internal, so we don't consider them part of our public API. Therefore, we can technically make any breaking change we want in them. However, if we can avoid large breakages (because I assume some of our users will use them, especially VarargMatcher which is known on StackOverflow to be used https://stackoverflow.com/a/7988918). Therefore, I think if we avoid any breakage by return Object.class, I would say let's go for that.

Looking at your implementation, I think the overall design looks fine. There are probably a couple of nits, but I like the general direction.

[big-andy-coates]

I'll probably create a new PR for the actual work, starting afresh. I'm going to refactor MatcherApplicationStrategy first, as its weirdly overly complicated. It could just be a utility method.

Would you prefer I do that in a separate PR, or just a separate commit in main PR?

[TimvdLippe]

@big-andy-coates Let's do separate PRs, but please keep refactoring to a minimum. Refactoring MatcherApplicationStrategy seems good to me, but let's keep it limited to that. I would prefer not to overhaul the system too much here.