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 new file mode 100644 index 0000000000..92cb800f65 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java @@ -0,0 +1,249 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; +import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.annotations; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.enclosingClass; +import static com.google.errorprone.matchers.Matchers.hasModifier; +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 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; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.TEST_METHOD; + +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.common.collect.Streams; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +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.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 and usages. + * + *

At Picnic, we consider a JUnit factory method canonical if it: + * + *

+ */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "JUnit factory method declaration can likely be improved", + link = BUG_PATTERNS_BASE_URL + "JUnitFactoryMethodDeclaration", + linkType = CUSTOM, + severity = SUGGESTION, + tags = STYLE) +public final class JUnitFactoryMethodDeclaration extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher HAS_UNMODIFIABLE_SIGNATURE = + anyOf( + annotations(AT_LEAST_ONE, isType("java.lang.Override")), + allOf( + not(hasModifier(Modifier.FINAL)), + not(hasModifier(Modifier.PRIVATE)), + enclosingClass(hasModifier(Modifier.ABSTRACT)))); + + /** Instantiates a new {@link JUnitFactoryMethodDeclaration} instance. */ + public JUnitFactoryMethodDeclaration() {} + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (!TEST_METHOD.matches(tree, state) || !HAS_METHOD_SOURCE.matches(tree, state)) { + return Description.NO_MATCH; + } + + AnnotationTree methodSourceAnnotation = + ASTHelpers.getAnnotationWithSimpleName( + tree.getModifiers().getAnnotations(), "MethodSource"); + + Optional> factoryMethods = + MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation) + .map(name -> MoreASTHelpers.findMethods(name, state)); + + if (factoryMethods.isEmpty() || factoryMethods.orElseThrow().size() != 1) { + return Description.NO_MATCH; + } + + MethodTree factoryMethod = Iterables.getOnlyElement(factoryMethods.orElseThrow()); + ImmutableList descriptions = + ImmutableList.builder() + .addAll( + getFactoryMethodNameFixes( + tree.getName(), methodSourceAnnotation, factoryMethod, state)) + .addAll(getReturnStatementCommentFixes(tree, factoryMethod, state)) + .build(); + + descriptions.forEach(state::reportMatch); + return Description.NO_MATCH; + } + + private ImmutableList getFactoryMethodNameFixes( + Name methodName, + AnnotationTree methodSourceAnnotation, + MethodTree factoryMethod, + VisitorState state) { + String expectedFactoryMethodName = methodName + "TestCases"; + + if (HAS_UNMODIFIABLE_SIGNATURE.matches(factoryMethod, state) + || factoryMethod.getName().toString().equals(expectedFactoryMethodName)) { + return ImmutableList.of(); + } + + Optional blocker = + findMethodRenameBlocker( + ASTHelpers.getSymbol(factoryMethod), expectedFactoryMethodName, state); + if (blocker.isPresent()) { + reportMethodRenameBlocker( + factoryMethod, blocker.orElseThrow(), expectedFactoryMethodName, state); + return ImmutableList.of(); + } + + 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( + MethodTree tree, String reason, String suggestedName, VisitorState state) { + state.reportMatch( + buildDescription(tree) + .setMessage( + String.format( + "The test cases should be supplied by a method named `%s` (but note that %s)", + suggestedName, reason)) + .build()); + } + + private ImmutableList getReturnStatementCommentFixes( + MethodTree testMethod, MethodTree factoryMethod, VisitorState state) { + String expectedComment = createCommentContainingParameters(testMethod); + + List statements = factoryMethod.getBody().getStatements(); + + Stream returnStatementsNeedingComment = + Streams.mapWithIndex(statements.stream(), IndexedStatement::new) + .filter(indexedStatement -> indexedStatement.getStatement().getKind() == Kind.RETURN) + .filter( + indexedStatement -> + !hasExpectedComment( + factoryMethod, + expectedComment, + statements, + indexedStatement.getIndex(), + state)) + .map(IndexedStatement::getStatement); + + return returnStatementsNeedingComment + .map( + s -> + buildDescription(s) + .setMessage( + "The return statement should be prefixed by a comment giving the names of the test case parameters") + .addFix(SuggestedFix.prefixWith(s, expectedComment + "\n")) + .build()) + .collect(toImmutableList()); + } + + private static String createCommentContainingParameters(MethodTree testMethod) { + return testMethod.getParameters().stream() + .map(VariableTree::getName) + .map(Object::toString) + .collect(joining(", ", "/* { ", " } */")); + } + + private static boolean hasExpectedComment( + MethodTree factoryMethod, + String expectedComment, + List statements, + long statementIndex, + VisitorState state) { + int startPosition = + statementIndex > 0 + ? state.getEndPosition(statements.get((int) statementIndex - 1)) + : ASTHelpers.getStartPosition(factoryMethod); + int endPosition = state.getEndPosition(statements.get((int) statementIndex)); + + ImmutableList comments = + extractReturnStatementComments(startPosition, endPosition, state); + + return comments.stream() + .map(Comment::getText) + .anyMatch(comment -> comment.equals(expectedComment)); + } + + private static ImmutableList extractReturnStatementComments( + int startPosition, int endPosition, VisitorState state) { + return state.getOffsetTokens(startPosition, endPosition).stream() + .filter(t -> t.kind() == TokenKind.RETURN) + .flatMap(errorProneToken -> errorProneToken.comments().stream()) + .collect(toImmutableList()); + } + + private static final class IndexedStatement { + private final StatementTree statement; + private final long index; + + private IndexedStatement(StatementTree statement, long index) { + this.statement = statement; + this.index = index; + } + + public StatementTree getStatement() { + return statement; + } + + public long getIndex() { + return index; + } + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java index 20afd2fc96..f6cd5e6138 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java @@ -8,8 +8,8 @@ import static com.google.errorprone.matchers.Matchers.hasModifier; import static com.google.errorprone.matchers.Matchers.not; import static java.util.function.Predicate.not; +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.JavaKeywords.isValidIdentifier; import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.SETUP_OR_TEARDOWN_METHOD; import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.TEST_METHOD; @@ -25,15 +25,10 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; -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 com.sun.tools.javac.code.Symbol.MethodSymbol; -import com.sun.tools.javac.code.Type; import java.util.Optional; import javax.lang.model.element.Modifier; -import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** A {@link BugChecker} that flags non-canonical JUnit method declarations. */ // XXX: Consider introducing a class-level check that enforces that test classes: @@ -106,61 +101,7 @@ private void reportMethodRenameBlocker(MethodTree tree, String reason, VisitorSt .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( - MethodSymbol method, String newName, VisitorState state) { - if (isExistingMethodName(method.owner.type, newName, state)) { - return Optional.of( - String.format( - "a method named `%s` is already defined in this class or a supertype", newName)); - } - - if (isSimpleNameStaticallyImported(newName, state)) { - return Optional.of(String.format("`%s` is already statically imported", newName)); - } - - if (!isValidIdentifier(newName)) { - return Optional.of(String.format("`%s` is not a valid identifier", newName)); - } - - return Optional.empty(); - } - - private static boolean isExistingMethodName(Type clazz, String name, VisitorState state) { - return ASTHelpers.matchingMethods(state.getName(name), method -> true, clazz, state.getTypes()) - .findAny() - .isPresent(); - } - - private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) { - return state.getPath().getCompilationUnit().getImports().stream() - .filter(ImportTree::isStatic) - .map(ImportTree::getQualifiedIdentifier) - .map(tree -> getStaticImportSimpleName(tree, state)) - .anyMatch(simpleName::contentEquals); - } - - private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) { - String source = SourceCode.treeToString(tree, state); - return source.subSequence(source.lastIndexOf('.') + 1, source.length()); - } - - private static Optional tryCanonicalizeMethodName(Symbol symbol) { + private static Optional tryCanonicalizeMethodName(MethodSymbol symbol) { return Optional.of(symbol.getQualifiedName().toString()) .filter(name -> name.startsWith(TEST_PREFIX)) .map(name -> name.substring(TEST_PREFIX.length())) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java new file mode 100644 index 0000000000..5911bf89a1 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java @@ -0,0 +1,78 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isValidIdentifier; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ImportTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import java.util.Optional; + +/** + * A set of helper methods for finding conflicts which would be caused by applying certain fixes. + */ +public final class ConflictDetection { + private ConflictDetection() {} + + /** + * 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.) + *
+ * + * @param method XXX The proposed name to assign. + * @param newName The proposed name to assign. + * @param state The {@link VisitorState} to use for searching for blockers. + * @return A human-readable argument against assigning the proposed name to an existing method, or + * {@link Optional#empty()} if no blocker was found. + */ + public static Optional findMethodRenameBlocker( + MethodSymbol method, String newName, VisitorState state) { + if (isExistingMethodName(method.owner.type, newName, state)) { + return Optional.of( + String.format( + "a method named `%s` is already defined in this class or a supertype", newName)); + } + + if (isSimpleNameStaticallyImported(newName, state)) { + return Optional.of(String.format("`%s` is already statically imported", newName)); + } + + if (!isValidIdentifier(newName)) { + return Optional.of(String.format("`%s` is not a valid identifier", newName)); + } + + return Optional.empty(); + } + + private static boolean isExistingMethodName(Type clazz, String name, VisitorState state) { + return ASTHelpers.matchingMethods(state.getName(name), method -> true, clazz, state.getTypes()) + .findAny() + .isPresent(); + } + + private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) { + return state.getPath().getCompilationUnit().getImports().stream() + .filter(ImportTree::isStatic) + .map(ImportTree::getQualifiedIdentifier) + .map(tree -> getStaticImportSimpleName(tree, state)) + .anyMatch(simpleName::contentEquals); + } + + private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) { + String source = SourceCode.treeToString(tree, state); + return source.subSequence(source.lastIndexOf('.') + 1, source.length()); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java index c26bda471f..1660c9f41a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java @@ -10,14 +10,19 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.errorprone.matchers.AnnotationMatcherUtils; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.MultiMatcher; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewArrayTree; +import com.sun.tools.javac.code.Type; +import java.util.Optional; +import javax.lang.model.type.TypeKind; import org.jspecify.annotations.Nullable; /** @@ -78,4 +83,24 @@ private static String toMethodSourceFactoryName( return requireNonNullElse( Strings.emptyToNull(ASTHelpers.constValue(tree, String.class)), annotatedMethodName); } + + /** + * Extracts the name of the JUnit factory method from a {@link + * org.junit.jupiter.params.provider.MethodSource} annotation. + * + * @param methodSourceAnnotation The {@link org.junit.jupiter.params.provider.MethodSource} + * annotation to extract a method name from. + * @return The name of the factory method referred to in the annotation. Alternatively, {@link + * Optional#empty()} if there is more than one. + */ + public static Optional extractSingleFactoryMethodName( + AnnotationTree methodSourceAnnotation) { + ExpressionTree attributeExpression = + ((AssignmentTree) Iterables.getOnlyElement(methodSourceAnnotation.getArguments())) + .getExpression(); + Type attributeType = ASTHelpers.getType(attributeExpression); + return attributeType.getKind() == TypeKind.ARRAY + ? Optional.empty() + : Optional.of(attributeType.stringValue()); + } } 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 new file mode 100644 index 0000000000..38d81fee6d --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java @@ -0,0 +1,185 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class JUnitFactoryMethodDeclarationTest { + @Test + void identification() { + CompilationTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()) + .addSourceLines( + "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;", + "import org.junit.jupiter.params.provider.MethodSource;", + "", + "class A extends SuperA {", + " @ParameterizedTest", + " // BUG: Diagnostic contains: The test cases should be supplied by a method named", + " // `method1TestCases`", + " @MethodSource(\"testCasesForMethod1\")", + " void method1(int foo, boolean bar, String baz) {}", + "", + " // BUG: Diagnostic contains: The test cases should be supplied by a method named", + " // `method1TestCases`", + " 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() {", + " // BUG: Diagnostic contains: The return statement should be prefixed by a comment giving the", + " // names of the test case parameters", + " 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() {}", + "", + " // BUG: Diagnostic contains: The test cases should be supplied by a method named", + " // `method5TestCases` (but note that a method named `method5TestCases` is already defined in this", + " // class or a supertype)", + " 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\"));", + " if (true == true) {", + " /* { foo, bar, baz } */", + " return arguments.stream();", + " }", + " // BUG: Diagnostic contains: The return statement should be prefixed by a comment giving the", + " // names of the test case parameters", + " return arguments.stream();", + " }", + "", + " private static Stream method8TestCases() {", + " /* { foo, bar, baz } */", + " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"method8TestCases\")", + " void method8(int foo, boolean bar, String baz) {}", + "", + " private static Stream testCasesForMethod9() {", + " /* { foo, bar, baz } */", + " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));", + " }", + "", + " @MethodSource(\"testCasesForMethod9\")", + " void method9(int foo, boolean bar, String baz) {}", + "", + " @ParameterizedTest", + " void method10(int foo, boolean bar, String baz) {}", + "", + " @ParameterizedTest", + " @MethodSource(\"testCasesForMethod11\")", + " void method11(int foo, boolean bar, String baz) {}", + "", + " @Override", + " Stream testCasesForMethod11() {", + " /* { foo, bar, baz } */", + " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));", + " }", + "}") + .addSourceLines( + "SuperA.java", + "abstract class SuperA {", + " abstract Object testCasesForMethod11();", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()) + .addInputLines( + "A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", + "import java.util.stream.Stream;", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.Arguments;", + "import org.junit.jupiter.params.provider.MethodSource;", + "", + "class A {", + " @ParameterizedTest", + " @MethodSource(\"testCasesForMethod1\")", + " void method1(int foo, boolean bar, String baz) {}", + "", + " private static Stream testCasesForMethod1() {", + " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));", + " }", + "}") + .addOutputLines( + "A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", + "import java.util.stream.Stream;", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.Arguments;", + "import org.junit.jupiter.params.provider.MethodSource;", + "", + "class A {", + " @ParameterizedTest", + " @MethodSource(\"method1TestCases\")", + " void method1(int foo, boolean bar, String baz) {}", + "", + " private static Stream method1TestCases() {", + " /* { foo, bar, baz } */", + " return Stream.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java new file mode 100644 index 0000000000..9f6ce68dd5 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java @@ -0,0 +1,67 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Symbol; +import org.junit.jupiter.api.Test; + +final class ConflictDetectionTest { + @Test + void matcher() { + CompilationTestHelper.newInstance(RenameBlockerFlagger.class, getClass()) + .addSourceLines( + "/A.java", + "import static pkg.B.foo3t;", + "", + "class A {", + " private void foo1() {", + " foo3t();", + " }", + "", + " // BUG: Diagnostic contains: a method named `foo2t` is already defined in this class or a", + " // supertype", + " private void foo2() {}", + "", + " private void foo2t() {}", + "", + " // BUG: Diagnostic contains: `foo3t` is already statically imported", + " private void foo3() {}", + "", + " // BUG: Diagnostic contains: `int` is not a valid identifier", + " private void in() {}", + "}") + .addSourceLines( + "/pkg/B.java", + "package pkg;", + "", + "public class B {", + " public static void foo3t() {}", + "}") + .doTest(); + } + + /** + * A {@link BugChecker} that flags method rename blockers found by {@link + * ConflictDetection#findMethodRenameBlocker(Symbol.MethodSymbol, String, VisitorState)}. + */ + @BugPattern(summary = "Flags blockers for renaming methods", severity = ERROR) + public static final class RenameBlockerFlagger extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + return ConflictDetection.findMethodRenameBlocker( + ASTHelpers.getSymbol(tree), tree.getName() + "t", state) + .map(blocker -> buildDescription(tree).setMessage(blocker).build()) + .orElse(Description.NO_MATCH); + } + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java index c60bb8b286..686aad207c 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/RefasterRulesTest.java @@ -73,6 +73,7 @@ final class RefasterRulesTest { private static Stream validateRuleCollectionTestCases() { // XXX: Drop the filter once we have added tests for AssertJ! We can then also replace this // method with `@ValueSource(classes = {...})`. + /* { clazz } */ return RULE_COLLECTIONS.stream() .filter(not(AssertJRules.class::equals)) .map(Arguments::arguments);