Skip to content

Commit

Permalink
Simplify and remove obsolete tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@rickie
rickie committed Nov 10, 2022
1 parent 732eb79 commit fe8a4c2
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 199 deletions.
122 changes: 43 additions & 79 deletions ...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
*
* <p>At Picnic, we consider a JUnit factory method canonical if it
* <p>At Picnic, we consider a JUnit factory method canonical if it:
*
* <ul>
* <li>has the same name as the test method it provides test cases for, but with a `TestCases`
Expand All @@ -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() {}
Expand All @@ -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<String> factoryMethodName =
MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation);
Optional<ImmutableList<MethodTree>> 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<MethodTree> 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<Description> descriptions =
ImmutableList.<Description>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<Description> 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<Description> getSuggestedFixes(
MethodTree tree,
AnnotationTree methodSourceAnnotation,
MethodTree factoryMethod,
VisitorState state) {
ImmutableList<Description> factoryMethodNameFixes =
getFactoryMethodNameFixes(tree, methodSourceAnnotation, factoryMethod, state);

ImmutableList<Description> commentFixes =
getReturnStatementCommentFixes(tree, factoryMethod, state);

return ImmutableList.<Description>builder()
.addAll(factoryMethodNameFixes)
.addAll(commentFixes)
.build();
}

private ImmutableList<Description> 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)) {
Expand All @@ -163,29 +127,29 @@ private ImmutableList<Description> 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(
Expand Down
120 changes: 0 additions & 120 deletions ...b/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;",
Expand All @@ -133,67 +132,8 @@ void replacement() {
" void method1(int foo, boolean bar, String baz) {}",
"",
" private static Stream<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> method6TestCases() {",
" List<Arguments> 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<Arguments> method7TestCases() {",
" List<Arguments> arguments = List.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
" return arguments.stream();",
" }",
"}")
.addOutputLines(
"A.java",
Expand All @@ -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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> method6TestCases() {",
" List<Arguments> 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<Arguments> method7TestCases() {",
" List<Arguments> arguments = List.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
" /* { foo, bar, baz } */",
" return arguments.stream();",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
}
Expand Down

0 comments on commit fe8a4c2

Please sign in to comment.