Skip to content

Commit

Permalink
Suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@Stephan202 @rickie
Stephan202 authored and rickie committed Apr 12, 2022
1 parent f2e1c73 commit c27c0ff
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 168 deletions.
95 changes: 56 additions & 39 deletions ...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
*
* <p>This method implements imperfect heuristics. Things it currently does not consider include
* the following:
*
* <ul>
* <li>Whether the rename would merely introduce a method overload, rather than clashing with an
* existing method declaration.
* <li>Whether the rename would cause a method in a superclass to be overridden.
* <li>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.)
* </ul>
*/
private static Optional<String> 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)
Expand All @@ -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());
}
Expand Down
157 changes: 28 additions & 129 deletions ...rib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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();
}
Expand All @@ -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;",
Expand All @@ -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;",
Expand All @@ -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);
}
Expand Down

0 comments on commit c27c0ff

Please sign in to comment.