From 329e315858f3a8b23aab64f486fff86360abec79 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Mon, 24 Oct 2022 14:55:04 +0200 Subject: [PATCH] Improvements --- .../bugpatterns/JUnitValueSource.java | 45 ++++++++++++------- .../bugpatterns/JUnitValueSourceTest.java | 35 +++++++++++++-- 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java index 9e13c1a7c2..ea672bf35b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java @@ -22,7 +22,6 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; -import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; @@ -35,10 +34,11 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.ReturnTree; -import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; import com.sun.tools.javac.code.Type; import java.util.Optional; import java.util.stream.Stream; +import javax.lang.model.type.TypeKind; import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** @@ -46,6 +46,9 @@ * org.junit.jupiter.params.provider.MethodSource} that can be written as a {@link * org.junit.jupiter.params.provider.ValueSource}. */ +// XXX: Support rewriting when there are multiple sources defined for the `@MethodSource`, iff +// applicable. +// XXX: Don't remove factory methods that are used by another `@MethodSource`. @AutoService(BugChecker.class) @BugPattern( summary = @@ -77,15 +80,20 @@ public Description matchMethod(MethodTree tree, VisitorState state) { AnnotationTree annotationTree = ASTHelpers.getAnnotationWithSimpleName( tree.getModifiers().getAnnotations(), "MethodSource"); - Optional fix = tryConstructValueSourceFix(parameterType, annotationTree, state); - return fix.isPresent() ? describeMatch(tree, fix.orElseThrow()) : Description.NO_MATCH; + return tryConstructValueSourceFix(parameterType, annotationTree, state) + .map(fix -> describeMatch(tree, fix.build())) + .orElse(Description.NO_MATCH); } - private static Optional tryConstructValueSourceFix( + private static Optional tryConstructValueSourceFix( Type parameterType, AnnotationTree methodSourceAnnotation, VisitorState state) { - String factoryMethodName = extractFactoryMethodName(methodSourceAnnotation); - MethodTree factoryMethod = getFactoryMethod(factoryMethodName, state); + Optional factoryMethodName = extractSingleFactoryMethodName(methodSourceAnnotation); + if (factoryMethodName.isEmpty()) { + /* `@MethodSource` defines more than one source. */ + return Optional.empty(); + } + MethodTree factoryMethod = findFactoryMethod(factoryMethodName.orElseThrow(), state); Optional valueSourceAttributeValue = getReturnTree(factoryMethod) @@ -104,8 +112,7 @@ private static Optional tryConstructValueSourceFix( String.format( "@ValueSource(%s = {%s})", toValueSourceAttributeName(parameterType.toString()), attributeValue)) - .delete(factoryMethod) - .build()); + .delete(factoryMethod)); } private static Optional getReturnTree(MethodTree methodTree) { @@ -115,14 +122,18 @@ private static Optional getReturnTree(MethodTree methodTree) { .map(ReturnTree.class::cast); } - private static String extractFactoryMethodName(AnnotationTree methodSourceAnnotation) { - ExpressionTree expression = + private static Optional extractSingleFactoryMethodName( + AnnotationTree methodSourceAnnotation) { + ExpressionTree attributeExpression = ((AssignmentTree) Iterables.getOnlyElement(methodSourceAnnotation.getArguments())) .getExpression(); - return ASTHelpers.getType(expression).stringValue(); + Type attributeType = ASTHelpers.getType(attributeExpression); + return attributeType.getKind() == TypeKind.ARRAY + ? Optional.empty() + : Optional.of(attributeType.stringValue()); } - private static MethodTree getFactoryMethod(String factoryMethodName, VisitorState state) { + private static MethodTree findFactoryMethod(String factoryMethodName, VisitorState state) { return state.findEnclosing(ClassTree.class).getMembers().stream() .filter(MethodTree.class::isInstance) .map(MethodTree.class::cast) @@ -133,7 +144,7 @@ private static MethodTree getFactoryMethod(String factoryMethodName, VisitorStat private static Optional extractArgumentsFromStream( MethodInvocationTree tree, VisitorState state) { - ImmutableList args = + ImmutableList arguments = tree.getArguments().stream() .filter(MethodInvocationTree.class::isInstance) .map(MethodInvocationTree.class::cast) @@ -143,10 +154,10 @@ private static Optional extractArgumentsFromStream( .collect(toImmutableList()); /* Not all values are compile-time constants. */ - if (args.size() != tree.getArguments().size()) { + if (arguments.size() != tree.getArguments().size()) { return Optional.empty(); } - return Optional.of(String.join(", ", args)); + return Optional.of(String.join(", ", arguments)); } private static String toValueSourceAttributeName(String type) { @@ -163,7 +174,7 @@ private static String toValueSourceAttributeName(String type) { } private static boolean isCompileTimeConstant(ExpressionTree argument) { - return argument.getKind() == Tree.Kind.MEMBER_SELECT + return argument.getKind() == Kind.MEMBER_SELECT ? ((MemberSelectTree) argument).getIdentifier().contentEquals("class") : ASTHelpers.constValue(argument) != null; } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java index c2ca30c896..7ad25251cb 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java @@ -11,8 +11,6 @@ final class JUnitValueSourceTest { private final BugCheckerRefactoringTestHelper refactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance(JUnitValueSource.class, getClass()); - // XXX: Add a test case for when a factory is used by more than one test. - @Test void identificationChar() { compilationTestHelper @@ -182,7 +180,38 @@ void identificationNoRuntimeParameters() { } @Test - void identificationConstantValues() { + void identificationDontFlagForMultipleFactories() { + compilationTestHelper + .addSourceLines( + "A.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "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({\"fooTestCases\", \"barTestCases\"})", + " void foo(int i) {", + " assertThat(i).isNotNull();", + " }", + "", + " private static Stream fooTestCases() {", + " return Stream.of(arguments(1));", + " }", + "", + " private static Stream barTestCases() {", + " return Stream.of(arguments(1));", + " }", + "}") + .doTest(); + } + + @Test + void identificationOnlyConstantValues() { compilationTestHelper .addSourceLines( "A.java",