From 710576d4feab65644859f7ef21d989074a6eeb64 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 --- ....java => JunitMethodDeclarationCheck.java} | 36 +++- .../JunitMethodDeclarationCheckTest.java | 174 ++++++++++++++++++ .../bugpatterns/TestMethodNameCheckTest.java | 89 --------- 3 files changed, 201 insertions(+), 98 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{TestMethodNameCheck.java => JunitMethodDeclarationCheck.java} (55%) 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/TestMethodNameCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheck.java similarity index 55% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheck.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheck.java index e969eb8dbe2..bb3f31f3bc8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/TestMethodNameCheck.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheck.java @@ -3,45 +3,58 @@ import static com.google.errorprone.fixes.SuggestedFixes.renameMethod; 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.util.ASTHelpers; import com.sun.source.tree.MethodTree; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.util.Name; +import javax.lang.model.element.Modifier; -/** A {@link BugChecker} which flags redundant 'test' prefixes in test method names. */ +/** A {@link BugChecker} which flags improvements in Junit method declarations. */ @AutoService(BugChecker.class) @BugPattern( - name = "TestMethodNameCheck", - summary = "Method names starting with a 'test' prefix can likely be removed", + 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 TestMethodNameCheck extends BugChecker implements MethodTreeMatcher { +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); @Override public Description matchMethod(MethodTree tree, VisitorState state) { MethodSymbol sym = ASTHelpers.getSymbol(tree); + if (sym == null || !ASTHelpers.isJUnitTestCode(state) - || !hasTestPrefix(sym.getQualifiedName())) { + || (!hasTestPrefix(sym.getQualifiedName()) && !hasIllegalModifiers(tree))) { return Description.NO_MATCH; } String originalMethodName = sym.getQualifiedName().toString(); String newMethodName = removeTestPrefix(originalMethodName); + SuggestedFix.Builder builder = SuggestedFix.builder(); - /* The new method name is not valid */ - if (Character.isDigit(newMethodName.charAt(0))) { - return Description.NO_MATCH; + /* Remove all illegal modifiers, if has. */ + SuggestedFixes.removeModifiers(tree, state, ILLEGAL_MODIFIERS.toArray(Modifier[]::new)) + .ifPresent(builder::merge); + + /* The new method name is valid. */ + if (hasTestPrefix(sym.getQualifiedName()) && !Character.isDigit(newMethodName.charAt(0))) { + builder.merge(renameMethod(tree, newMethodName, state)); } - return describeMatch(tree, renameMethod(tree, newMethodName, state)); + + return describeMatch(tree, builder.build()); } /** Determines whether the provided method name starts with the word 'test'. */ @@ -57,4 +70,9 @@ private static String removeTestPrefix(String methodName) { return sb.toString(); } + + /** 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/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..6dd2deefdea --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheckTest.java @@ -0,0 +1,174 @@ +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 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() {}", + "}") + .addSourceLines( + "B.java", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "final class B {", + " @ParameterizedTest void method6() {}", + "", + " // BUG: Diagnostic contains:", + " @ParameterizedTest void testMethod7() {}", + "", + "}") + .doTest(); + } + + @Test + public void testReplacementTestPrefix() { + 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); - } -}