From c27c0ffab6241500d0186fc5c27181eab093eabb Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 10 Apr 2022 09:07:15 +0200 Subject: [PATCH] Suggestions --- .../JUnitMethodDeclarationCheck.java | 95 ++++++----- .../JUnitMethodDeclarationCheckTest.java | 157 ++++-------------- 2 files changed, 84 insertions(+), 168 deletions(-) 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 index ea1bda6c78..9a8c598b0c 100644 --- 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 @@ -25,12 +25,10 @@ import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; -import com.sun.source.tree.CompilationUnitTree; import com.sun.source.tree.ImportTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol; -import java.util.Objects; import java.util.Optional; import javax.lang.model.element.Modifier; import javax.lang.model.element.Name; @@ -83,50 +81,72 @@ public Description matchMethod(MethodTree tree, VisitorState state) { return Description.NO_MATCH; } - SuggestedFix.Builder builder = SuggestedFix.builder(); + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); SuggestedFixes.removeModifiers(tree.getModifiers(), state, ILLEGAL_MODIFIERS) - .ifPresent(builder::merge); + .ifPresent(fixBuilder::merge); if (isTestMethod) { - tryCanonicalizeMethodName(tree) - .filter(methodName -> isValidMethodName(tree, methodName, state)) - .ifPresent( - methodName -> builder.merge(SuggestedFixes.renameMethod(tree, methodName, state))); + suggestTestMethodRenameIfApplicable(tree, fixBuilder, state); } - return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build()); + + return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); + } + + private void suggestTestMethodRenameIfApplicable( + MethodTree tree, SuggestedFix.Builder fixBuilder, VisitorState state) { + tryCanonicalizeMethodName(tree) + .ifPresent( + newName -> + findMethodRenameBlocker(newName, state) + .ifPresentOrElse( + blocker -> reportMethodRenameBlocker(tree, blocker, state), + () -> fixBuilder.merge(SuggestedFixes.renameMethod(tree, newName, state)))); } - private boolean isValidMethodName(MethodTree tree, String methodName, VisitorState state) { - if (isMethodNameInClass(methodName, state)) { - reportIncorrectMethodName( - methodName, tree, "A method with name %s already exists in the class.", state); - return false; + private void reportMethodRenameBlocker(MethodTree tree, String reason, VisitorState state) { + state.reportMatch( + buildDescription(tree) + .setMessage( + String.format( + "This method's name should not redundantly start with `%s` (but note that %s)", + TEST_PREFIX, reason)) + .build()); + } + + /** + * If applicable, returns a human-readable argument against assigning the given name to an + * existing method. + * + *

This method implements imperfect heuristics. Things it currently does not consider include + * the following: + * + *

+ */ + private static Optional findMethodRenameBlocker(String methodName, VisitorState state) { + if (isMethodInEnclosingClass(methodName, state)) { + return Optional.of( + String.format("a method named `%s` already exists in this class", methodName)); } - if (isMethodNameStaticallyImported(methodName, state)) { - reportIncorrectMethodName( - methodName, tree, "A method with name %s is already statically imported.", state); - return false; + if (isSimpleNameStaticallyImported(methodName, state)) { + return Optional.of(String.format("`%s` is already statically imported", methodName)); } if (isReservedKeyword(methodName)) { - reportIncorrectMethodName( - methodName, - tree, - "Method name `%s` is not possible because it is a Java keyword.", - state); - return false; + return Optional.of(String.format("`%s` is a reserved keyword", methodName)); } - return true; - } - private void reportIncorrectMethodName( - String methodName, MethodTree tree, String message, VisitorState state) { - state.reportMatch( - buildDescription(tree).setMessage(String.format(message, methodName)).build()); + return Optional.empty(); } - private static boolean isMethodNameInClass(String methodName, VisitorState state) { + private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) { return state.findEnclosing(ClassTree.class).getMembers().stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) @@ -136,18 +156,15 @@ private static boolean isMethodNameInClass(String methodName, VisitorState state .anyMatch(methodName::equals); } - private static boolean isMethodNameStaticallyImported(String methodName, VisitorState state) { - CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit(); - - return compilationUnit.getImports().stream() - .filter(Objects::nonNull) + private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) { + return state.getPath().getCompilationUnit().getImports().stream() .filter(ImportTree::isStatic) .map(ImportTree::getQualifiedIdentifier) - .map(tree -> getStaticImportIdentifier(tree, state)) - .anyMatch(methodName::contentEquals); + .map(tree -> getStaticImportSimpleName(tree, state)) + .anyMatch(simpleName::contentEquals); } - private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) { + private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) { String source = Util.treeToString(tree, state); return source.subSequence(source.lastIndexOf('.') + 1, source.length()); } 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 index 9c25eb0d2e..9e26bb8a44 100644 --- 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 @@ -16,6 +16,8 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", "import org.junit.jupiter.api.AfterAll;", "import org.junit.jupiter.api.AfterEach;", "import org.junit.jupiter.api.BeforeAll;", @@ -81,6 +83,14 @@ void identification() { " protected void testNonTestMethod3() {}", " private void testNonTestMethod4() {}", " @Test void test5() {}", + "", + " // BUG: Diagnostic contains: (but note that a method named `overload` already exists in this class)", + " @Test void testOverload() {}", + " void overload() {}", + " // BUG: Diagnostic contains: (but note that `arguments` is already statically imported)", + " @Test void testArguments() {}", + " // BUG: Diagnostic contains: (but note that `public` is a reserved keyword)", + " @Test void testPublic() {}", "}") .addSourceLines( "B.java", @@ -122,6 +132,11 @@ void identification() { " @Override public void testNonTestMethod2() {}", " @Override protected void testNonTestMethod3() {}", " @Override @Test void test5() {}", + "", + " @Override @Test void testOverload() {}", + " @Override void overload() {}", + " @Override @Test void testArguments() {}", + " @Override @Test void testPublic() {}", "}") .doTest(); } @@ -131,6 +146,8 @@ void replacement() { refactoringTestHelper .addInputLines( "in/A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", "import org.junit.jupiter.api.AfterAll;", "import org.junit.jupiter.api.AfterEach;", "import org.junit.jupiter.api.BeforeAll;", @@ -151,9 +168,16 @@ void replacement() { " @Test public void baz() {}", " @RepeatedTest(2) private void qux() {}", " @ParameterizedTest protected void quux() {}", + "", + " @Test public void testOverload() {}", + " void overload() {}", + " @Test protected void testArguments() {}", + " @Test private void testClass() {}", "}") .addOutputLines( "out/A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", "import org.junit.jupiter.api.AfterAll;", "import org.junit.jupiter.api.AfterEach;", "import org.junit.jupiter.api.BeforeAll;", @@ -174,136 +198,11 @@ void replacement() { " @Test void baz() {}", " @RepeatedTest(2) void qux() {}", " @ParameterizedTest void quux() {}", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void methodAlreadyExistsInClass() { - refactoringTestHelper - .addInputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test void testFoo() {}", - " void foo() {}", - "", - " @Test void testBar() {}", - " private void bar() {}", - "", - " @Test void testFooDifferent() {}", - " @Test void testBarDifferent() {}", - "}") - .addOutputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test void testFoo() {}", - " void foo() {}", - "", - " @Test void testBar() {}", - " private void bar() {}", - "", - " @Test void fooDifferent() {}", - " @Test void barDifferent() {}", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void methodAlreadyInStaticImports() { - refactoringTestHelper - .addInputLines( - "A.java", - "import static com.google.common.collect.ImmutableSet.toImmutableSet;", - "import static org.junit.jupiter.params.provider.Arguments.arguments;", - "", - "import org.junit.jupiter.api.Test;", - "import com.google.common.collect.ImmutableSet;", - "", - "class A {", - " @Test", - " void testArguments() {", - " arguments(1, 2, 3);", - " }", - "", - " @Test", - " void testToImmutableSet() {", - " ImmutableSet.of(1).stream().filter(i -> i > 1).collect(toImmutableSet());", - " }", - "", - " @Test", - " void testArgumentsDifferentName() {}", - "", - " @Test", - " void testToImmutableSetDifferentName() {}", - "}") - .addOutputLines( - "A.java", - "import static com.google.common.collect.ImmutableSet.toImmutableSet;", - "import static org.junit.jupiter.params.provider.Arguments.arguments;", - "", - "import org.junit.jupiter.api.Test;", - "import com.google.common.collect.ImmutableSet;", - "", - "class A {", - " @Test", - " void testArguments() {", - " arguments(1, 2, 3);", - " }", - "", - " @Test", - " void testToImmutableSet() {", - " ImmutableSet.of(1).stream().filter(i -> i > 1).collect(toImmutableSet());", - " }", - "", - " @Test", - " void argumentsDifferentName() {}", - "", - " @Test", - " void toImmutableSetDifferentName() {}", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void methodHasJavaKeyword() { - refactoringTestHelper - .addInputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test", - " void testClass() {}", - "", - " @Test", - " void testClazz() {}", - "", - " @Test", - " void testThrow() {}", - "", - " @Test", - " void testThrowww() {}", - "}") - .addOutputLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "", - "class A {", - " @Test", - " void testClass() {}", - "", - " @Test", - " void clazz() {}", - "", - " @Test", - " void testThrow() {}", "", - " @Test", - " void throwww() {}", + " @Test void testOverload() {}", + " void overload() {}", + " @Test void testArguments() {}", + " @Test void testClass() {}", "}") .doTest(TestMode.TEXT_MATCH); }