From 86f7f2a256cc3f3fb7f88de9ee969652b217460b Mon Sep 17 00:00:00 2001 From: anicolasgar Date: Thu, 24 Dec 2020 13:54:57 -0300 Subject: [PATCH] PSM-671 Extend checker to also flag illegal modifiers in Junit method declarations --- .../JunitMethodDeclarationCheck.java | 99 +++++++++ .../bugpatterns/TestMethodNameCheck.java | 60 ------ .../JunitMethodDeclarationCheckTest.java | 204 ++++++++++++++++++ .../bugpatterns/TestMethodNameCheckTest.java | 89 -------- 4 files changed, 303 insertions(+), 149 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheck.java delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheck.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheckTest.java delete mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheckTest.java 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 00000000000..9c48bd26ad1 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheck.java @@ -0,0 +1,99 @@ +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.isType; + +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.MultiMatcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import java.util.Optional; +import javax.lang.model.element.Modifier; + +/** A {@link BugChecker} which flags improvements in Junit method declarations. */ +@AutoService(BugChecker.class) +@BugPattern( + name = "JunitMethodDeclarationCheck", + summary = "Junit method declaration can likely be improved", + linkType = BugPattern.LinkType.NONE, + severity = BugPattern.SeverityLevel.WARNING, + 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 OVERRIDE_ANNOTATION = + annotations(AT_LEAST_ONE, isType("java.lang.Override")); + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + MethodSymbol sym = ASTHelpers.getSymbol(tree); + + if (sym == null + || !OVERRIDE_ANNOTATION.multiMatchResult(tree, state).matchingNodes().isEmpty() + || !ASTHelpers.isJUnitTestCode(state) + || (!hasTestPrefix(sym.getQualifiedName().toString()) && !hasIllegalModifiers(tree))) { + return Description.NO_MATCH; + } + + SuggestedFix.Builder builder = SuggestedFix.builder(); + /* Remove all illegal modifiers, if has. */ + SuggestedFixes.removeModifiers(tree, state, ILLEGAL_MODIFIERS.toArray(Modifier[]::new)) + .ifPresent(builder::merge); + /* Remove the 'test' prefix, if possible. */ + renameMethod(tree, state, sym.getQualifiedName().toString()).ifPresent(builder::merge); + + return describeMatch(tree, builder.build()); + } + + /** Renames the method removing the 'test' prefix, if possible. */ + private static Optional renameMethod( + MethodTree tree, VisitorState state, String methodName) { + if (hasTestPrefix(methodName)) { + String newMethodName = removeTestPrefix(methodName); + + return isValidMethodName(newMethodName) + ? Optional.of(SuggestedFixes.renameMethod(tree, newMethodName, state)) + : Optional.empty(); + } + + return Optional.empty(); + } + + /** Removes the 'test' prefix from the method name, if possible. */ + private static String removeTestPrefix(String methodName) { + StringBuilder sb = new StringBuilder(methodName); + sb.delete(0, 4); + sb.setCharAt(0, Character.toLowerCase(sb.charAt(0))); + + return sb.toString(); + } + + /** Determines whether the provided method name is valid. */ + private static boolean isValidMethodName(String methodName) { + return !methodName.isEmpty() && !Character.isDigit(methodName.charAt(0)); + } + + /** Determines whether the provided method name starts with the word 'test'. */ + private static boolean hasTestPrefix(String methodName) { + return methodName.length() > 4 && methodName.startsWith(TEST_PREFIX); + } + + /** Determines whether the method tree has any modifier that is not allowed. */ + private static boolean hasIllegalModifiers(MethodTree tree) { + return ASTHelpers.getModifiers(tree).getFlags().stream().anyMatch(ILLEGAL_MODIFIERS::contains); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheck.java deleted file mode 100644 index e969eb8dbe2..00000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheck.java +++ /dev/null @@ -1,60 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import static com.google.errorprone.fixes.SuggestedFixes.renameMethod; - -import com.google.auto.service.AutoService; -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.matchers.Description; -import com.google.errorprone.util.ASTHelpers; -import com.sun.source.tree.MethodTree; -import com.sun.tools.javac.code.Symbol.MethodSymbol; -import com.sun.tools.javac.util.Name; - -/** A {@link BugChecker} which flags redundant 'test' prefixes in test method names. */ -@AutoService(BugChecker.class) -@BugPattern( - name = "TestMethodNameCheck", - summary = "Method names starting with a 'test' prefix can likely be removed", - linkType = BugPattern.LinkType.NONE, - severity = BugPattern.SeverityLevel.WARNING, - tags = BugPattern.StandardTags.SIMPLIFICATION) -public final class TestMethodNameCheck extends BugChecker implements MethodTreeMatcher { - private static final long serialVersionUID = 1L; - private static final String TEST_PREFIX = "test"; - - @Override - public Description matchMethod(MethodTree tree, VisitorState state) { - MethodSymbol sym = ASTHelpers.getSymbol(tree); - if (sym == null - || !ASTHelpers.isJUnitTestCode(state) - || !hasTestPrefix(sym.getQualifiedName())) { - return Description.NO_MATCH; - } - - String originalMethodName = sym.getQualifiedName().toString(); - String newMethodName = removeTestPrefix(originalMethodName); - - /* The new method name is not valid */ - if (Character.isDigit(newMethodName.charAt(0))) { - return Description.NO_MATCH; - } - return describeMatch(tree, renameMethod(tree, newMethodName, state)); - } - - /** Determines whether the provided method name starts with the word 'test'. */ - private static boolean hasTestPrefix(Name methodName) { - return methodName.length() > 4 && methodName.toString().startsWith(TEST_PREFIX); - } - - /** Removes the 'test' prefix from the method name. */ - private static String removeTestPrefix(String methodName) { - StringBuilder sb = new StringBuilder(methodName); - sb.delete(0, 4); - sb.setCharAt(0, Character.toLowerCase(sb.charAt(0))); - - return sb.toString(); - } -} 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 00000000000..899c199e8e1 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheckTest.java @@ -0,0 +1,204 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +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.Test;", + "", + "final class A {", + " @Test void foo() {}", + " @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() {}", + "", + " // BUG: Diagnostic contains:", + " @Test private void method6() { ", + " new Foo() {", + " @Override", + " public void bar() {}", + " };", + " }", + "", + " // BUG: Diagnostic contains:", + " @Test void testMethod7() { ", + " new Foo() {", + " @Override", + " public void bar() {}", + " };", + " }", + "", + "", + " @Test void method8() { ", + " new Foo() {", + " @Override", + " public void bar() {}", + " };", + " }", + "", + "}", + "", + "abstract class Foo { ", + " abstract public void bar();", + "}") + .addSourceLines( + "B.java", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "final class B {", + " @ParameterizedTest void method1() {}", + "", + " // BUG: Diagnostic contains:", + " @ParameterizedTest void testMethod2() {}", + "", + "}") + .doTest(); + } + + @Test + public void testReplaceTestPrefix() { + refactoringTestHelper + .addInputLines( + "in/Container.java", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "interface Container {", + " class A {", + " @Test void test() {}", + " @Test void testMethod1() {}", + " @Test void test2() {}", + " @Test void method3() {}", + " void method4() {}", + " }", + "", + " class B {", + " @ParameterizedTest void test() {}", + " @ParameterizedTest void testMethod1() {}", + " @ParameterizedTest void test2() {}", + " @ParameterizedTest void method3() {}", + " void method4() {}", + " }", + "}") + .addOutputLines( + "out/Container.java", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "interface Container {", + " class A {", + " @Test void test() {}", + " @Test void method1() {}", + " @Test void test2() {}", + " @Test void method3() {}", + " void method4() {}", + " }", + "", + " class B {", + " @ParameterizedTest void test() {}", + " @ParameterizedTest void method1() {}", + " @ParameterizedTest void test2() {}", + " @ParameterizedTest void method3() {}", + " void method4() {}", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void testDropModifiers() { + refactoringTestHelper + .addInputLines( + "in/Container.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;", + "interface Container {", + " class A {", + " @BeforeEach public void setUp1() {}", + " @BeforeEach protected void setUp2() {}", + " @BeforeEach private void setUp3() {}", + " @BeforeEach void setUp4() {}", + " @BeforeAll public void setUp5() {}", + " @BeforeAll protected void setUp6() {}", + " @BeforeAll private void setUp7() {}", + " @BeforeAll void setUp8() {}", + " @AfterEach public void tearDown1() {}", + " @AfterEach protected void tearDown2() {}", + " @AfterEach private void tearDown3() {}", + " @AfterEach void tearDown4() {}", + " @AfterAll public void tearDown5() {}", + " @AfterAll protected void tearDown6() {}", + " @AfterAll private void tearDown7() {}", + " @AfterAll void tearDown8() {}", + " @Test void method1() {}", + " @Test public void method2() {}", + " @Test protected void method3() {}", + " @Test private void method4() {}", + " void method5() {}", + " public void method6() {}", + " protected void method7() {}", + " private void method8() {}", + " }", + "}") + .addOutputLines( + "out/Container.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;", + "interface Container {", + " class A {", + " @BeforeEach void setUp1() {}", + " @BeforeEach void setUp2() {}", + " @BeforeEach void setUp3() {}", + " @BeforeEach void setUp4() {}", + " @BeforeAll void setUp5() {}", + " @BeforeAll void setUp6() {}", + " @BeforeAll void setUp7() {}", + " @BeforeAll void setUp8() {}", + " @AfterEach void tearDown1() {}", + " @AfterEach void tearDown2() {}", + " @AfterEach void tearDown3() {}", + " @AfterEach void tearDown4() {}", + " @AfterAll void tearDown5() {}", + " @AfterAll void tearDown6() {}", + " @AfterAll void tearDown7() {}", + " @AfterAll void tearDown8() {}", + " @Test void method1() {}", + " @Test void method2() {}", + " @Test void method3() {}", + " @Test void method4() {}", + " void method5() {}", + " public void method6() {}", + " protected void method7() {}", + " private void method8() {}", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheckTest.java deleted file mode 100644 index 7950f829c9b..00000000000 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheckTest.java +++ /dev/null @@ -1,89 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import com.google.errorprone.BugCheckerRefactoringTestHelper; -import com.google.errorprone.CompilationTestHelper; -import org.junit.jupiter.api.Test; - -public final class TestMethodNameCheckTest { - private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(TestMethodNameCheck.class, getClass()); - private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance(new TestMethodNameCheck(), getClass()); - - @Test - public void testIdentification() { - compilationTestHelper - .addSourceLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "final class A {", - " @Test void method1() {}", - "", - " // BUG: Diagnostic contains:", - " @Test void testMethod2() {}", - "}") - .addSourceLines( - "B.java", - "import org.junit.jupiter.params.ParameterizedTest;", - "", - "final class B {", - " @ParameterizedTest void method3() {}", - "", - " // BUG: Diagnostic contains:", - " @ParameterizedTest void testMethod4() {}", - "", - "}") - .doTest(); - } - - @Test - public void testReplacement() { - refactoringTestHelper - .addInputLines( - "in/Container.java", - "import org.junit.jupiter.api.Test;", - "import org.junit.jupiter.params.ParameterizedTest;", - "", - "interface Container {", - " class A {", - " @Test void test() {}", - " @Test void testMethod1() {}", - " @Test void test2() {}", - " @Test void method3() {}", - " void method4() {}", - " }", - "", - " class B {", - " @ParameterizedTest void test() {}", - " @ParameterizedTest void testMethod1() {}", - " @ParameterizedTest void test2() {}", - " @ParameterizedTest void method3() {}", - " void method4() {}", - " }", - "}") - .addOutputLines( - "out/Container.java", - "import org.junit.jupiter.api.Test;", - "import org.junit.jupiter.params.ParameterizedTest;", - "", - "interface Container {", - " class A {", - " @Test void test() {}", - " @Test void method1() {}", - " @Test void test2() {}", - " @Test void method3() {}", - " void method4() {}", - " }", - "", - " class B {", - " @ParameterizedTest void test() {}", - " @ParameterizedTest void method1() {}", - " @ParameterizedTest void test2() {}", - " @ParameterizedTest void method3() {}", - " void method4() {}", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } -}