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

Introduce BugPattern for removing duplicate Mockito.verifyNoInteractions() calls #475

Open
3 of 4 tasks
Kamil-Gabaydullin opened this issue Jan 24, 2023 · 3 comments · May be fixed by #476
Open
3 of 4 tasks

Introduce BugPattern for removing duplicate Mockito.verifyNoInteractions() calls #475

Kamil-Gabaydullin opened this issue Jan 24, 2023 · 3 comments · May be fixed by #476

Comments

@Kamil-Gabaydullin
Copy link

Problem

When verifying that mocks didn't have interactions in tests sometimes many calls to Mockito.verifyNoInteractions() are used
verifyNoInteractions(mock1);
verifyNoInteractions(mock2);
which unnecessarily pollutes the code, because this method actually accepts varargs.
This can be simplified to just:
verifyNoInteractions(mock1, mock2);

Description of the proposed new feature

  • Support a stylistic preference.
  • Avoid a common gotcha, or potential problem.
  • Improve performance.

We should rewrite multiple calls to verifyNoInteractions to one call.

I would like to rewrite the following code:

verifyNoInteractions(mock1);
verifyNoInteractions(mock2);
...
verifyNoInteractions(mockN);

to:

verifyNoInteractions(mock1, mock2, ..., mockN);

Considerations

Tricky parts in my opinion are:

  1. That there might be a variable number of mocks verified in this way.
  2. That multiple individual calls might not be consequential.
  3. That the scope we need to check this must be just inside one test method.

Participation

  • I am willing to submit a pull request to implement this improvement.(I'm willing to try to submit a pr, this will be my first encounter with error prone 🙂)
@rickie
Copy link
Member

rickie commented Jan 24, 2023

Nice idea @Kamil-Gabaydullin! I think important to consider is where we put the verifyNoInteractions that combines all the mocks.

I'm willing to try to submit a pr, this will be my first encounter with error prone 🙂.

Sounds good, let us know if you have any questions. We can definitely have a chat about it.

I started thinking about this check and can write out my train of thoughts 😄. Let me know if it is clear or you have different ideas, we're definitely open to that as well 🙂. A rough idea on a possible implementation direction:

  • Create an Error Prone BugChecker that matches on MethodTrees.
  • For a given MethodTree determine whether it is a test method using the MoreJUnitMatchers#TEST_METHOD.
  • If so, traverse the MethodTree using a TreeScanner (a relatively unrelated example of such a scanner is in the LexicographicalAnnotationAttributeListing). The scanner should find all MethodInvocationTrees that are calling verifyNoInteractions.
  • Next up, collect the mocks that are passed as parameter.
  • Insert a statement that combines all the mocks in one invocation (or insert them in the last occurrence of verifyNoInteractions.

We can also discuss implementation details IRL if you'd prefer that 😄. It can sound a bit daunting at first.

@ghokun
Copy link
Member

ghokun commented Jan 25, 2023

Should we also add verifyNoMoreInteractions?

@Kamil-Gabaydullin
Copy link
Author

I was thinking the same @ghokun, perhaps as a step 2 🙂
I've made an attempt to solve this issue in this PR, there are some things I couldn't resolve though, would love to hear feedback on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants