Hack about to fix #2796 #2797

big-andy-coates · 2022-11-23T11:50:40Z

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

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.

@test

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.

TimvdLippe · 2022-11-23T15:14:40Z

src/test/java/org/mockitousage/stubbing/StubbingWithAdditionalAnswersTest.java


        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");


Is this the only stub that would now start failing?

Unintentional removal - I'll add it back in.

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 · 2022-11-23T18:31:04Z

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 · 2022-11-23T18:31:47Z

Questions for me are:

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...
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.
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:

Have default implementation that returns Object.class, avoiding breaking change.
Add default constructor to maintain existing API.
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 · 2022-11-23T18:41:26Z

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

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!

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 · 2022-11-23T22:17:41Z

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 · 2022-11-26T20:31:35Z

@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.

big-andy-coates added 2 commits November 23, 2022 11:50

spotless

8f1bd18

TimvdLippe reviewed Nov 23, 2022

View reviewed changes

Add back in unintentional removal

7f89900

big-andy-coates mentioned this pull request Nov 28, 2022

Simplify MatcherApplicationStrategy #2803

Merged

8 tasks

big-andy-coates closed this Nov 28, 2022

big-andy-coates deleted the varargs branch November 28, 2022 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hack about to fix #2796 #2797

Hack about to fix #2796 #2797

big-andy-coates commented Nov 23, 2022 •

edited

Loading

TimvdLippe Nov 23, 2022

big-andy-coates Nov 23, 2022

TimvdLippe Nov 23, 2022

codecov-commenter commented Nov 23, 2022 •

edited

Loading

big-andy-coates commented Nov 23, 2022 •

edited

Loading

TimvdLippe commented Nov 23, 2022

big-andy-coates commented Nov 23, 2022

TimvdLippe commented Nov 26, 2022

Hack about to fix #2796 #2797

Hack about to fix #2796 #2797

Conversation

big-andy-coates commented Nov 23, 2022 • edited Loading

TimvdLippe Nov 23, 2022

Choose a reason for hiding this comment

big-andy-coates Nov 23, 2022

Choose a reason for hiding this comment

TimvdLippe Nov 23, 2022

Choose a reason for hiding this comment

codecov-commenter commented Nov 23, 2022 • edited Loading

Codecov Report

big-andy-coates commented Nov 23, 2022 • edited Loading

TimvdLippe commented Nov 23, 2022

big-andy-coates commented Nov 23, 2022

TimvdLippe commented Nov 26, 2022

big-andy-coates commented Nov 23, 2022 •

edited

Loading

codecov-commenter commented Nov 23, 2022 •

edited

Loading

big-andy-coates commented Nov 23, 2022 •

edited

Loading