mirror of
https://github.com/jlengrand/error-prone-support.git
synced 2026-03-10 08:11:25 +00:00
JUnitMethodDecl: emit warning instead of SuggestedFix if name clashes
This commit is contained in:
Rick Ossendrijver
committed by
Rick Ossendrijver
Rick Ossendrijver
parent
0824a5254b
commit
3ba8a83cd1
@@ -20,10 +20,16 @@ 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.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;
|
||||
|
||||
/** A {@link BugChecker} which flags non-canonical JUnit method declarations. */
|
||||
// XXX: Consider introducing a class-level check which enforces that test classes:
|
||||
@@ -78,24 +84,83 @@ public final class JUnitMethodDeclarationCheck extends BugChecker implements Met
|
||||
.ifPresent(builder::merge);
|
||||
|
||||
if (isTestMethod) {
|
||||
// XXX: In theory this rename could clash with an existing method or static import. In that
|
||||
// case we should emit a warning without a suggested replacement.
|
||||
tryCanonicalizeMethodName(tree, state).ifPresent(builder::merge);
|
||||
}
|
||||
Optional<String> newMethodName = tryCanonicalizeMethodName(tree);
|
||||
|
||||
if (newMethodName.isEmpty()) {
|
||||
return describeMatchesIfPresent(tree, builder);
|
||||
}
|
||||
|
||||
boolean reportedNameClash =
|
||||
reportDescriptionForPossibleNameClash(tree, newMethodName.orElseThrow(), state);
|
||||
if (!reportedNameClash) {
|
||||
builder.merge(SuggestedFixes.renameMethod(tree, newMethodName.orElseThrow(), state));
|
||||
}
|
||||
}
|
||||
return describeMatchesIfPresent(tree, builder);
|
||||
}
|
||||
|
||||
private Description describeMatchesIfPresent(MethodTree tree, SuggestedFix.Builder builder) {
|
||||
return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build());
|
||||
}
|
||||
|
||||
private static Optional<SuggestedFix> tryCanonicalizeMethodName(
|
||||
MethodTree tree, VisitorState state) {
|
||||
private boolean reportDescriptionForPossibleNameClash(
|
||||
MethodTree tree, String methodName, VisitorState state) {
|
||||
if (isMethodNameInClass(methodName, state)) {
|
||||
state.reportMatch(
|
||||
buildDescription(tree)
|
||||
.setMessage(
|
||||
String.format("A method with name %s already exists in the class.", methodName))
|
||||
.build());
|
||||
return true;
|
||||
}
|
||||
|
||||
if (isMethodNameStaticallyImported(methodName, state)) {
|
||||
state.reportMatch(
|
||||
buildDescription(tree)
|
||||
.setMessage(
|
||||
String.format(
|
||||
"A method with name %s is already statically imported.", methodName))
|
||||
.build());
|
||||
return true;
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
private static boolean isMethodNameInClass(String methodName, VisitorState state) {
|
||||
return state.findEnclosing(ClassTree.class).getMembers().stream()
|
||||
.filter(MethodTree.class::isInstance)
|
||||
.map(MethodTree.class::cast)
|
||||
.filter(member -> !ASTHelpers.isGeneratedConstructor(member))
|
||||
.map(MethodTree::getName)
|
||||
.map(Name::toString)
|
||||
.anyMatch(methodName::equals);
|
||||
}
|
||||
|
||||
private static boolean isMethodNameStaticallyImported(String methodName, VisitorState state) {
|
||||
CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit();
|
||||
|
||||
return compilationUnit.getImports().stream()
|
||||
.filter(Objects::nonNull)
|
||||
.filter(ImportTree::isStatic)
|
||||
.map(ImportTree::getQualifiedIdentifier)
|
||||
.map(tree -> getStaticImportIdentifier(tree, state))
|
||||
.anyMatch(methodName::contentEquals);
|
||||
}
|
||||
|
||||
private static CharSequence getStaticImportIdentifier(Tree tree, VisitorState state) {
|
||||
String source = Util.treeToString(tree, state);
|
||||
return source.subSequence(source.lastIndexOf('.') + 1, source.length());
|
||||
}
|
||||
|
||||
private static Optional<String> tryCanonicalizeMethodName(MethodTree tree) {
|
||||
return Optional.ofNullable(ASTHelpers.getSymbol(tree))
|
||||
.map(sym -> sym.getQualifiedName().toString())
|
||||
.filter(name -> name.startsWith(TEST_PREFIX))
|
||||
.map(name -> name.substring(TEST_PREFIX.length()))
|
||||
.filter(not(String::isEmpty))
|
||||
.map(name -> Character.toLowerCase(name.charAt(0)) + name.substring(1))
|
||||
.filter(name -> !Character.isDigit(name.charAt(0)))
|
||||
.map(name -> SuggestedFixes.renameMethod(tree, name, state));
|
||||
.filter(name -> !Character.isDigit(name.charAt(0)));
|
||||
}
|
||||
|
||||
// XXX: Move to a `MoreMatchers` utility class.
|
||||
|
||||
@@ -177,4 +177,94 @@ public final class JUnitMethodDeclarationCheckTest {
|
||||
"}")
|
||||
.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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user