Introduce BugPattern for removing duplicate `Mockito.verifyNoInteractions()` calls

[Kamil-Gabaydullin]

This pr adds a BugPattern to recognize multiple verifyNoInteractions() calls in test methods.

I'll leave comments throughout the pr to address questions and decisions.

[github-actions[bot]]

• Surviving mutants in this change: 3
• Killed mutants in this change: 15

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage	2	13
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1	1	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 2
• Killed mutants in this change: 18

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage	1	16
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1	1	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 2
• Killed mutants in this change: 18

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage	1	16
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1	1	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 2
• Killed mutants in this change: 18

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage	1	16
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1	1	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 1
• Killed mutants in this change: 19

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1	1	2
:tada:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage	0	17

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[github-actions[bot]]

• Surviving mutants in this change: 1
• Killed mutants in this change: 19

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1	1	2
:tada:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage	0	17

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[Kamil-Gabaydullin]

• Surviving mutants in this change: 1
• Killed mutants in this change: 19

class surviving killed 🧟tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1 1 2 🎉tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage 0 17 Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

No idea if something can be done with this warning that return super.visitMethodInvocation(node, unused); can be replaced with return null; in TreeScanner and no tests would fail, I've replaced same calls with nulls in LexicographicalAnnotationAttributeListing TreeScanner implementation and no tests failed as well 😬

[github-actions[bot]]

• Surviving mutants in this change: 1
• Killed mutants in this change: 19

class	surviving	killed
:zombie:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage$1	1	2
:tada:tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage	0	17

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

[rickie]

Hi @Kamil-Gabaydullin, for a while there was no activity on this PR. Partly because @Stephan202 and I were swamped and reviewing PRs that contain a new BugPattern can take quite some time. Still, having to wait for a long time for reviews is not nice. This situation is far from ideal and we want to do better. Hence, we set up an (internal) review pool to have others help out with reviewing PRs. Concretely this means we want to continue where we left off and start reviewing this again.

Therefore, we'd like to discuss the state and direction of the PR. Stephan flagged in this comment that the current state could lead to semantic changes and therefore we need to change some things. There are some things we need to consider to determine how we want to continue with this PR.

There are a few options:

1. Update the PR to only merge adjacent calls of both Mockito#{reset,verifyNoInteractions}.
2. Make this PR more generic and merge any adjacent method invocations that can be merged. Another example would be the addAll, so it would focus on adjacent statements that can be merged.
3. Generify even further and also allow for chained method calls to be merged. This means that the functionality of #443 will also be part of this PR. As a result, we can solve the merging of two method invocations for once and for all :smile:.

So the question is, are you still up for applying changes to get it into a state where we can start reviewing again? If yes, which direction do you want to go in?

[Kamil-Gabaydullin]

Yep, I've been thinking for a while about this one after Stephan's comments. To me the best direction for this PR seems to be the third one you've described, since making it only about Mockito is clearly just small part of the problem when there is an apparent generalization that can be done.

For the implementation, I think we should run this BugPattern only for specific cases(i.e. explicitly specify Mockito.verifyNoInteractions, Mockito.verifyNoMoreInteractions, ImmutableList.add, ImmutableList.addAll etc.), so that we can ensure that there is a correct vararg alternative present and also type safety, and perhaps debugging is easier when the calls are chained for some calls - so we'd like to avoid squashing the calls to a vararg call(or readability in general). What's the best approach of specifying these calls - I have no idea, looks like this BugPattern would be the first of a kind 😅

This doesn't seem like a "good first issue" anymore though 😁 I'm okay to try implementing this (again), but I have my doubts 😅

[rickie]

What's the best approach of specifying these calls

We have a few BugPatterns where we have a curated list of calls that we want to match, see StaticImport or IdentityConversion. So that will not be a problem.

This doesn't seem like a "good first issue" anymore though :grin: I'm okay to try implementing this (again), but I have my doubts :sweat_smile:

Yeah I can imagine, this will not be an easy one. We could leave this one for now and maybe @Stephan202 or I will pick it up. We can definitely find you another BugPattern if you are interested, one that fits the category good first issue more than the third option mentioned for this pattern. Would you like that?

[Kamil-Gabaydullin]

Sounds good, I'll have a look around and pick something when I have the time. Should I close this PR (and the issue)? Or close this PR and make changes to the issue?