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

Add ArgumentMatchers#assertArg method. #2949

Merged
merged 4 commits into from Apr 11, 2023

Conversation

maciejwalkowiak
Copy link
Contributor

Fixes #2285

Enables convenient alternative to ArgumentCaptor to assert arguments during verification:

verify(otherServiceMock).doStuff(assertArg(param -> {
    assertThat(param.getField1()).isEqualTo("value1");
    assertThat(param.getField2()).isEqualTo("value2");
}));

Verification succeeds as long as no exception is thrown from the lambda passed to assertArg, which means it works with any assertions (JUnit, AssertJ, ...).

Since this is my first PR to Mockito, I may have missed something, or perhaps the documentation should be improved. Feedback is very welcome!

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple and elegant, nice! I wonder if we can even deprecate ArgumentCaptor at this point in favor of this, but let's cross that bridge when we get to it.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah in fact, it is good to call out this new API in our main documentation. To do so, add section 55 to

and add it to the index at
* <a href="#54">54. Mocking/spying without specifying class (Since 4.9.0)</a><br/>

It's best to include an example usage (such as the one you included in your PR description).

@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (5486617) 85.70% compared to head (f9aeefe) 85.75%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2949      +/-   ##
============================================
+ Coverage     85.70%   85.75%   +0.04%     
- Complexity     2865     2872       +7     
============================================
  Files           325      325              
  Lines          8696     8716      +20     
  Branches       1074     1076       +2     
============================================
+ Hits           7453     7474      +21     
  Misses          966      966              
+ Partials        277      276       -1     
Impacted Files Coverage Δ
src/main/java/org/mockito/Mockito.java 96.55% <ø> (ø)
src/main/java/org/mockito/ArgumentMatchers.java 99.04% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@maciejwalkowiak
Copy link
Contributor Author

@TimvdLippe Javadocs added (feel free to rephrase if you find better wording). I added "(Since 5.3.0)" but I am not sure in which version exactly it would be released.

@crohit
Copy link

crohit commented Apr 3, 2023

Oops I didn't mean to approve! I was just really looking forward for this change and clicked the wrong button!.

@maciejwalkowiak
Copy link
Contributor Author

@TimvdLippe checkstyle issues are fixed now. Please take another look when you have a minute

@TimvdLippe TimvdLippe mentioned this pull request Apr 9, 2023
Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome thank you!

@TimvdLippe TimvdLippe merged commit 6ffd23e into mockito:main Apr 11, 2023
13 checks passed
@sbernard31
Copy link

@TimvdLippe,

Simple and elegant, nice! I wonder if we can even deprecate ArgumentCaptor at this point in favor of this, but let's cross that bridge when we get to it.

Even if this new feature is great and more elegant than ArgumentCaptor in some cases.
This is less powerful than ArgumentCaptor and I don't think this is a good idea to deprecate or remove ArgumentCaptor.

We have some use cases where this will be hard to replace :

  • like capturing a arguument and then use it in another verify.
  • create some utility doing general validation on some argument but returning a captured one for more specific validation.

@maciejwalkowiak
Copy link
Contributor Author

@TimvdLippe
Copy link
Contributor

@sbernard31 @maciejwalkowiak I understand that use case and I think you can achieve the same with assertArg:

AtomicReference<String> reference = new AtomicReference<>();
verify(mock).invocation(assertArg(arg -> reference.set(arg)));
verify(mock).secondInvocation(eq(reference.get()));

Hence I am not sure if we need a dedicate ArgumentCaptor class if a lambda suffices. But that was mostly speculation. No need to deprecate it already. Let's see how assertArg is used first, before we make any further decisions.

@sbernard31
Copy link

@TimvdLippe you're right about it seems we can do the same with assertArg combining with AtomicReference and it seems I was wrong when I said "This is less powerful than ArgumentCaptor".

But If we compare ArgumentCaptors to assertArg+AtomicReference, I feel ArgumentCaptor is less verbose and more easy to read and this could be a good reason to keep it. But clearly a less strong reason that what I thought at the beginning.

// assertArg + AtomicReference
AtomicReference<String> reference = new AtomicReference<>();
verify(mock).invocation(assertArg(arg -> reference.set(arg)));
verify(mock).secondInvocation(eq(reference.get()));

// ArgumentCaptor
ArgumentCaptor<String> arg = new  ArgumentCaptor<>();
verify(mock).invocation(arg.capture());
verify(mock).secondInvocation(eq(arg.getValue()));

@TimvdLippe
Copy link
Contributor

@sbernard31 Right, that's exactly what we should be monitoring in the years to come, which pattern is used more frequently than the other. For now, there is no need to deprecate it, I only saw the similarities between the two API's and hence my comment, but nothing more than that.

@edwnmrtnz
Copy link

Will this work in kotlin too? Tried it just now and the arg is null.

 @Test
    fun `should only fetch active movies`() = runBlocking {
        val repo = Mockito.mock(MoviesRepository::class.java)
        val sut = FetchMoviesUseCase(repo)

        sut.execute(page = 1)

        Mockito.verify(repo).fetch(assertArg { params ->
            Truth.assertThat(params["status"]).isEqualTo("active")
        })
        Unit
    }

Above code results to

assertArg {
            println(it)
        } must not be null
java.lang.NullPointerException: assertArg {
            println(it)
        } must not be null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertions on parameters during verification
6 participants