Prevent JUnitMethodDeclarationCheck from renaming likely-overridden…

… methods (#93)
PicnicSupermarket · May 17, 2022 · 8e57f64 · 8e57f64
1 parent 53e81f3
commit 8e57f64
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 6 deletions.
diff --git a/...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java
@@ -4,8 +4,11 @@
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
 import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
+import static com.google.errorprone.matchers.Matchers.allOf;
 import static com.google.errorprone.matchers.Matchers.annotations;
 import static com.google.errorprone.matchers.Matchers.anyOf;
+import static com.google.errorprone.matchers.Matchers.enclosingClass;
+import static com.google.errorprone.matchers.Matchers.hasModifier;
 import static com.google.errorprone.matchers.Matchers.isType;
 import static java.util.function.Predicate.not;
 import static tech.picnic.errorprone.bugpatterns.JavaKeywords.isReservedKeyword;
@@ -20,6 +23,7 @@
 import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.Matchers;
 import com.google.errorprone.matchers.MultiMatcher;
 import com.google.errorprone.predicates.TypePredicate;
 import com.google.errorprone.util.ASTHelpers;
@@ -51,8 +55,13 @@ public final class JUnitMethodDeclarationCheck extends BugChecker implements Met
   private static final String TEST_PREFIX = "test";
   private static final ImmutableSet<Modifier> ILLEGAL_MODIFIERS =
       ImmutableSet.of(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC);
-  private static final MultiMatcher<MethodTree, AnnotationTree> OVERRIDE_METHOD =
-      annotations(AT_LEAST_ONE, isType("java.lang.Override"));
+  private static final Matcher<MethodTree> HAS_UNMODIFIABLE_SIGNATURE =
+      anyOf(
+          annotations(AT_LEAST_ONE, isType("java.lang.Override")),
+          allOf(
+              Matchers.not(hasModifier(Modifier.FINAL)),
+              Matchers.not(hasModifier(Modifier.PRIVATE)),
+              enclosingClass(hasModifier(Modifier.ABSTRACT))));
   private static final MultiMatcher<MethodTree, AnnotationTree> TEST_METHOD =
       annotations(
           AT_LEAST_ONE,
@@ -70,9 +79,7 @@ public final class JUnitMethodDeclarationCheck extends BugChecker implements Met
 
   @Override
   public Description matchMethod(MethodTree tree, VisitorState state) {
-    // XXX: Perhaps we should also skip analysis of non-`private` non-`final` methods in abstract
-    // classes?
-    if (OVERRIDE_METHOD.matches(tree, state)) {
+    if (HAS_UNMODIFIABLE_SIGNATURE.matches(tree, state)) {
       return Description.NO_MATCH;
     }
 

diff --git a/...rib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java b/...rib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java
@@ -5,7 +5,7 @@
 import com.google.errorprone.CompilationTestHelper;
 import org.junit.jupiter.api.Test;
 
-public final class JUnitMethodDeclarationCheckTest {
+final class JUnitMethodDeclarationCheckTest {
   private final CompilationTestHelper compilationTestHelper =
       CompilationTestHelper.newInstance(JUnitMethodDeclarationCheck.class, getClass());
   private final BugCheckerRefactoringTestHelper refactoringTestHelper =
@@ -138,6 +138,21 @@ void identification() {
             "  @Override @Test void testArguments() {}",
             "  @Override @Test void testPublic() {}",
             "}")
+        .addSourceLines(
+            "C.java",
+            "import org.junit.jupiter.api.AfterAll;",
+            "import org.junit.jupiter.api.BeforeAll;",
+            "import org.junit.jupiter.api.Test;",
+            "",
+            "abstract class C {",
+            "  @BeforeAll public void setUp() {}",
+            "  @Test void testMethod1() {}",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  @AfterAll private void tearDown() {}",
+            "  // BUG: Diagnostic contains:",
+            "  @Test final void testMethod2() {}",
+            "}")
         .doTest();
   }