diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java new file mode 100644 index 0000000000..f2c26477a0 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java @@ -0,0 +1,114 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; +import static com.google.errorprone.matchers.Matchers.annotations; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.isType; +import static java.util.function.Predicate.not; + +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.MultiMatcher; +import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Symbol; +import java.util.Optional; +import javax.lang.model.element.Modifier; + +/** A {@link BugChecker} which flags non-canonical JUnit method declarations. */ +// XXX: Consider introducing a class-level check which enforces that test classes: +// 1. Are named `*Test` or `Abstract*TestCase`. +// 2. If not `abstract`, don't have public methods and subclasses. +// 3. Only have private fields. +// XXX: If implemented, the current logic could flag only `private` JUnit methods. +@AutoService(BugChecker.class) +@BugPattern( + name = "JUnitMethodDeclaration", + summary = "JUnit method declaration can likely be improved", + linkType = BugPattern.LinkType.NONE, + severity = BugPattern.SeverityLevel.SUGGESTION, + tags = BugPattern.StandardTags.SIMPLIFICATION) +public final class JUnitMethodDeclarationCheck extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final String TEST_PREFIX = "test"; + private static final ImmutableSet ILLEGAL_MODIFIERS = + ImmutableSet.of(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC); + private static final MultiMatcher IS_OVERRIDE_METHOD = + annotations(AT_LEAST_ONE, isType("java.lang.Override")); + private static final MultiMatcher IS_TEST_METHOD = + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.junit.jupiter.api.Test"), + hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"))); + private static final MultiMatcher IS_SETUP_OR_TEARDOWN_METHOD = + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.junit.jupiter.api.AfterAll"), + isType("org.junit.jupiter.api.AfterEach"), + isType("org.junit.jupiter.api.BeforeAll"), + isType("org.junit.jupiter.api.BeforeEach"))); + + @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 (IS_OVERRIDE_METHOD.matches(tree, state)) { + return Description.NO_MATCH; + } + + boolean isTestMethod = IS_TEST_METHOD.matches(tree, state); + if (!isTestMethod && !IS_SETUP_OR_TEARDOWN_METHOD.matches(tree, state)) { + return Description.NO_MATCH; + } + + SuggestedFix.Builder builder = SuggestedFix.builder(); + SuggestedFixes.removeModifiers(tree.getModifiers(), state, ILLEGAL_MODIFIERS) + .ifPresent(builder::merge); + + if (isTestMethod) { + // XXX: In theory this rename could clash with an existing method or static import. In that + // case we should emit a warning without a suggested replacement. + tryCanonicalizeMethodName(tree, state).ifPresent(builder::merge); + } + + return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build()); + } + + private static Optional tryCanonicalizeMethodName( + MethodTree tree, VisitorState state) { + return Optional.ofNullable(ASTHelpers.getSymbol(tree)) + .map(sym -> sym.getQualifiedName().toString()) + .filter(name -> name.startsWith(TEST_PREFIX)) + .map(name -> name.substring(TEST_PREFIX.length())) + .filter(not(String::isEmpty)) + .map(name -> Character.toLowerCase(name.charAt(0)) + name.substring(1)) + .filter(name -> !Character.isDigit(name.charAt(0))) + .map(name -> SuggestedFixes.renameMethod(tree, name, state)); + } + + // XXX: Move to a `MoreMatchers` utility class. + private static Matcher hasMetaAnnotation(String annotationClassName) { + TypePredicate typePredicate = hasAnnotation(annotationClassName); + return (tree, state) -> { + Symbol sym = ASTHelpers.getDeclaredSymbol(tree); + return sym != null && typePredicate.apply(sym.type, state); + }; + } + + // XXX: Move to a `MoreTypePredicates` utility class. + private static TypePredicate hasAnnotation(String annotationClassName) { + return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationClassName, state); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java new file mode 100644 index 0000000000..5b90f4f368 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java @@ -0,0 +1,180 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +public final class JUnitMethodDeclarationCheckTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(JUnitMethodDeclarationCheck.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(new JUnitMethodDeclarationCheck(), getClass()); + + @Test + public void testIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import org.junit.jupiter.api.AfterAll;", + "import org.junit.jupiter.api.AfterEach;", + "import org.junit.jupiter.api.BeforeAll;", + "import org.junit.jupiter.api.BeforeEach;", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "class A {", + " @BeforeAll void setUp1() {}", + " // BUG: Diagnostic contains:", + " @BeforeAll public void setUp2() {}", + " // BUG: Diagnostic contains:", + " @BeforeAll protected void setUp3() {}", + " // BUG: Diagnostic contains:", + " @BeforeAll private void setUp4() {}", + "", + " @BeforeEach void setup5() {}", + " // BUG: Diagnostic contains:", + " @BeforeEach public void setUp6() {}", + " // BUG: Diagnostic contains:", + " @BeforeEach protected void setUp7() {}", + " // BUG: Diagnostic contains:", + " @BeforeEach private void setUp8() {}", + "", + " @AfterEach void tearDown1() {}", + " // BUG: Diagnostic contains:", + " @AfterEach public void tearDown2() {}", + " // BUG: Diagnostic contains:", + " @AfterEach protected void tearDown3() {}", + " // BUG: Diagnostic contains:", + " @AfterEach private void tearDown4() {}", + "", + " @AfterAll void tearDown5() {}", + " // BUG: Diagnostic contains:", + " @AfterAll public void tearDown6() {}", + " // BUG: Diagnostic contains:", + " @AfterAll protected void tearDown7() {}", + " // BUG: Diagnostic contains:", + " @AfterAll private void tearDown8() {}", + "", + " @Test void method1() {}", + " // BUG: Diagnostic contains:", + " @Test void testMethod2() {}", + " // BUG: Diagnostic contains:", + " @Test public void method3() {}", + " // BUG: Diagnostic contains:", + " @Test protected void method4() {}", + " // BUG: Diagnostic contains:", + " @Test private void method5() {}", + "", + " @ParameterizedTest void method6() {}", + " // BUG: Diagnostic contains:", + " @ParameterizedTest void testMethod7() {}", + " // BUG: Diagnostic contains:", + " @ParameterizedTest public void method8() {}", + " // BUG: Diagnostic contains:", + " @ParameterizedTest protected void method9() {}", + " // BUG: Diagnostic contains:", + " @ParameterizedTest private void method10() {}", + "", + " @BeforeEach @BeforeAll @AfterEach @AfterAll void testNonTestMethod1() {}", + " public void testNonTestMethod2() {}", + " protected void testNonTestMethod3() {}", + " private void testNonTestMethod4() {}", + " @Test void test5() {}", + "}") + .addSourceLines( + "B.java", + "import org.junit.jupiter.api.AfterAll;", + "import org.junit.jupiter.api.AfterEach;", + "import org.junit.jupiter.api.BeforeAll;", + "import org.junit.jupiter.api.BeforeEach;", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "final class B extends A {", + " @Override @BeforeAll void setUp1() {}", + " @Override @BeforeAll public void setUp2() {}", + " @Override @BeforeAll protected void setUp3() {}", + "", + " @Override @BeforeEach void setup5() {}", + " @Override @BeforeEach public void setUp6() {}", + " @Override @BeforeEach protected void setUp7() {}", + "", + " @Override @AfterEach void tearDown1() {}", + " @Override @AfterEach public void tearDown2() {}", + " @Override @AfterEach protected void tearDown3() {}", + "", + " @Override @AfterAll void tearDown5() {}", + " @Override @AfterAll public void tearDown6() {}", + " @Override @AfterAll protected void tearDown7() {}", + "", + " @Override @Test void method1() {}", + " @Override @Test void testMethod2() {}", + " @Override @Test public void method3() {}", + " @Override @Test protected void method4() {}", + "", + " @Override @ParameterizedTest void method6() {}", + " @Override @ParameterizedTest void testMethod7() {}", + " @Override @ParameterizedTest public void method8() {}", + " @Override @ParameterizedTest protected void method9() {}", + "", + " @Override @BeforeEach @BeforeAll @AfterEach @AfterAll void testNonTestMethod1() {}", + " @Override public void testNonTestMethod2() {}", + " @Override protected void testNonTestMethod3() {}", + " @Override @Test void test5() {}", + "}") + .doTest(); + } + + @Test + public void testReplacement() { + refactoringTestHelper + .addInputLines( + "in/A.java", + "import org.junit.jupiter.api.AfterAll;", + "import org.junit.jupiter.api.AfterEach;", + "import org.junit.jupiter.api.BeforeAll;", + "import org.junit.jupiter.api.BeforeEach;", + "import org.junit.jupiter.api.RepeatedTest;", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "class A {", + " @BeforeAll public void setUp1() {}", + " @BeforeEach protected void setUp2() {}", + " @AfterEach private void setUp3() {}", + " @AfterAll private void setUp4() {}", + "", + " @Test void testFoo() {}", + " @ParameterizedTest void testBar() {}", + "", + " @Test public void baz() {}", + " @RepeatedTest(2) private void qux() {}", + " @ParameterizedTest protected void quux() {}", + "}") + .addOutputLines( + "out/A.java", + "import org.junit.jupiter.api.AfterAll;", + "import org.junit.jupiter.api.AfterEach;", + "import org.junit.jupiter.api.BeforeAll;", + "import org.junit.jupiter.api.BeforeEach;", + "import org.junit.jupiter.api.RepeatedTest;", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "class A {", + " @BeforeAll void setUp1() {}", + " @BeforeEach void setUp2() {}", + " @AfterEach void setUp3() {}", + " @AfterAll void setUp4() {}", + "", + " @Test void foo() {}", + " @ParameterizedTest void bar() {}", + "", + " @Test void baz() {}", + " @RepeatedTest(2) void qux() {}", + " @ParameterizedTest void quux() {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } +}