From 63d351c54addca063ed4749ec63e90e05b9f16c8 Mon Sep 17 00:00:00 2001 From: Kamil Gabaydullin Date: Mon, 30 Jan 2023 10:40:57 +0100 Subject: [PATCH] Issue#475 Review suggestions --- .../MockitoVerifyNoInteractionsUsage.java | 26 +- .../MockitoVerifyNoInteractionsUsageTest.java | 287 ++++++++++++++---- 2 files changed, 253 insertions(+), 60 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java index 60cae372eb..2e0fcfb0e8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsage.java @@ -3,6 +3,7 @@ import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static com.google.errorprone.matchers.Matchers.staticMethod; import static java.util.stream.Collectors.joining; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.TEST_METHOD; @@ -15,14 +16,16 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.util.TreeScanner; -import com.sun.tools.javac.code.Symbol; import java.util.List; import org.jspecify.annotations.Nullable; import org.mockito.Mockito; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** * A {@link BugChecker} that flags multiple usages of {@link Mockito#verifyNoInteractions} in favor @@ -42,14 +45,16 @@ public final class MockitoVerifyNoInteractionsUsage extends BugChecker implements MethodTreeMatcher { private static final long serialVersionUID = 1L; + private static final Matcher VERIFY_NO_INTERACTIONS = + staticMethod().onClass("org.mockito.Mockito").named("verifyNoInteractions"); @Override public Description matchMethod(MethodTree tree, VisitorState state) { - boolean isTestMethod = TEST_METHOD.matches(tree, state); - if (!isTestMethod) { + if (!TEST_METHOD.matches(tree, state)) { return Description.NO_MATCH; } - var verifyNoInteractionsInvocations = getVerifyNoInteractionsInvocations(tree); + ImmutableList verifyNoInteractionsInvocations = + getVerifyNoInteractionsInvocations(tree, state); if (verifyNoInteractionsInvocations.size() < 2) { return Description.NO_MATCH; } @@ -72,21 +77,26 @@ public Description matchMethod(MethodTree tree, VisitorState state) { ""); } }); - fixBuilder.replace(lastInvocation, "verifyNoInteractions(" + combinedArgument + ")"); + + String callAsString = SourceCode.treeToString(lastInvocation, state); + fixBuilder.replace( + lastInvocation, + callAsString.startsWith("Mockito.") + ? "Mockito.verifyNoInteractions(" + combinedArgument + ")" + : "verifyNoInteractions(" + combinedArgument + ")"); return describeMatch(tree, fixBuilder.build()); } private static ImmutableList getVerifyNoInteractionsInvocations( - MethodTree methodTree) { + MethodTree methodTree, VisitorState state) { ImmutableList.Builder invocationTreeBuilder = ImmutableList.builder(); new TreeScanner<@Nullable Void, @Nullable Void>() { @Override public @Nullable Void visitMethodInvocation( MethodInvocationTree node, @Nullable Void unused) { - Symbol symbol = ASTHelpers.getSymbol(node); - if (symbol.getSimpleName().toString().equals("verifyNoInteractions")) { + if (VERIFY_NO_INTERACTIONS.matches(node, state)) { invocationTreeBuilder.add(node); } return super.visitMethodInvocation(node, unused); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java index 7aaacd20b9..6f4df7da8b 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoVerifyNoInteractionsUsageTest.java @@ -15,8 +15,10 @@ void identification() { CompilationTestHelper.newInstance(MockitoVerifyNoInteractionsUsage.class, getClass()) .addSourceLines( "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", "import static org.mockito.Mockito.mock;", "import static org.mockito.Mockito.verifyNoInteractions;", + "import static org.mockito.Mockito.when;", "", "import org.junit.jupiter.api.Test;", "", @@ -25,43 +27,118 @@ void identification() { " void a() {}", " @Test", " void b() {", - " Runnable runnable = mock(Runnable.class);", - " verifyNoInteractions(runnable);", + " Object mock = mock(Object.class);", + " verifyNoInteractions(mock);", " }", " @Test", " void c() {", - " Runnable runnable = mock(Runnable.class);", - " Runnable runnable1 = mock(Runnable.class);", - " verifyNoInteractions(runnable, runnable1);", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock1, mock2);", " }", " @Test", " // BUG: Diagnostic contains:", " void d() {", - " Runnable runnable1 = mock(Runnable.class);", - " Runnable runnable2 = mock(Runnable.class);", - " verifyNoInteractions(runnable1);", - " verifyNoInteractions(runnable2);", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock1);", + " verifyNoInteractions(mock2);", " }", " @Test", " // BUG: Diagnostic contains:", " void e() {", - " Runnable runnable1 = mock(Runnable.class);", - " verifyNoInteractions(runnable1);", - " Runnable runnable2 = mock(Runnable.class);", - " verifyNoInteractions(runnable2);", + " Object mock1 = mock(Object.class);", + " verifyNoInteractions(mock1);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock2);", " }", " @Test", " // BUG: Diagnostic contains:", " void f() {", - " Runnable runnable1 = mock(Runnable.class);", - " verifyNoInteractions(runnable1);", - " Runnable runnable2 = mock(Runnable.class);", - " Runnable runnable3 = mock(Runnable.class);", - " verifyNoInteractions(runnable2, runnable3);", - " Runnable runnable4 = mock(Runnable.class);", - " Runnable runnable5 = mock(Runnable.class);", - " verifyNoInteractions(runnable4);", - " verifyNoInteractions(runnable5);", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " when(mock1.toString()).thenReturn(\"test\");", + " Object mock3 = mock(Object.class);", + " verifyNoInteractions(mock2, mock3);", + " Object mock4 = mock(Object.class);", + " String str = mock1.toString();", + " Object mock5 = mock(Object.class);", + " verifyNoInteractions(mock4);", + " assertThat(str).isEqualTo(\"test\");", + " verifyNoInteractions(mock5);", + " }", + "}") + .addSourceLines( + "B.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.mockito.Mockito.mock;", + "import static org.mockito.Mockito.when;", + "", + "import org.junit.jupiter.api.Test;", + "import org.mockito.Mockito;", + "", + "class B {", + " private static void verifyNoInteractions(Object... objects) {}", + "", + " @Test", + " void a() {", + " Object mock = mock(Object.class);", + " verifyNoInteractions(mock);", + " }", + " @Test", + " void b() {", + " Object mock = mock(Object.class);", + " Mockito.verifyNoInteractions(mock);", + " }", + " @Test", + " void c() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock1, mock2);", + " }", + " @Test", + " void d() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock1);", + " verifyNoInteractions(mock2);", + " }", + " @Test", + " void e() {", + " Object mock1 = mock(Object.class);", + " verifyNoInteractions(mock1);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock2);", + " }", + " @Test", + " void f() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock1);", + " Mockito.verifyNoInteractions(mock2);", + " }", + " @Test", + " // BUG: Diagnostic contains:", + " void g() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " Mockito.verifyNoInteractions(mock1);", + " Mockito.verifyNoInteractions(mock2);", + " }", + " @Test", + " // BUG: Diagnostic contains:", + " void h() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " when(mock1.toString()).thenReturn(\"test\");", + " Object mock3 = mock(Object.class);", + " Mockito.verifyNoInteractions(mock2, mock3);", + " Object mock4 = mock(Object.class);", + " String str = mock1.toString();", + " Object mock5 = mock(Object.class);", + " Mockito.verifyNoInteractions(mock4);", + " assertThat(str).isEqualTo(\"test\");", + " verifyNoInteractions(mock5);", " }", "}") .doTest(); @@ -80,10 +157,10 @@ void replaceSequentialCalls() { "class A {", " @Test", " void m() {", - " Runnable runnable1 = mock(Runnable.class);", - " Runnable runnable2 = mock(Runnable.class);", - " verifyNoInteractions(runnable1);", - " verifyNoInteractions(runnable2);", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock1);", + " verifyNoInteractions(mock2);", " }", "}") .addOutputLines( @@ -96,10 +173,10 @@ void replaceSequentialCalls() { "class A {", " @Test", " void m() {", - " Runnable runnable1 = mock(Runnable.class);", - " Runnable runnable2 = mock(Runnable.class);", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", "", - " verifyNoInteractions(runnable1, runnable2);", + " verifyNoInteractions(mock1, mock2);", " }", "}") .doTest(TestMode.TEXT_MATCH); @@ -118,10 +195,10 @@ void replaceNonSequentialCalls() { "class A {", " @Test", " void m() {", - " Runnable runnable1 = mock(Runnable.class);", - " verifyNoInteractions(runnable1);", - " Runnable runnable2 = mock(Runnable.class);", - " verifyNoInteractions(runnable2);", + " Object mock1 = mock(Object.class);", + " verifyNoInteractions(mock1);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock2);", " }", "}") .addOutputLines( @@ -134,58 +211,164 @@ void replaceNonSequentialCalls() { "class A {", " @Test", " void m() {", - " Runnable runnable1 = mock(Runnable.class);", + " Object mock1 = mock(Object.class);", "", - " Runnable runnable2 = mock(Runnable.class);", - " verifyNoInteractions(runnable1, runnable2);", + " Object mock2 = mock(Object.class);", + " verifyNoInteractions(mock1, mock2);", " }", "}") .doTest(TestMode.TEXT_MATCH); } @Test - void replaceManyCalls() { + void replaceComplexCalls() { refactoringTestHelper .addInputLines( "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", "import static org.mockito.Mockito.mock;", "import static org.mockito.Mockito.verifyNoInteractions;", + "import static org.mockito.Mockito.when;", "", "import org.junit.jupiter.api.Test;", "", "class A {", " @Test", " void m() {", - " Runnable runnable1 = mock(Runnable.class);", - " verifyNoInteractions(runnable1);", - " Runnable runnable2 = mock(Runnable.class);", - " Runnable runnable3 = mock(Runnable.class);", - " verifyNoInteractions(runnable2, runnable3);", - " Runnable runnable4 = mock(Runnable.class);", - " Runnable runnable5 = mock(Runnable.class);", - " verifyNoInteractions(runnable4);", - " verifyNoInteractions(runnable5);", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " when(mock1.toString()).thenReturn(\"test\");", + " Object mock3 = mock(Object.class);", + " verifyNoInteractions(mock2, mock3);", + " Object mock4 = mock(Object.class);", + " String str = mock1.toString();", + " Object mock5 = mock(Object.class);", + " verifyNoInteractions(mock4);", + " assertThat(str).isEqualTo(\"test\");", + " verifyNoInteractions(mock5);", " }", "}") .addOutputLines( "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", "import static org.mockito.Mockito.mock;", "import static org.mockito.Mockito.verifyNoInteractions;", + "import static org.mockito.Mockito.when;", "", "import org.junit.jupiter.api.Test;", "", "class A {", " @Test", " void m() {", - " Runnable runnable1 = mock(Runnable.class);", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " when(mock1.toString()).thenReturn(\"test\");", + " Object mock3 = mock(Object.class);", + "", + " Object mock4 = mock(Object.class);", + " String str = mock1.toString();", + " Object mock5 = mock(Object.class);", + "", + " assertThat(str).isEqualTo(\"test\");", + " verifyNoInteractions(mock2, mock3, mock4, mock5);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void replaceStaticCalls() { + refactoringTestHelper + .addInputLines( + "A.java", + "import static org.mockito.Mockito.mock;", "", - " Runnable runnable2 = mock(Runnable.class);", - " Runnable runnable3 = mock(Runnable.class);", + "import org.junit.jupiter.api.Test;", + "import org.mockito.Mockito;", "", - " Runnable runnable4 = mock(Runnable.class);", - " Runnable runnable5 = mock(Runnable.class);", + "class A {", + " @Test", + " void m() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " Mockito.verifyNoInteractions(mock1);", + " Mockito.verifyNoInteractions(mock2);", + " }", + "}") + .addOutputLines( + "A.java", + "import static org.mockito.Mockito.mock;", + "", + "import org.junit.jupiter.api.Test;", + "import org.mockito.Mockito;", + "", + "class A {", + " @Test", + " void m() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + "", + " Mockito.verifyNoInteractions(mock1, mock2);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void replaceComplexStaticCalls() { + refactoringTestHelper + .addInputLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.mockito.Mockito.mock;", + "import static org.mockito.Mockito.when;", + "", + "import org.junit.jupiter.api.Test;", + "import org.mockito.Mockito;", + "", + "class A {", + " private static void verifyNoInteractions(Object... objects) {}", + "", + " @Test", + " void m() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " when(mock1.toString()).thenReturn(\"test\");", + " Object mock3 = mock(Object.class);", + " Mockito.verifyNoInteractions(mock2, mock3);", + " Object mock4 = mock(Object.class);", + " String str = mock1.toString();", + " Object mock5 = mock(Object.class);", + " Mockito.verifyNoInteractions(mock4);", + " assertThat(str).isEqualTo(\"test\");", + " verifyNoInteractions(mock5);", + " }", + "}") + .addOutputLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.mockito.Mockito.mock;", + "import static org.mockito.Mockito.when;", + "", + "import org.junit.jupiter.api.Test;", + "import org.mockito.Mockito;", + "", + "class A {", + " private static void verifyNoInteractions(Object... objects) {}", + "", + " @Test", + " void m() {", + " Object mock1 = mock(Object.class);", + " Object mock2 = mock(Object.class);", + " when(mock1.toString()).thenReturn(\"test\");", + " Object mock3 = mock(Object.class);", "", - " verifyNoInteractions(runnable1, runnable2, runnable3, runnable4, runnable5);", + " Object mock4 = mock(Object.class);", + " String str = mock1.toString();", + " Object mock5 = mock(Object.class);", + " Mockito.verifyNoInteractions(mock2, mock3, mock4);", + " assertThat(str).isEqualTo(\"test\");", + " verifyNoInteractions(mock5);", " }", "}") .doTest(TestMode.TEXT_MATCH);