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

Kamil-Gabaydullin · 2023-01-25T15:17:19Z

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.

Kamil-Gabaydullin · 2023-01-25T15:18:03Z

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java

+      return Description.NO_MATCH;
+    }
+    var verifyNoInteractionsInvocations = getVerifyNoInteractionsInvocations(tree);
+    if (verifyNoInteractionsInvocations.size() < 2) {


If there is only one call to verifyNoInteractions() we don't want to do anything too

Kamil-Gabaydullin · 2023-01-25T15:19:22Z

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java

+            fixBuilder.replace(
+                ASTHelpers.getStartPosition(invocationTree),
+                state.getEndPosition(invocationTree) + 1,
+                "");


This replacement leaves an empty line behind, I couldn't figure out how to collapse lines 😞 or perhaps it should be taken care of by formatting and it's not a big deal?

github-actions · 2023-01-25T15:19:55Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage`	2	13
🧟`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.

Kamil-Gabaydullin · 2023-01-25T15:22:12Z

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java

+                "");
+          }
+        });
+    fixBuilder.replace(lastInvocation, "verifyNoInteractions(" + combinedArgument + ")");


I've decided that adding it as a last line in a test might not be preferable and thus replacing the last invocation is better, since it preserved intended order and place of this verification.

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java

Kamil-Gabaydullin · 2023-01-25T15:25:35Z

...b/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java

+            "  @Test",
+            "  void m() {",
+            "    Runnable runnable1 = mock(Runnable.class);",
+            "",


To the point that there are empty lines left behind 😞

Kamil-Gabaydullin · 2023-01-25T15:26:17Z

...b/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java

+            "    Runnable runnable3 = mock(Runnable.class);",
+            "    verifyNoInteractions(runnable2, runnable3);",
+            "    Runnable runnable4 = mock(Runnable.class);",
+            "    Runnable runnable5 = mock(Runnable.class);",


Perhaps there is a less verbose way to test this 🤔

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java

rickie

Hey @Kamil-Gabaydullin , thanks for opening the PR! Looks cool 🚀 !

I left a few suggestions on the PR 😄. I have to do some expectation management. The are a lot of PRs in review so I'm not sure when one of us gets around to doing a in depth review...
We'll keep you up to date for sure.

...b/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java

github-actions · 2023-01-30T09:43:30Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage`	1	16
🧟`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 · 2023-01-30T10:09:04Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage`	1	16
🧟`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 · 2023-01-30T10:23:02Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MockitoVerifyNoInteractionsUsage`	1	16
🧟`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 · 2023-01-30T10:40:22Z

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.

github-actions · 2023-01-30T11:51:32Z

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.

Kamil-Gabaydullin · 2023-01-30T11:57:42Z

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 · 2023-03-15T13:30:39Z

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.

Stephan202

Thanks for contributing to Error Prone Support, @Kamil-Gabaydullin!

I haven't deeply looked into the PR yet, but I'm seeing parallels with #443: in both cases we want to merge adjacent method calls. Other examples might include e.g. addAll builder methods. So I'm thinking that perhaps a more generic check is hiding in this code 👀

Stephan202 · 2023-03-17T13:40:58Z

...b/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java

+            "    Mockito.verifyNoInteractions(mock2, mock3);",
+            "    Object mock4 = mock(Object.class);",
+            "    String str = mock1.toString();",
+            "    Object mock5 = mock(Object.class);",
+            "    Mockito.verifyNoInteractions(mock4);",


In general, collapsing two non-adjacent verifyNoInteractions calls changes the semantics of the code, as the deferred verification could cause a test failure.

Stephan202 · 2023-03-17T14:08:40Z

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java

+        verifyNoInteractionsInvocations.stream()
+            .map(MethodInvocationTree::getArguments)
+            .flatMap(List::stream)
+            .map(Object::toString)


👀 looks like we're converting a Tree to String here. For that we should use SourceCode.treeToString.

rickie · 2023-05-01T11:14:42Z

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:

Update the PR to only merge adjacent calls of both Mockito#{reset,verifyNoInteractions}.
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.
Generify even further and also allow for chained method calls to be merged. This means that the functionality of Introduce BugPattern for removing duplicate StepVerifier#expectNext calls #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 😄.

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 · 2023-05-02T13:23:57Z

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 · 2023-05-04T14:29:48Z

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 😁 I'm okay to try implementing this (again), but I have my doubts 😅

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 · 2023-05-10T08:54:40Z

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?

Kamil-Gabaydullin commented Jan 25, 2023

View reviewed changes

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java Outdated Show resolved Hide resolved

Kamil-Gabaydullin commented Jan 25, 2023

View reviewed changes

...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java Show resolved Hide resolved

Kamil-Gabaydullin mentioned this pull request Jan 25, 2023

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

Open

4 tasks

Kamil-Gabaydullin linked an issue Jan 25, 2023 that may be closed by this pull request

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

Open

4 tasks

rickie requested changes Jan 25, 2023

View reviewed changes

Kamil-Gabaydullin marked this pull request as ready for review January 30, 2023 11:48

Kamil-Gabaydullin force-pushed the Kamil-Gabaydullin/verify-no-interactions branch from a17bfff to b5f99d5 Compare January 30, 2023 11:48

Kamil-Gabaydullin added 5 commits March 15, 2023 14:27

Issue#475 Initial implementation

26a04b9

Issue#475 Review suggestions

63d351c

Issue#475 Added a default constructor

88cabab

Issue#475 Formatting

83da373

Issue#475 Non @test method test

301ec17

Kamil-Gabaydullin force-pushed the Kamil-Gabaydullin/verify-no-interactions branch from b5f99d5 to 301ec17 Compare March 15, 2023 13:28

Kamil-Gabaydullin requested a review from rickie March 15, 2023 13:49

Stephan202 reviewed Mar 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Kamil-Gabaydullin commented Jan 25, 2023

Kamil-Gabaydullin Jan 25, 2023

Kamil-Gabaydullin Jan 25, 2023

github-actions bot commented Jan 25, 2023

Kamil-Gabaydullin Jan 25, 2023

Kamil-Gabaydullin Jan 25, 2023

Kamil-Gabaydullin Jan 25, 2023

rickie left a comment

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

Kamil-Gabaydullin commented Jan 30, 2023

github-actions bot commented Mar 15, 2023

Stephan202 left a comment

Stephan202 Mar 17, 2023

Stephan202 Mar 17, 2023

rickie commented May 1, 2023

Kamil-Gabaydullin commented May 2, 2023 •

edited

rickie commented May 4, 2023

Kamil-Gabaydullin commented May 10, 2023

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

Are you sure you want to change the base?

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

Conversation

Kamil-Gabaydullin commented Jan 25, 2023

Kamil-Gabaydullin Jan 25, 2023

Choose a reason for hiding this comment

Kamil-Gabaydullin Jan 25, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2023

Kamil-Gabaydullin Jan 25, 2023

Choose a reason for hiding this comment

Kamil-Gabaydullin Jan 25, 2023

Choose a reason for hiding this comment

Kamil-Gabaydullin Jan 25, 2023

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

github-actions bot commented Jan 30, 2023

Kamil-Gabaydullin commented Jan 30, 2023

github-actions bot commented Mar 15, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Mar 17, 2023

Choose a reason for hiding this comment

Stephan202 Mar 17, 2023

Choose a reason for hiding this comment

rickie commented May 1, 2023

Kamil-Gabaydullin commented May 2, 2023 • edited

rickie commented May 4, 2023

Kamil-Gabaydullin commented May 10, 2023

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

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

Kamil-Gabaydullin commented May 2, 2023 •

edited