From ec79df92e76ad097c5beab3716b2b45e01e0dc35 Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Fri, 4 Nov 2022 16:30:15 +0100 Subject: [PATCH 01/14] Create JUnit matchers as preparation for additional JUnit BugCheckers --- .../bugpatterns/util/MoreASTHelpers.java | 8 +++--- .../bugpatterns/util/MoreJUnitMatchers.java | 25 +++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java index ccfd3353bb..14b08a5c19 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java @@ -29,10 +29,10 @@ public static ImmutableList findMethods(CharSequence methodName, Vis ClassTree clazz = state.findEnclosing(ClassTree.class); checkArgument(clazz != null, "Visited node is not enclosed by a class"); return clazz.getMembers().stream() - .filter(MethodTree.class::isInstance) - .map(MethodTree.class::cast) - .filter(method -> method.getName().contentEquals(methodName)) - .collect(toImmutableList()); + .filter(MethodTree.class::isInstance) + .map(MethodTree.class::cast) + .filter(method -> method.getName().contentEquals(methodName)) + .collect(toImmutableList()); } /** 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..be780a49ce 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 methods referred to in the annotation if there is only one, or + * {@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()); + } } From de71440bb2c8bc407921c862e1d9a4698853c4e4 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 7 Nov 2022 14:55:01 +0100 Subject: [PATCH 02/14] Suggestions --- .../bugpatterns/util/MoreJUnitMatchers.java | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) 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 be780a49ce..e3365df835 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 @@ -34,25 +34,25 @@ public final class MoreJUnitMatchers { /** Matches JUnit Jupiter test methods. */ public static final MultiMatcher TEST_METHOD = - annotations( - AT_LEAST_ONE, - anyOf( - isType("org.junit.jupiter.api.Test"), - hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"))); + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.junit.jupiter.api.Test"), + hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"))); /** Matches JUnit Jupiter setup and teardown methods. */ public static final MultiMatcher SETUP_OR_TEARDOWN_METHOD = - annotations( - AT_LEAST_ONE, - anyOf( - isType("org.junit.jupiter.api.AfterAll"), - isType("org.junit.jupiter.api.AfterEach"), - isType("org.junit.jupiter.api.BeforeAll"), - isType("org.junit.jupiter.api.BeforeEach"))); + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.junit.jupiter.api.AfterAll"), + isType("org.junit.jupiter.api.AfterEach"), + isType("org.junit.jupiter.api.BeforeAll"), + isType("org.junit.jupiter.api.BeforeEach"))); /** * Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation. */ public static final MultiMatcher HAS_METHOD_SOURCE = - annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")); + annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")); private MoreJUnitMatchers() {} @@ -64,7 +64,7 @@ private MoreJUnitMatchers() {} * @return One or more value factory names. */ static ImmutableSet getMethodSourceFactoryNames( - AnnotationTree methodSourceAnnotation, MethodTree method) { + AnnotationTree methodSourceAnnotation, MethodTree method) { String methodName = method.getName().toString(); ExpressionTree value = AnnotationMatcherUtils.getArgument(methodSourceAnnotation, "value"); @@ -73,15 +73,15 @@ static ImmutableSet getMethodSourceFactoryNames( } return ((NewArrayTree) value) - .getInitializers().stream() + .getInitializers().stream() .map(name -> toMethodSourceFactoryName(name, methodName)) .collect(toImmutableSet()); } private static String toMethodSourceFactoryName( - @Nullable ExpressionTree tree, String annotatedMethodName) { + @Nullable ExpressionTree tree, String annotatedMethodName) { return requireNonNullElse( - Strings.emptyToNull(ASTHelpers.constValue(tree, String.class)), annotatedMethodName); + Strings.emptyToNull(ASTHelpers.constValue(tree, String.class)), annotatedMethodName); } /** @@ -94,13 +94,13 @@ private static String toMethodSourceFactoryName( * {@link Optional#empty()} if there is more than one. */ public static Optional extractSingleFactoryMethodName( - AnnotationTree methodSourceAnnotation) { + AnnotationTree methodSourceAnnotation) { ExpressionTree attributeExpression = - ((AssignmentTree) Iterables.getOnlyElement(methodSourceAnnotation.getArguments())) - .getExpression(); + ((AssignmentTree) Iterables.getOnlyElement(methodSourceAnnotation.getArguments())) + .getExpression(); Type attributeType = ASTHelpers.getType(attributeExpression); return attributeType.getKind() == TypeKind.ARRAY - ? Optional.empty() - : Optional.of(attributeType.stringValue()); + ? Optional.empty() + : Optional.of(attributeType.stringValue()); } } From 40b1e867a631d29811428ea5673171564eb4eb51 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 7 Nov 2022 15:49:07 +0100 Subject: [PATCH 03/14] Suggestions --- .../bugpatterns/util/MoreJUnitMatchers.java | 87 ++++++------------- 1 file changed, 27 insertions(+), 60 deletions(-) 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 e3365df835..f158a6f4dd 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 @@ -1,17 +1,13 @@ package tech.picnic.errorprone.bugpatterns.util; -import static com.google.common.collect.ImmutableSet.toImmutableSet; 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.isType; -import static java.util.Objects.requireNonNullElse; import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.hasMetaAnnotation; -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; @@ -19,88 +15,59 @@ 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; /** - * A collection of JUnit-specific helper methods and {@link Matcher}s. + * A set of JUnit Jupiter-specific helper methods and {@link Matcher Matchers}. * - *

These constants and methods are additions to the ones found in {@link - * com.google.errorprone.matchers.JUnitMatchers}. + *

These are additions to the ones from {@link com.google.errorprone.matchers.JUnitMatchers}. */ public final class MoreJUnitMatchers { /** Matches JUnit Jupiter test methods. */ public static final MultiMatcher TEST_METHOD = - annotations( - AT_LEAST_ONE, - anyOf( - isType("org.junit.jupiter.api.Test"), - hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"))); - /** Matches JUnit Jupiter setup and teardown methods. */ + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.junit.jupiter.api.Test"), + hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"))); + + /** Matches JUnit setup and teardown methods. */ public static final MultiMatcher SETUP_OR_TEARDOWN_METHOD = - annotations( - AT_LEAST_ONE, - anyOf( - isType("org.junit.jupiter.api.AfterAll"), - isType("org.junit.jupiter.api.AfterEach"), - isType("org.junit.jupiter.api.BeforeAll"), - isType("org.junit.jupiter.api.BeforeEach"))); + annotations( + AT_LEAST_ONE, + anyOf( + isType("org.junit.jupiter.api.AfterAll"), + isType("org.junit.jupiter.api.AfterEach"), + isType("org.junit.jupiter.api.BeforeAll"), + isType("org.junit.jupiter.api.BeforeEach"))); + /** * Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation. */ - public static final MultiMatcher HAS_METHOD_SOURCE = - annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")); + public static final Matcher HAS_METHOD_SOURCE = + allOf(annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource"))); private MoreJUnitMatchers() {} - /** - * Returns the names of the JUnit value factory methods specified by the given {@link - * org.junit.jupiter.params.provider.MethodSource} annotation. - * - * @param methodSourceAnnotation The annotation from which to extract value factory method names. - * @return One or more value factory names. - */ - static ImmutableSet getMethodSourceFactoryNames( - AnnotationTree methodSourceAnnotation, MethodTree method) { - String methodName = method.getName().toString(); - ExpressionTree value = AnnotationMatcherUtils.getArgument(methodSourceAnnotation, "value"); - - if (!(value instanceof NewArrayTree)) { - return ImmutableSet.of(toMethodSourceFactoryName(value, methodName)); - } - - return ((NewArrayTree) value) - .getInitializers().stream() - .map(name -> toMethodSourceFactoryName(name, methodName)) - .collect(toImmutableSet()); - } - - private static String toMethodSourceFactoryName( - @Nullable ExpressionTree tree, String annotatedMethodName) { - 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 methods referred to in the annotation if there is only one, or - * {@link Optional#empty()} if there is more than one. + * @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) { + AnnotationTree methodSourceAnnotation) { ExpressionTree attributeExpression = - ((AssignmentTree) Iterables.getOnlyElement(methodSourceAnnotation.getArguments())) - .getExpression(); + ((AssignmentTree) Iterables.getOnlyElement(methodSourceAnnotation.getArguments())) + .getExpression(); Type attributeType = ASTHelpers.getType(attributeExpression); return attributeType.getKind() == TypeKind.ARRAY - ? Optional.empty() - : Optional.of(attributeType.stringValue()); + ? Optional.empty() + : Optional.of(attributeType.stringValue()); } } From 72045152a4aa5987178f1f2758639f88aa9721ad Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Tue, 8 Nov 2022 11:35:34 +0100 Subject: [PATCH 04/14] Address review comments --- .../errorprone/bugpatterns/util/MoreJUnitMatchers.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 f158a6f4dd..abe98b07d4 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 @@ -1,7 +1,6 @@ package tech.picnic.errorprone.bugpatterns.util; 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.isType; @@ -20,7 +19,7 @@ import javax.lang.model.type.TypeKind; /** - * A set of JUnit Jupiter-specific helper methods and {@link Matcher Matchers}. + * A set of JUnit-specific helper methods and {@link Matcher Matchers}. * *

These are additions to the ones from {@link com.google.errorprone.matchers.JUnitMatchers}. */ @@ -47,7 +46,7 @@ public final class MoreJUnitMatchers { * Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation. */ public static final Matcher HAS_METHOD_SOURCE = - allOf(annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource"))); + annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")); private MoreJUnitMatchers() {} From 6cd9d8a9b81aa3ffd4cd1ed4a697023bd224fb14 Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Tue, 8 Nov 2022 14:07:46 +0100 Subject: [PATCH 05/14] Address review comments --- .../picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 abe98b07d4..732a945453 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 @@ -21,7 +21,8 @@ /** * A set of JUnit-specific helper methods and {@link Matcher Matchers}. * - *

These are additions to the ones from {@link com.google.errorprone.matchers.JUnitMatchers}. + *

These helper methods are additions to the ones from {@link + * com.google.errorprone.matchers.JUnitMatchers}. */ public final class MoreJUnitMatchers { /** Matches JUnit Jupiter test methods. */ From 2f5f35546283e64954ecfd9ec281c5301ec5570b Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Tue, 25 Oct 2022 14:21:00 +0200 Subject: [PATCH 06/14] Introduce JUnit factory method BugChecker --- .../JUnitFactoryMethodDeclaration.java | 283 ++++++++++++++++++ .../bugpatterns/JUnitMethodDeclaration.java | 65 +--- .../bugpatterns/util/ConflictDetection.java | 67 +++++ .../JUnitFactoryMethodDeclarationTest.java | 280 +++++++++++++++++ .../refasterrules/RefasterRulesTest.java | 1 + 5 files changed, 634 insertions(+), 62 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java 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..a9f6af7416 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java @@ -0,0 +1,283 @@ +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 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; +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.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 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 + * + *

    + *
  • has the same name as the test method it provides test cases for, but with a `TestCases` + * suffix, and + *
  • has a comment which connects the return statement to the names of the parameters in the + * corresponding test method. + *
+ */ +@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(FINAL)), + not(hasModifier(PRIVATE)), + enclosingClass(hasModifier(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"); + + if (methodSourceAnnotation == null) { + return Description.NO_MATCH; + } + + Optional factoryMethodName = + MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation); + + if (factoryMethodName.isEmpty()) { + /* If a test has multiple factory methods, not all of them can be given the desired name. */ + 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()); + + 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); + 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, + AnnotationTree methodSourceAnnotation, + MethodTree factoryMethod, + VisitorState state) { + String expectedFactoryMethodName = tree.getName() + "TestCases"; + + if (HAS_UNMODIFIABLE_SIGNATURE.matches(factoryMethod, state) + || factoryMethod.getName().toString().equals(expectedFactoryMethodName)) { + return ImmutableList.of(); + } + + Optional blocker = findMethodRenameBlocker(expectedFactoryMethodName, state); + if (blocker.isPresent()) { + 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()); + } + } + + 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) { + ImmutableList parameterNames = + testMethod.getParameters().stream() + .map(VariableTree::getName) + .map(Object::toString) + .collect(toImmutableList()); + + String expectedComment = parameterNames.stream().collect(joining(", ", "/* { ", " } */")); + + 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 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..8ad1131be2 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: @@ -90,7 +85,7 @@ private void suggestTestMethodRenameIfApplicable( tryCanonicalizeMethodName(symbol) .ifPresent( newName -> - findMethodRenameBlocker(symbol, newName, state) + findMethodRenameBlocker(newName, state) .ifPresentOrElse( blocker -> reportMethodRenameBlocker(tree, blocker, state), () -> fixBuilder.merge(SuggestedFixes.renameMethod(tree, newName, state)))); @@ -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..398bfdeda6 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java @@ -0,0 +1,67 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isReservedKeyword; +import static tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers.isMethodInEnclosingClass; + +import com.google.errorprone.VisitorState; +import com.sun.source.tree.ImportTree; +import com.sun.source.tree.Tree; +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 methodName 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(String methodName, VisitorState state) { + if (MoreASTHelpers.methodExistsInEnclosingClass(methodName, state)) { + return Optional.of( + String.format("a method named `%s` already exists in this class", methodName)); + } + + if (isSimpleNameStaticallyImported(methodName, state)) { + return Optional.of(String.format("`%s` is already statically imported", methodName)); + } + + if (isReservedKeyword(methodName)) { + return Optional.of(String.format("`%s` is a reserved keyword", methodName)); + } + + return Optional.empty(); + } + + 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/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..1173376521 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java @@ -0,0 +1,280 @@ +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 { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()); + + @Test + void identification() { + compilationTestHelper + .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 {", + " @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: (but note that a method named `method5TestCases` already exists in", + " // this class)", + " 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\"));", + " // 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) {}", + "}") + .doTest(); + } + + @Test + void replacement() { + refactoringTestHelper + .addInputLines( + "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 {", + " @ParameterizedTest", + " @MethodSource(\"testCasesForMethod1\")", + " 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", + "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 {", + " @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\"));", + " }", + "", + " @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); + } +} 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); From 3db334f081eb06f6ec37c471b369f858f3fe4c22 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 10 Nov 2022 21:31:35 +0100 Subject: [PATCH 07/14] Simplify and remove obsolete tests --- .../JUnitFactoryMethodDeclaration.java | 122 ++++++------------ .../JUnitFactoryMethodDeclarationTest.java | 120 ----------------- 2 files changed, 43 insertions(+), 199 deletions(-) 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); } From 048ef6e5cc040e6dd08eeafe03311c135b284cc2 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 8 Dec 2022 21:27:25 +0100 Subject: [PATCH 08/14] Rebase and resolve conflicts --- .../bugpatterns/util/ConflictDetection.java | 4 +- .../bugpatterns/util/MoreASTHelpers.java | 8 ++-- .../bugpatterns/util/MoreJUnitMatchers.java | 45 ++++++++++++++++--- .../JUnitFactoryMethodDeclarationTest.java | 1 - 4 files changed, 45 insertions(+), 13 deletions(-) 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 index 398bfdeda6..dbb313d1ef 100644 --- 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 @@ -1,7 +1,7 @@ package tech.picnic.errorprone.bugpatterns.util; import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isReservedKeyword; -import static tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers.isMethodInEnclosingClass; +import static tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers.methodExistsInEnclosingClass; import com.google.errorprone.VisitorState; import com.sun.source.tree.ImportTree; @@ -36,7 +36,7 @@ private ConflictDetection() {} * {@link Optional#empty()} if no blocker was found. */ public static Optional findMethodRenameBlocker(String methodName, VisitorState state) { - if (MoreASTHelpers.methodExistsInEnclosingClass(methodName, state)) { + if (methodExistsInEnclosingClass(methodName, state)) { return Optional.of( String.format("a method named `%s` already exists in this class", methodName)); } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java index 14b08a5c19..ccfd3353bb 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java @@ -29,10 +29,10 @@ public static ImmutableList findMethods(CharSequence methodName, Vis ClassTree clazz = state.findEnclosing(ClassTree.class); checkArgument(clazz != null, "Visited node is not enclosed by a class"); return clazz.getMembers().stream() - .filter(MethodTree.class::isInstance) - .map(MethodTree.class::cast) - .filter(method -> method.getName().contentEquals(methodName)) - .collect(toImmutableList()); + .filter(MethodTree.class::isInstance) + .map(MethodTree.class::cast) + .filter(method -> method.getName().contentEquals(methodName)) + .collect(toImmutableList()); } /** 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 732a945453..52e647e0c4 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 @@ -1,12 +1,17 @@ package tech.picnic.errorprone.bugpatterns.util; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; import static com.google.errorprone.matchers.Matchers.annotations; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.isType; +import static java.util.Objects.requireNonNullElse; import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.hasMetaAnnotation; +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; @@ -14,14 +19,16 @@ 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.nullness.Nullable; /** - * A set of JUnit-specific helper methods and {@link Matcher Matchers}. + * A collection of JUnit-specific helper methods and {@link Matcher}s. * - *

    These helper methods are additions to the ones from {@link + *

    These constants and methods are additions to the ones found in {@link * com.google.errorprone.matchers.JUnitMatchers}. */ public final class MoreJUnitMatchers { @@ -32,8 +39,7 @@ public final class MoreJUnitMatchers { anyOf( isType("org.junit.jupiter.api.Test"), hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"))); - - /** Matches JUnit setup and teardown methods. */ + /** Matches JUnit Jupiter setup and teardown methods. */ public static final MultiMatcher SETUP_OR_TEARDOWN_METHOD = annotations( AT_LEAST_ONE, @@ -42,15 +48,42 @@ public final class MoreJUnitMatchers { isType("org.junit.jupiter.api.AfterEach"), isType("org.junit.jupiter.api.BeforeAll"), isType("org.junit.jupiter.api.BeforeEach"))); - /** * Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation. */ - public static final Matcher HAS_METHOD_SOURCE = + public static final MultiMatcher HAS_METHOD_SOURCE = annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")); private MoreJUnitMatchers() {} + /** + * Returns the names of the JUnit value factory methods specified by the given {@link + * org.junit.jupiter.params.provider.MethodSource} annotation. + * + * @param methodSourceAnnotation The annotation from which to extract value factory method names. + * @return One or more value factory names. + */ + static ImmutableSet getMethodSourceFactoryNames( + AnnotationTree methodSourceAnnotation, MethodTree method) { + String methodName = method.getName().toString(); + ExpressionTree value = AnnotationMatcherUtils.getArgument(methodSourceAnnotation, "value"); + + if (!(value instanceof NewArrayTree)) { + return ImmutableSet.of(toMethodSourceFactoryName(value, methodName)); + } + + return ((NewArrayTree) value) + .getInitializers().stream() + .map(name -> toMethodSourceFactoryName(name, methodName)) + .collect(toImmutableSet()); + } + + private static String toMethodSourceFactoryName( + @Nullable ExpressionTree tree, String annotatedMethodName) { + 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. 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 56f9eacbf2..02729e57b2 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 @@ -139,7 +139,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;", From 2758fedf60f376164a8defe4c9d5e0aa278127c0 Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Mon, 12 Dec 2022 16:36:45 +0100 Subject: [PATCH 09/14] Refine tests based on mutation testing --- .../JUnitFactoryMethodDeclarationTest.java | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) 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 02729e57b2..0d9d069d74 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 @@ -24,7 +24,7 @@ void identification() { "import org.junit.jupiter.params.provider.Arguments;", "import org.junit.jupiter.params.provider.MethodSource;", "", - "class A {", + "class A extends SuperA {", " @ParameterizedTest", " // BUG: Diagnostic contains: The test cases should be supplied by a method named", " // `method1TestCases`", @@ -97,6 +97,10 @@ void identification() { "", " 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();", @@ -110,6 +114,32 @@ void identification() { " @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(); } From 4ac2a0aaf96f96c2456146efbb18df8eee556457 Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Mon, 19 Dec 2022 15:24:25 +0100 Subject: [PATCH 10/14] Add tests for the ConflictDetection utility class --- .../bugpatterns/util/MoreJUnitMatchers.java | 2 +- .../util/ConflictDetectionTest.java | 60 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java 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 52e647e0c4..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 @@ -23,7 +23,7 @@ import com.sun.tools.javac.code.Type; import java.util.Optional; import javax.lang.model.type.TypeKind; -import org.jspecify.nullness.Nullable; +import org.jspecify.annotations.Nullable; /** * A collection of JUnit-specific helper methods and {@link Matcher}s. 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..abf0140891 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java @@ -0,0 +1,60 @@ +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.matchers.Description; +import com.sun.source.tree.MethodTree; +import org.junit.jupiter.api.Test; + +final class ConflictDetectionTest { + @Test + void matcher() { + CompilationTestHelper.newInstance(RenameBlockerFlagger.class, getClass()) + .addSourceLines( + "/A.java", + "import static staticimport.B.foo3t;", + "", + "class A {", + " private void foo1() {", + " foo3t();", + " }", + "", + " // BUG: Diagnostic contains: [RenameBlockerFlagger] a method named `foo2t` already exists in this", + " // class", + " private void foo2() {}", + "", + " private void foo2t() {}", + "", + " // BUG: Diagnostic contains: [RenameBlockerFlagger] `foo3t` is already statically imported", + " private void foo3() {}", + "", + " // BUG: Diagnostic contains: [RenameBlockerFlagger] `int` is a reserved keyword", + " private void in() {}", + "}") + .addSourceLines( + "/staticimport/B.java", + "package staticimport;", + "", + "public class B {", + " public static void foo3t() {}", + "}") + .doTest(); + } + + @BugPattern(summary = "Flags blockers for renaming methods", severity = ERROR) + public static final class RenameBlockerFlagger extends BugChecker + implements BugChecker.MethodTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + return ConflictDetection.findMethodRenameBlocker(tree.getName() + "t", state) + .map(blocker -> buildDescription(tree).setMessage(blocker).build()) + .orElse(Description.NO_MATCH); + } + } +} From d7adfbcfab7bb1dfc14dda5bf39823293148bb7b Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 12 Jan 2023 08:37:46 +0100 Subject: [PATCH 11/14] Update after rebase with latest improvements --- .../JUnitFactoryMethodDeclaration.java | 4 ++- .../bugpatterns/JUnitMethodDeclaration.java | 2 +- .../bugpatterns/util/ConflictDetection.java | 31 +++++++++++++------ .../errorprone/refasterrules/StringRules.java | 7 ++++- .../JUnitFactoryMethodDeclarationTest.java | 5 +-- .../util/ConflictDetectionTest.java | 14 +++++---- 6 files changed, 42 insertions(+), 21 deletions(-) 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 294d446ff6..e353cca4ca 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 @@ -122,7 +122,9 @@ private ImmutableList getFactoryMethodNameFixes( return ImmutableList.of(); } - Optional blocker = findMethodRenameBlocker(expectedFactoryMethodName, state); + Optional blocker = + findMethodRenameBlocker( + ASTHelpers.getSymbol(factoryMethod), expectedFactoryMethodName, state); if (blocker.isPresent()) { reportMethodRenameBlocker( factoryMethod, blocker.orElseThrow(), expectedFactoryMethodName, state); 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 8ad1131be2..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 @@ -85,7 +85,7 @@ private void suggestTestMethodRenameIfApplicable( tryCanonicalizeMethodName(symbol) .ifPresent( newName -> - findMethodRenameBlocker(newName, state) + findMethodRenameBlocker(symbol, newName, state) .ifPresentOrElse( blocker -> reportMethodRenameBlocker(tree, blocker, state), () -> fixBuilder.merge(SuggestedFixes.renameMethod(tree, newName, state)))); 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 index dbb313d1ef..5911bf89a1 100644 --- 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 @@ -1,11 +1,13 @@ package tech.picnic.errorprone.bugpatterns.util; -import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isReservedKeyword; -import static tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers.methodExistsInEnclosingClass; +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; /** @@ -30,28 +32,37 @@ private ConflictDetection() {} * consideration cannot be referenced directly.) *

* - * @param methodName The proposed name to assign. + * @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(String methodName, VisitorState state) { - if (methodExistsInEnclosingClass(methodName, state)) { + 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` already exists in this class", methodName)); + String.format( + "a method named `%s` is already defined in this class or a supertype", newName)); } - if (isSimpleNameStaticallyImported(methodName, state)) { - return Optional.of(String.format("`%s` is already statically imported", methodName)); + if (isSimpleNameStaticallyImported(newName, state)) { + return Optional.of(String.format("`%s` is already statically imported", newName)); } - if (isReservedKeyword(methodName)) { - return Optional.of(String.format("`%s` is a reserved keyword", methodName)); + 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) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java index 4b403a02fb..35febeec3f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java @@ -29,7 +29,12 @@ private StringRules() {} static final class StringIsEmpty { @BeforeTemplate boolean before(String str) { - return Refaster.anyOf(str.length() == 0, str.length() <= 0, str.length() < 1); + return str.equals(""); + } + + @BeforeTemplate + boolean before2(String str) { + return str.length() == 0; } @AfterTemplate 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 0d9d069d74..1082f7653a 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 @@ -74,8 +74,9 @@ void identification() { "", " void method5TestCases() {}", "", - " // BUG: Diagnostic contains: (but note that a method named `method5TestCases` already exists in", - " // this class)", + " // BUG: Diagnostic contains: The test cases should be supplied by a method named", + " // `method5TestCases` (but note that a method named `method5TestCases` 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\"));", 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 index abf0140891..ea670d824b 100644 --- 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 @@ -6,7 +6,9 @@ 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 org.junit.jupiter.api.Test; @@ -23,8 +25,8 @@ void matcher() { " foo3t();", " }", "", - " // BUG: Diagnostic contains: [RenameBlockerFlagger] a method named `foo2t` already exists in this", - " // class", + " // BUG: Diagnostic contains: [RenameBlockerFlagger] a method named `foo2t` is already defined in", + " // this class or a supertype", " private void foo2() {}", "", " private void foo2t() {}", @@ -32,7 +34,7 @@ void matcher() { " // BUG: Diagnostic contains: [RenameBlockerFlagger] `foo3t` is already statically imported", " private void foo3() {}", "", - " // BUG: Diagnostic contains: [RenameBlockerFlagger] `int` is a reserved keyword", + " // BUG: Diagnostic contains: [RenameBlockerFlagger] `int` is not a valid identifier", " private void in() {}", "}") .addSourceLines( @@ -46,13 +48,13 @@ void matcher() { } @BugPattern(summary = "Flags blockers for renaming methods", severity = ERROR) - public static final class RenameBlockerFlagger extends BugChecker - implements BugChecker.MethodTreeMatcher { + 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(tree.getName() + "t", state) + return ConflictDetection.findMethodRenameBlocker( + ASTHelpers.getSymbol(tree), tree.getName() + "t", state) .map(blocker -> buildDescription(tree).setMessage(blocker).build()) .orElse(Description.NO_MATCH); } From 79b2b51a00866b7cfa175d2e59a87c4d42e76930 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 12 Jan 2023 21:55:24 +0100 Subject: [PATCH 12/14] Suggestions --- .../bugpatterns/JUnitFactoryMethodDeclaration.java | 2 +- .../errorprone/refasterrules/StringRules.java | 7 +------ .../JUnitFactoryMethodDeclarationTest.java | 13 ++++--------- 3 files changed, 6 insertions(+), 16 deletions(-) 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 e353cca4ca..705819de30 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 @@ -47,7 +47,7 @@ import tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers; /** - * A {@link BugChecker} that flags non-canonical JUnit factory method declarations. + * A {@link BugChecker} that flags non-canonical JUnit factory method declarations and usages. * *

At Picnic, we consider a JUnit factory method canonical if it: * diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java index 35febeec3f..4b403a02fb 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java @@ -29,12 +29,7 @@ private StringRules() {} static final class StringIsEmpty { @BeforeTemplate boolean before(String str) { - return str.equals(""); - } - - @BeforeTemplate - boolean before2(String str) { - return str.length() == 0; + return Refaster.anyOf(str.length() == 0, str.length() <= 0, str.length() < 1); } @AfterTemplate 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 1082f7653a..38d81fee6d 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 @@ -6,14 +6,9 @@ import org.junit.jupiter.api.Test; final class JUnitFactoryMethodDeclarationTest { - private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()); - private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()); - @Test void identification() { - compilationTestHelper + CompilationTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()) .addSourceLines( "A.java", "import static org.junit.jupiter.params.provider.Arguments.arguments;", @@ -74,8 +69,8 @@ void identification() { "", " void method5TestCases() {}", "", - " // BUG: Diagnostic contains: The test cases should be supplied by a method named", - " // `method5TestCases` (but note that a method named `method5TestCases` already defined in this", + " // 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 } */", @@ -147,7 +142,7 @@ void identification() { @Test void replacement() { - refactoringTestHelper + BugCheckerRefactoringTestHelper.newInstance(JUnitFactoryMethodDeclaration.class, getClass()) .addInputLines( "A.java", "import static org.junit.jupiter.params.provider.Arguments.arguments;", From a2626ee8bf83029b216bb7c2551eac8effc874b5 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 13 Jan 2023 10:44:49 +0100 Subject: [PATCH 13/14] More minor tweaks --- .../util/ConflictDetectionTest.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) 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 index ea670d824b..9f6ce68dd5 100644 --- 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 @@ -10,6 +10,7 @@ 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 { @@ -18,28 +19,28 @@ void matcher() { CompilationTestHelper.newInstance(RenameBlockerFlagger.class, getClass()) .addSourceLines( "/A.java", - "import static staticimport.B.foo3t;", + "import static pkg.B.foo3t;", "", "class A {", " private void foo1() {", " foo3t();", " }", "", - " // BUG: Diagnostic contains: [RenameBlockerFlagger] a method named `foo2t` is already defined in", - " // this class or a supertype", + " // 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: [RenameBlockerFlagger] `foo3t` is already statically imported", + " // BUG: Diagnostic contains: `foo3t` is already statically imported", " private void foo3() {}", "", - " // BUG: Diagnostic contains: [RenameBlockerFlagger] `int` is not a valid identifier", + " // BUG: Diagnostic contains: `int` is not a valid identifier", " private void in() {}", "}") .addSourceLines( - "/staticimport/B.java", - "package staticimport;", + "/pkg/B.java", + "package pkg;", "", "public class B {", " public static void foo3t() {}", @@ -47,6 +48,10 @@ void matcher() { .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; From fc6af3337d746d28fcf3e664bd8ea828cb6413fe Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 13 Jan 2023 12:58:23 +0100 Subject: [PATCH 14/14] Extract and improve method --- .../JUnitFactoryMethodDeclaration.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 705819de30..92cb800f65 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 @@ -67,7 +67,6 @@ 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")), @@ -167,13 +166,7 @@ private void reportMethodRenameBlocker( private ImmutableList getReturnStatementCommentFixes( MethodTree testMethod, MethodTree factoryMethod, VisitorState state) { - ImmutableList parameterNames = - testMethod.getParameters().stream() - .map(VariableTree::getName) - .map(Object::toString) - .collect(toImmutableList()); - - String expectedComment = parameterNames.stream().collect(joining(", ", "/* { ", " } */")); + String expectedComment = createCommentContainingParameters(testMethod); List statements = factoryMethod.getBody().getStatements(); @@ -201,6 +194,13 @@ private ImmutableList getReturnStatementCommentFixes( .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,