From 338ba3abd532f415ce7a4f1cb2786be56e076161 Mon Sep 17 00:00:00 2001 From: Gijs de Jong Date: Wed, 24 Aug 2022 09:47:58 +0200 Subject: [PATCH] Introduce `JUnitClassDeclaration` --- .../bugpatterns/JUnitClassModifiers.java | 103 ++++++++++++++ .../bugpatterns/JUnitClassModifiersTest.java | 133 ++++++++++++++++++ 2 files changed, 236 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiersTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java new file mode 100644 index 00000000000..585a20d23d0 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiers.java @@ -0,0 +1,103 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +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.hasMethod; +import static com.google.errorprone.matchers.Matchers.hasModifier; +import static com.google.errorprone.matchers.Matchers.isType; +import static com.google.errorprone.matchers.Matchers.not; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +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.ClassTreeMatcher; +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.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol; +import javax.lang.model.element.Modifier; + +/** + * A {@link BugChecker} that flags non-final and non package-private JUnit test class declarations. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "JUnit test classes should be declared as package private final", + linkType = CUSTOM, + link = BUG_PATTERNS_BASE_URL + "JUnitClassDeclaration", + severity = WARNING, + tags = SIMPLIFICATION) +public final class JUnitClassModifiers extends BugChecker implements ClassTreeMatcher { + private static final long serialVersionUID = 1L; + private static final MultiMatcher TEST_METHOD = + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.junit.jupiter.api.Test"), + hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"))); + private static final Matcher NON_FINAL_TEST_CLASS = + allOf( + not( + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.springframework.context.annotation.Configuration"), + hasMetaAnnotation("org.springframework.context.annotation.Configuration")))), + hasMethod(TEST_METHOD), + not(hasModifier(Modifier.ABSTRACT)), + anyOf( + not(hasModifier(Modifier.FINAL)), + hasModifier(Modifier.PRIVATE), + hasModifier(Modifier.PROTECTED), + hasModifier(Modifier.PUBLIC))); + + /** Instantiates a new {@link JUnitClassModifiers} instance. */ + public JUnitClassModifiers() {} + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + if (!NON_FINAL_TEST_CLASS.matches(tree, state)) { + return Description.NO_MATCH; + } + + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + SuggestedFixes.addModifiers(tree, state, Modifier.FINAL).ifPresent(fixBuilder::merge); + SuggestedFixes.removeModifiers( + tree.getModifiers(), + state, + ImmutableSet.of(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC)) + .ifPresent(fixBuilder::merge); + + return describeMatch(tree, fixBuilder.build()); + } + + // XXX: Move to a `MoreMatchers` utility class. + private static Matcher hasMetaAnnotation(String annotationClassName) { + TypePredicate typePredicate = hasAnnotation(annotationClassName); + return (tree, state) -> { + Symbol sym = ASTHelpers.getSymbol(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/JUnitClassModifiersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiersTest.java new file mode 100644 index 00000000000..cdb0284a2a3 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitClassModifiersTest.java @@ -0,0 +1,133 @@ +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; + +final class JUnitClassModifiersTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(JUnitClassModifiers.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(JUnitClassModifiers.class, getClass()); + + @Test + void identification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "", + "// BUG: Diagnostic contains:", + "class A {", + " @Test", + " void foo() {}", + "}") + .addSourceLines( + "B.java", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "// BUG: Diagnostic contains:", + "class B {", + " @ParameterizedTest", + " void foo() {}", + "}") + .addSourceLines( + "C.java", + "import org.junit.jupiter.api.Test;", + "", + "// BUG: Diagnostic contains:", + "public class C {", + " @Test", + " void foo() {}", + "}") + .addSourceLines( + "D.java", + "import org.junit.jupiter.api.Nested;", + "import org.junit.jupiter.api.Test;", + "", + "class D {", + " @Nested", + " // BUG: Diagnostic contains:", + " class Nested1 {", + " @Test", + " void foo() {}", + " }", + "", + " // BUG: Diagnostic contains:", + " static class Nested2 {", + " @Test", + " void bar() {}", + " }", + "}") + .addSourceLines( + "E.java", + "import org.junit.jupiter.api.Test;", + "", + "final class E {", + " @Test", + " void foo() {}", + "}") + .addSourceLines( + "F.java", + "import org.junit.jupiter.api.Test;", + "import org.springframework.context.annotation.Configuration;", + "", + "@Configuration", + "public class F {", + " @Test", + " void foo() {}", + "}") + .addSourceLines( + "G.java", + "import org.junit.jupiter.api.Test;", + "import org.springframework.boot.test.context.TestConfiguration;", + "", + "@TestConfiguration", + "public class G {", + " @Test", + " void foo() {}", + "}") + .addSourceLines( + "H.java", + "import org.junit.jupiter.api.Test;", + "", + "public abstract class H {", + " @Test", + " abstract void foo();", + "}") + .doTest(); + } + + @Test + void replacement() { + refactoringTestHelper + .addInputLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "", + "public class A {", + " @Test", + " void foo() {}", + "", + " private static class B {", + " @Test", + " void bar() {}", + " }", + "}") + .addOutputLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "", + "final class A {", + " @Test", + " void foo() {}", + "", + " static final class B {", + " @Test", + " void bar() {}", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +}