diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
index a9f6af7416..294d446ff6 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
@@ -13,9 +13,6 @@
import static com.google.errorprone.matchers.Matchers.isType;
import static com.google.errorprone.matchers.Matchers.not;
import static java.util.stream.Collectors.joining;
-import static javax.lang.model.element.Modifier.ABSTRACT;
-import static javax.lang.model.element.Modifier.FINAL;
-import static javax.lang.model.element.Modifier.PRIVATE;
import static tech.picnic.errorprone.bugpatterns.util.ConflictDetection.findMethodRenameBlocker;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.HAS_METHOD_SOURCE;
@@ -35,24 +32,24 @@
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AnnotationTree;
-import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.parser.Tokens.Comment;
import com.sun.tools.javac.parser.Tokens.TokenKind;
-import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
+import javax.lang.model.element.Modifier;
+import javax.lang.model.element.Name;
import tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers;
import tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers;
/**
* A {@link BugChecker} that flags non-canonical JUnit factory method declarations.
*
- *
At Picnic, we consider a JUnit factory method canonical if it
+ *
At Picnic, we consider a JUnit factory method canonical if it:
*
*
* - has the same name as the test method it provides test cases for, but with a `TestCases`
@@ -75,9 +72,9 @@ public final class JUnitFactoryMethodDeclaration extends BugChecker implements M
anyOf(
annotations(AT_LEAST_ONE, isType("java.lang.Override")),
allOf(
- not(hasModifier(FINAL)),
- not(hasModifier(PRIVATE)),
- enclosingClass(hasModifier(ABSTRACT))));
+ not(hasModifier(Modifier.FINAL)),
+ not(hasModifier(Modifier.PRIVATE)),
+ enclosingClass(hasModifier(Modifier.ABSTRACT))));
/** Instantiates a new {@link JUnitFactoryMethodDeclaration} instance. */
public JUnitFactoryMethodDeclaration() {}
@@ -92,66 +89,33 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
ASTHelpers.getAnnotationWithSimpleName(
tree.getModifiers().getAnnotations(), "MethodSource");
- if (methodSourceAnnotation == null) {
- return Description.NO_MATCH;
- }
-
- Optional factoryMethodName =
- MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation);
+ Optional> factoryMethods =
+ MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation)
+ .map(name -> MoreASTHelpers.findMethods(name, state));
- if (factoryMethodName.isEmpty()) {
- /* If a test has multiple factory methods, not all of them can be given the desired name. */
+ if (factoryMethods.isEmpty() || factoryMethods.orElseThrow().size() != 1) {
return Description.NO_MATCH;
}
- ImmutableList factoryMethods =
- Optional.ofNullable(state.findEnclosing(ClassTree.class))
- .map(
- enclosingClass ->
- MoreASTHelpers.findMethods(factoryMethodName.orElseThrow(), state))
- .stream()
- .flatMap(Collection::stream)
- .filter(methodTree -> methodTree.getParameters().isEmpty())
- .collect(toImmutableList());
+ MethodTree factoryMethod = Iterables.getOnlyElement(factoryMethods.orElseThrow());
+ ImmutableList descriptions =
+ ImmutableList.builder()
+ .addAll(
+ getFactoryMethodNameFixes(
+ tree.getName(), methodSourceAnnotation, factoryMethod, state))
+ .addAll(getReturnStatementCommentFixes(tree, factoryMethod, state))
+ .build();
- if (factoryMethods.size() != 1) {
- /* If we cannot reliably find the factory method, err on the side of not proposing any fixes. */
- return Description.NO_MATCH;
- }
-
- ImmutableList fixes =
- getSuggestedFixes(
- tree, methodSourceAnnotation, Iterables.getOnlyElement(factoryMethods), state);
-
- /* Even though we match on the test method, none of the fixes apply to it directly, so we report
- the fixes separately using `VisitorState::reportMatch`. */
- fixes.forEach(state::reportMatch);
+ descriptions.forEach(state::reportMatch);
return Description.NO_MATCH;
}
- private ImmutableList getSuggestedFixes(
- MethodTree tree,
- AnnotationTree methodSourceAnnotation,
- MethodTree factoryMethod,
- VisitorState state) {
- ImmutableList factoryMethodNameFixes =
- getFactoryMethodNameFixes(tree, methodSourceAnnotation, factoryMethod, state);
-
- ImmutableList commentFixes =
- getReturnStatementCommentFixes(tree, factoryMethod, state);
-
- return ImmutableList.builder()
- .addAll(factoryMethodNameFixes)
- .addAll(commentFixes)
- .build();
- }
-
private ImmutableList getFactoryMethodNameFixes(
- MethodTree tree,
+ Name methodName,
AnnotationTree methodSourceAnnotation,
MethodTree factoryMethod,
VisitorState state) {
- String expectedFactoryMethodName = tree.getName() + "TestCases";
+ String expectedFactoryMethodName = methodName + "TestCases";
if (HAS_UNMODIFIABLE_SIGNATURE.matches(factoryMethod, state)
|| factoryMethod.getName().toString().equals(expectedFactoryMethodName)) {
@@ -163,29 +127,29 @@ private ImmutableList getFactoryMethodNameFixes(
reportMethodRenameBlocker(
factoryMethod, blocker.orElseThrow(), expectedFactoryMethodName, state);
return ImmutableList.of();
- } else {
- return ImmutableList.of(
- buildDescription(methodSourceAnnotation)
- .setMessage(
- String.format(
- "The test cases should be supplied by a method named `%s`",
- expectedFactoryMethodName))
- .addFix(
- SuggestedFixes.updateAnnotationArgumentValues(
- methodSourceAnnotation,
- state,
- "value",
- ImmutableList.of("\"" + expectedFactoryMethodName + "\""))
- .build())
- .build(),
- buildDescription(factoryMethod)
- .setMessage(
- String.format(
- "The test cases should be supplied by a method named `%s`",
- expectedFactoryMethodName))
- .addFix(SuggestedFixes.renameMethod(factoryMethod, expectedFactoryMethodName, state))
- .build());
}
+
+ return ImmutableList.of(
+ buildDescription(methodSourceAnnotation)
+ .setMessage(
+ String.format(
+ "The test cases should be supplied by a method named `%s`",
+ expectedFactoryMethodName))
+ .addFix(
+ SuggestedFixes.updateAnnotationArgumentValues(
+ methodSourceAnnotation,
+ state,
+ "value",
+ ImmutableList.of("\"" + expectedFactoryMethodName + "\""))
+ .build())
+ .build(),
+ buildDescription(factoryMethod)
+ .setMessage(
+ String.format(
+ "The test cases should be supplied by a method named `%s`",
+ expectedFactoryMethodName))
+ .addFix(SuggestedFixes.renameMethod(factoryMethod, expectedFactoryMethodName, state))
+ .build());
}
private void reportMethodRenameBlocker(
diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java
index 1173376521..56f9eacbf2 100644
--- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java
+++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java
@@ -121,7 +121,6 @@ void replacement() {
"A.java",
"import static org.junit.jupiter.params.provider.Arguments.arguments;",
"",
- "import java.util.List;",
"import java.util.stream.Stream;",
"import org.junit.jupiter.params.ParameterizedTest;",
"import org.junit.jupiter.params.provider.Arguments;",
@@ -133,67 +132,8 @@ void replacement() {
" void method1(int foo, boolean bar, String baz) {}",
"",
" private static Stream testCasesForMethod1() {",
- " /* { foo, bar, baz } */",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method2TestCases\")",
- " void method2(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method2TestCases() {",
- " /* { foo, bar, baz } */",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " private static void method2TestCases(int i) {}",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method3TestCases\")",
- " void method3(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method3TestCases() {",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method4TestCases\")",
- " void method4(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method4TestCases() {",
- " /* { foo, bar, baz } */",
" return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
" }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"testCasesForMethod5\")",
- " void method5(int foo, boolean bar, String baz) {}",
- "",
- " void method5TestCases() {}",
- "",
- " private static Stream testCasesForMethod5() {",
- " /* { foo, bar, baz } */",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method6TestCases\")",
- " void method6(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method6TestCases() {",
- " List arguments = List.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " /* { foo, bar, baz } */",
- " return arguments.stream();",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method7TestCases\")",
- " void method7(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method7TestCases() {",
- " List arguments = List.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " return arguments.stream();",
- " }",
"}")
.addOutputLines(
"A.java",
@@ -214,66 +154,6 @@ void replacement() {
" /* { foo, bar, baz } */",
" return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
" }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method2TestCases\")",
- " void method2(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method2TestCases() {",
- " /* { foo, bar, baz } */",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " private static void method2TestCases(int i) {}",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method3TestCases\")",
- " void method3(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method3TestCases() {",
- " /* { foo, bar, baz } */",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method4TestCases\")",
- " void method4(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method4TestCases() {",
- " /* { foo, bar, baz } */",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"testCasesForMethod5\")",
- " void method5(int foo, boolean bar, String baz) {}",
- "",
- " void method5TestCases() {}",
- "",
- " private static Stream testCasesForMethod5() {",
- " /* { foo, bar, baz } */",
- " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method6TestCases\")",
- " void method6(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method6TestCases() {",
- " List arguments = List.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " /* { foo, bar, baz } */",
- " return arguments.stream();",
- " }",
- "",
- " @ParameterizedTest",
- " @MethodSource(\"method7TestCases\")",
- " void method7(int foo, boolean bar, String baz) {}",
- "",
- " private static Stream method7TestCases() {",
- " List arguments = List.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
- " /* { foo, bar, baz } */",
- " return arguments.stream();",
- " }",
"}")
.doTest(TestMode.TEXT_MATCH);
}