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 390afa55038..71057ab4451 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
@@ -22,12 +22,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;
@@ -80,50 +78,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:
+ *
+ *
+ * - Whether the rename would merely introduce a method overload, rather than clashing with an
+ * existing method declaration.
+ *
- Whether the rename would cause a method in a superclass to be overridden.
+ *
- Whether the rename would in fact clash with a static import. (It could be that a static
+ * import of the same name is only referenced from lexical scopes in which the method under
+ * consideration cannot be referenced directly.)
+ *
+ */
+ 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)
@@ -133,18 +153,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 9c25eb0d2e8..9e26bb8a44a 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);
}