From 30e9220a57cbc995dc7de0202ca777250fc1903c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Tue, 10 Oct 2023 17:44:00 +0200 Subject: [PATCH] Suggestions --- .../bugpatterns/AutowiredConstructor.java | 12 +- ...itNullaryParameterizedTestDeclaration.java | 91 ++++++++++ .../JUnitParameterizedMethodDeclaration.java | 48 ------ ...llaryParameterizedTestDeclarationTest.java | 155 ++++++++++++++++++ ...nitParameterizedMethodDeclarationTest.java | 30 ---- 5 files changed, 251 insertions(+), 85 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclaration.java delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclaration.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclarationTest.java delete mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclarationTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java index 0d9eaee043e..dc8e0b73228 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java @@ -9,7 +9,6 @@ import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -17,6 +16,7 @@ import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.MultiMatcher; +import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; @@ -48,11 +48,9 @@ public Description matchClass(ClassTree tree, VisitorState state) { return Description.NO_MATCH; } - ImmutableList annotations = - AUTOWIRED_ANNOTATION - .multiMatchResult(Iterables.getOnlyElement(constructors), state) - .matchingNodes(); - if (annotations.size() != 1) { + MultiMatchResult hasAutowiredAnnotation = + AUTOWIRED_ANNOTATION.multiMatchResult(Iterables.getOnlyElement(constructors), state); + if (!hasAutowiredAnnotation.matches()) { return Description.NO_MATCH; } @@ -61,7 +59,7 @@ public Description matchClass(ClassTree tree, VisitorState state) { * means that the associated import can be removed as well. Rather than adding code for this * case we leave flagging the unused import to Error Prone's `RemoveUnusedImports` check. */ - AnnotationTree annotation = Iterables.getOnlyElement(annotations); + AnnotationTree annotation = hasAutowiredAnnotation.onlyMatchingNode(); return describeMatch(annotation, SourceCode.deleteWithTrailingWhitespace(annotation, state)); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclaration.java new file mode 100644 index 00000000000..c9112f3c9ff --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclaration.java @@ -0,0 +1,91 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +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 tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; +import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.hasMetaAnnotation; + +import com.google.auto.service.AutoService; +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.matchers.MultiMatcher; +import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.MethodTree; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; + +/** + * A {@link BugChecker} that flags nullary {@link + * org.junit.jupiter.params.ParameterizedTest @ParameterizedTest} test methods. + * + *

Such tests are unnecessarily executed more than necessary. This checker suggests annotating + * the method with {@link org.junit.jupiter.api.Test @Test}, and to drop all declared {@link + * org.junit.jupiter.params.provider.ArgumentsSource argument sources}. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Nullary JUnit test methods need not be parameterized", + link = BUG_PATTERNS_BASE_URL + "JUnitNullaryParameterizedTestDeclaration", + linkType = CUSTOM, + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class JUnitNullaryParameterizedTestDeclaration extends BugChecker + implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final MultiMatcher IS_PARAMETERIZED_TEST = + annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.ParameterizedTest")); + private static final Matcher IS_ARGUMENT_SOURCE = + anyOf( + isType("org.junit.jupiter.params.provider.ArgumentsSource"), + isType("org.junit.jupiter.params.provider.ArgumentsSources"), + hasMetaAnnotation("org.junit.jupiter.params.provider.ArgumentsSource")); + + /** Instantiates a new {@link JUnitNullaryParameterizedTestDeclaration} instance. */ + public JUnitNullaryParameterizedTestDeclaration() {} + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (!tree.getParameters().isEmpty()) { + return Description.NO_MATCH; + } + + MultiMatchResult isParameterizedTest = + IS_PARAMETERIZED_TEST.multiMatchResult(tree, state); + if (!isParameterizedTest.matches()) { + return Description.NO_MATCH; + } + + /* + * This method is vacuously parameterized. Suggest replacing `@ParameterizedTest` with `@Test`. + * (As each method is checked independently, we cannot in general determine whether this + * suggestion makes a `ParameterizedTest` type import obsolete; that task is left to Error + * Prone's `RemoveUnusedImports` check.) + */ + SuggestedFix.Builder fix = SuggestedFix.builder(); + fix.merge( + SuggestedFix.replace( + isParameterizedTest.onlyMatchingNode(), + '@' + SuggestedFixes.qualifyType(state, fix, "org.junit.jupiter.api.Test"))); + + /* + * Also suggest dropping all (explicit and implicit) `@ArgumentsSource`s. No attempt is made to + * assess whether a dropped `@MethodSource` also makes the referenced factory method(s) unused. + */ + tree.getModifiers().getAnnotations().stream() + .filter(a -> IS_ARGUMENT_SOURCE.matches(a, state)) + .forEach(a -> fix.merge(SourceCode.deleteWithTrailingWhitespace(a, state))); + + return describeMatch(tree, fix.build()); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclaration.java deleted file mode 100644 index 4ba2eec526b..00000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclaration.java +++ /dev/null @@ -1,48 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import static com.google.errorprone.BugPattern.LinkType.CUSTOM; -import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; -import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; -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.isType; -import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; - -import com.google.auto.service.AutoService; -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.matchers.Description; -import com.google.errorprone.matchers.Matcher; -import com.sun.source.tree.MethodTree; - -/** - * A {@link BugChecker} that flags test methods using {@link - * org.junit.jupiter.params.ParameterizedTest} without actually having any arguments. - */ -@AutoService(BugChecker.class) -@BugPattern( - summary = "JUnit parameterized test used without arguments", - link = BUG_PATTERNS_BASE_URL + "JUnitParameterizedMethodDeclaration", - linkType = CUSTOM, - severity = SUGGESTION, - tags = SIMPLIFICATION) -public final class JUnitParameterizedMethodDeclaration extends BugChecker - implements MethodTreeMatcher { - private static final long serialVersionUID = 1L; - private static final Matcher IS_PARAMETERIZED_TEST = - annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.ParameterizedTest")); - - /** Instantiates a new {@link JUnitParameterizedMethodDeclaration} instance. */ - public JUnitParameterizedMethodDeclaration() {} - - @Override - public Description matchMethod(MethodTree tree, VisitorState state) { - if (IS_PARAMETERIZED_TEST.matches(tree, state) && tree.getParameters().isEmpty()) { - return describeMatch(tree); - } - - return Description.NO_MATCH; - } -} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclarationTest.java new file mode 100644 index 00000000000..ba46a7d4f11 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitNullaryParameterizedTestDeclarationTest.java @@ -0,0 +1,155 @@ +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 JUnitNullaryParameterizedTestDeclarationTest { + @Test + void identification() { + CompilationTestHelper.newInstance(JUnitNullaryParameterizedTestDeclaration.class, getClass()) + .addSourceLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.ValueSource;", + "", + "class A {", + " void nonTest() {}", + "", + " @Test", + " void nonParameterizedTest() {}", + "", + " @ParameterizedTest", + " @ValueSource(ints = {0, 1})", + " void goodParameterizedTest(int someInt) {}", + "", + " @ParameterizedTest", + " @ValueSource(ints = {0, 1})", + " // BUG: Diagnostic contains:", + " void nullaryParameterizedTest() {}", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance( + JUnitNullaryParameterizedTestDeclaration.class, getClass()) + .addInputLines( + "A.java", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.ArgumentsProvider;", + "import org.junit.jupiter.params.provider.ArgumentsSource;", + "import org.junit.jupiter.params.provider.ArgumentsSources;", + "import org.junit.jupiter.params.provider.MethodSource;", + "import org.junit.jupiter.params.provider.ValueSource;", + "", + "class A {", + " @ParameterizedTest", + " void withoutArgumentSource() {}", + "", + " @ParameterizedTest", + " @ArgumentsSource(ArgumentsProvider.class)", + " void withCustomArgumentSource() {}", + "", + " @ParameterizedTest", + " @ArgumentsSources({", + " @ArgumentsSource(ArgumentsProvider.class),", + " @ArgumentsSource(ArgumentsProvider.class)", + " })", + " void withCustomerArgumentSources() {}", + "", + " /** Foo. */", + " @ParameterizedTest", + " @ValueSource(ints = {0, 1})", + " void withValueSourceAndJavadoc() {}", + "", + " @ParameterizedTest", + " @MethodSource(\"nonexistentMethod\")", + " @SuppressWarnings(\"foo\")", + " void withMethodSourceAndUnrelatedAnnotation() {}", + "", + " @org.junit.jupiter.params.ParameterizedTest", + " @ArgumentsSource(ArgumentsProvider.class)", + " @ValueSource(ints = {0, 1})", + " @MethodSource(\"nonexistentMethod\")", + " void withMultipleArgumentSourcesAndFullyQualifiedImport() {}", + "", + " class NestedWithTestAnnotationFirst {", + " @ParameterizedTest", + " @ValueSource(ints = {0, 1})", + " void withValueSource() {}", + " }", + "", + " class NestedWithTestAnnotationSecond {", + " @ValueSource(ints = {0, 1})", + " @ParameterizedTest", + " void withValueSource() {}", + " }", + "}") + .addOutputLines( + "A.java", + "import org.junit.jupiter.api.Test;", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.ArgumentsProvider;", + "import org.junit.jupiter.params.provider.ArgumentsSource;", + "import org.junit.jupiter.params.provider.ArgumentsSources;", + "import org.junit.jupiter.params.provider.MethodSource;", + "import org.junit.jupiter.params.provider.ValueSource;", + "", + "class A {", + " @Test", + " void withoutArgumentSource() {}", + "", + " @Test", + " void withCustomArgumentSource() {}", + "", + " @Test", + " void withCustomerArgumentSources() {}", + "", + " /** Foo. */", + " @Test", + " void withValueSourceAndJavadoc() {}", + "", + " @Test", + " @SuppressWarnings(\"foo\")", + " void withMethodSourceAndUnrelatedAnnotation() {}", + "", + " @Test", + " void withMultipleArgumentSourcesAndFullyQualifiedImport() {}", + "", + " class NestedWithTestAnnotationFirst {", + " @Test", + " void withValueSource() {}", + " }", + "", + " class NestedWithTestAnnotationSecond {", + " @Test", + " void withValueSource() {}", + " }", + "}") + .addInputLines( + "B.java", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "class B {", + " @ParameterizedTest", + " void scopeInWhichIdentifierTestIsAlreadyDeclared() {}", + "", + " class Test {}", + "}") + .addOutputLines( + "B.java", + "import org.junit.jupiter.params.ParameterizedTest;", + "", + "class B {", + " @org.junit.jupiter.api.Test", + " void scopeInWhichIdentifierTestIsAlreadyDeclared() {}", + "", + " class Test {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclarationTest.java deleted file mode 100644 index 14e26f79693..00000000000 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitParameterizedMethodDeclarationTest.java +++ /dev/null @@ -1,30 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import com.google.errorprone.CompilationTestHelper; -import org.junit.jupiter.api.Test; - -final class JUnitParameterizedMethodDeclarationTest { - @Test - void identification() { - CompilationTestHelper.newInstance(JUnitParameterizedMethodDeclaration.class, getClass()) - .addSourceLines( - "A.java", - "import org.junit.jupiter.api.Test;", - "import org.junit.jupiter.params.ParameterizedTest;", - "", - "class A {", - " @Test", - " void test() {}", - "", - " @ParameterizedTest", - " // BUG: Diagnostic contains:", - " void badParameterizedTest() {}", - "", - " @ParameterizedTest", - " void goodParameterizedTest(Object someArgument) {}", - "", - " void nonTest() {}", - "}") - .doTest(); - } -}