Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArgumentCaptor should be type aware #565

Open
ChristianSchwarz opened this issue Aug 17, 2016 · 9 comments

Comments

@ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Aug 17, 2016

Assume we have the following test.

 @Mock
 private Consumer<Collection<?>> consumer;

 @Test
 public void test() throws Exception {

    consumer.accept(new HashSet<>());
    consumer.accept(new ArrayList<>());

    ArgumentCaptor<ArrayList<?>> captor = ArgumentCaptor.forClass(ArrayList.class);
    verify(consumer).accept(captor.capture());
 }

The test fails with:

org.mockito.exceptions.verification.TooManyActualInvocations:
consumer.accept(<Capturing argument>);
Wanted 1 time:
-> at foo.TestClass.testName(TestClass.java:64)
But was 2 times. Undesired invocation:
-> at agh.TestClass.testName(TestClass.java:61)

    at foo.TestClass.test(TestClass.java:64)
    [..]

The problem here is that ArgumentCaptor is not type aware. That should be fixed.

This issue was extracted from #358 and relates to #439.

@bric3

This comment has been minimized.

Copy link
Contributor

@bric3 bric3 commented Aug 17, 2016

We are aware of that issue, the design of this tool is to rely on the method signature. Not to capture an argument of a certain type.

This tool will be redesigned for Mockito 2.1. It would be nice if it could allow developers to express an arg capture depending on conditions or to capture everything passing by.

@mockitoguy

This comment has been minimized.

Copy link
Member

@mockitoguy mockitoguy commented Aug 19, 2016

@ChristianSchwarz, guys any ideas how to improve it?

@ChristianSchwarz

This comment has been minimized.

Copy link
Contributor Author

@ChristianSchwarz ChristianSchwarz commented Aug 21, 2016

The CapturingMatcher can use the Class-Object passed to the ArgumentCaptor and capture only arguments that are instances of the expected type. This solution has a down side the captor still matches anything which may surprise users, here is an example:

stringArgs= ArgumentCaptor.forClass(String.class);
mock.call(123);
mock.call("abc");
verify(mock).call(stringArgs.capture()); //fails with too many invocations cause call(123) matches too.

If the CapturingMatcher matches only instances of the expected type then existing client tests may break. But if we go this way we open a "can of worms™" cause CapturingMatcher implements VarargMatcher and there are many corner case that must be handled in ArgumentsComparator. I currently write regression tests to explore these. When I have more details I will post them here.

@terebesirobert

This comment has been minimized.

Copy link

@terebesirobert terebesirobert commented Aug 22, 2016

@ChristianSchwarz Hi,
This seems to be related to #358. WDYT?

@ChristianSchwarz

This comment has been minimized.

Copy link
Contributor Author

@ChristianSchwarz ChristianSchwarz commented Aug 22, 2016

@terebesirobert

This seems to be related to #358. WDYT?

Absolutly, I splitted #358 cause it included two toppics:

  1. adding a new captor API captureIf( ),still ticket #358
  2. type safe capture, this ticket

I think your PR #358 can be used to fix this issue. Since the the new API captureIf(..) is not approved by the mockito-core-team I think it should be excluded for this issue. Maybe you can create a branch of your solution and send a new PR to fix this issue.

@bric3

This comment has been minimized.

Copy link
Contributor

@bric3 bric3 commented Aug 22, 2016

This work is not yet accepted because, we want to freeze new features for mockito 2.0, we will explore new API — thanks to @ChristianSchwarz and others contributors — in version 2.1.

@ChristianSchwarz I'm eager to see what's possible to do. I'm leaning for a builder style API from the actual ArgumentCaptor. For annotations a reduced set of features might be added via either boolean and/or enum.

Also about type safety, let's keep in mind that Java 8 compiler can resolve type on a callsite. which is not available on previous java versions.

@paulduffin

This comment has been minimized.

Copy link
Contributor

@paulduffin paulduffin commented Mar 27, 2017

What is the status of this (or #358)?

@TimvdLippe

This comment has been minimized.

Copy link
Contributor

@TimvdLippe TimvdLippe commented Mar 27, 2017

Type-safety is still something I am investigating, targeting Mockito 3. It seems that fixing it without strong typing, e.g. dynamically checking the generics, is very hacky and error-prone. It would be nice to have a solution where the compiler can catch these issues and make sure the verification is working, rather than on run-time failing.

@adriangalera

This comment has been minimized.

Copy link

@adriangalera adriangalera commented Aug 8, 2019

What's the status on that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.