From 3494a88b90eb0860bdd13864e1edf58efbe099db Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 13 Nov 2022 17:37:30 +0100 Subject: [PATCH] Suggestions (2) --- .../bugpatterns/util/MoreJUnitMatchers.java | 22 ++- .../util/MoreJUnitMatchersTest.java | 129 ++++++++++++++---- .../bugpatterns/util/MoreMatchersTest.java | 15 +- 3 files changed, 121 insertions(+), 45 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 276b1c00a6..0c39a6db92 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 @@ -5,8 +5,10 @@ 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.errorprone.matchers.AnnotationMatcherUtils; import com.google.errorprone.matchers.Matcher; @@ -16,6 +18,7 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.NewArrayTree; +import org.jspecify.nullness.Nullable; /** * A collection of JUnit-specific helper methods and {@link Matcher}s. @@ -43,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 = + public static final MultiMatcher HAS_METHOD_SOURCE = annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")); private MoreJUnitMatchers() {} @@ -57,17 +60,22 @@ private MoreJUnitMatchers() {} */ 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(ASTHelpers.constValue(value, String.class)); + return ImmutableSet.of(toMethodSourceFactoryName(value, methodName)); } - NewArrayTree arrayTree = (NewArrayTree) value; - return arrayTree.getInitializers().isEmpty() - ? ImmutableSet.of(method.getName().toString()) - : arrayTree.getInitializers().stream() - .map(name -> ASTHelpers.constValue(name, String.class)) + 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); + } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java index 9b56cfaa04..caab722793 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java @@ -1,7 +1,14 @@ package tech.picnic.errorprone.bugpatterns.util; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.HAS_METHOD_SOURCE; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.SETUP_OR_TEARDOWN_METHOD; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.TEST_METHOD; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.VisitorState; @@ -9,15 +16,15 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.MethodTree; -import java.util.ArrayList; -import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Test; final class MoreJUnitMatchersTest { @Test - void matches() { - CompilationTestHelper.newInstance(TestChecker.class, getClass()) + void methodMatchers() { + CompilationTestHelper.newInstance(MethodMatchersTestChecker.class, getClass()) .addSourceLines( "A.java", "import static org.junit.jupiter.params.provider.Arguments.arguments;", @@ -36,24 +43,24 @@ void matches() { "class A {", " @BeforeAll", " // BUG: Diagnostic contains: SETUP_OR_TEARDOWN_METHOD", - " public void setUp1() {}", + " public void beforeAll() {}", "", " @BeforeEach", " @Test", " // BUG: Diagnostic contains: TEST_METHOD, SETUP_OR_TEARDOWN_METHOD", - " protected void setUp2() {}", + " protected void beforeEachAndTest() {}", "", " @AfterEach", " // BUG: Diagnostic contains: SETUP_OR_TEARDOWN_METHOD", - " private void tearDown1() {}", + " private void afterEach() {}", "", " @AfterAll", " // BUG: Diagnostic contains: SETUP_OR_TEARDOWN_METHOD", - " private void tearDown2() {}", + " private void afterAll() {}", "", " @Test", " // BUG: Diagnostic contains: TEST_METHOD", - " void testFoo() {}", + " void test() {}", "", " private static Stream booleanArgs() {", " return Stream.of(arguments(false), arguments(true));", @@ -62,43 +69,105 @@ void matches() { " @ParameterizedTest", " @MethodSource(\"booleanArgs\")", " // BUG: Diagnostic contains: TEST_METHOD, HAS_METHOD_SOURCE", - " void testBar(boolean b) {}", + " void parameterizedTest(boolean b) {}", "", " @RepeatedTest(2)", " // BUG: Diagnostic contains: TEST_METHOD", - " private void qux() {}", + " private void repeatedTest() {}", "", - " private void quux() {}", + " private void unannotatedMethod() {}", + "}") + .doTest(); + } + + @Test + void getMethodSourceFactoryNames() { + CompilationTestHelper.newInstance(MethodSourceFactoryNamesTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "import org.junit.jupiter.params.provider.MethodSource;", + "", + "class A {", + " @MethodSource", + " // BUG: Diagnostic contains: [matchingMethodSource]", + " void matchingMethodSource(boolean b) {}", + "", + " @MethodSource()", + " // BUG: Diagnostic contains: [matchingMethodSourceWithParens]", + " void matchingMethodSourceWithParens(boolean b) {}", + "", + " @MethodSource(\"\")", + " // BUG: Diagnostic contains: [matchingMethodSourceMadeExplicit]", + " void matchingMethodSourceMadeExplicit(boolean b) {}", + "", + " @MethodSource({\"\"})", + " // BUG: Diagnostic contains: [matchingMethodSourceMadeExplicitWithParens]", + " void matchingMethodSourceMadeExplicitWithParens(boolean b) {}", + "", + " @MethodSource({})", + " // BUG: Diagnostic contains: []", + " void noMethodSources(boolean b) {}", + "", + " @MethodSource(\"myValueFactory\")", + " // BUG: Diagnostic contains: [myValueFactory]", + " void singleCustomMethodSource(boolean b) {}", + "", + " @MethodSource({\"firstValueFactory\", \"secondValueFactory\"})", + " // BUG: Diagnostic contains: [firstValueFactory, secondValueFactory]", + " void twoCustomMethodSources(boolean b) {}", + "", + " @MethodSource({\"myValueFactory\", \"\"})", + " // BUG: Diagnostic contains: [myValueFactory, customAndMatchingMethodSources]", + " void customAndMatchingMethodSources(boolean b) {}", "}") .doTest(); } /** - * A {@link BugChecker} that delegates to {@link Matcher Matchers} from {@link MoreJUnitMatchers}. + * A {@link BugChecker} that flags methods matched by {@link Matcher}s of {@link MethodTree}s + * exposed by {@link MoreJUnitMatchers}. */ - @BugPattern( - summary = "Flags methods matched by Matchers from `MoreJUnitMatchers`", - severity = ERROR) - public static final class TestChecker extends BugChecker implements MethodTreeMatcher { + @BugPattern(summary = "Interacts with `MoreJUnitMatchers` for testing purposes", severity = ERROR) + public static final class MethodMatchersTestChecker extends BugChecker + implements MethodTreeMatcher { private static final long serialVersionUID = 1L; + private static final ImmutableMap> METHOD_MATCHERS = + ImmutableMap.of( + "TEST_METHOD", TEST_METHOD, + "HAS_METHOD_SOURCE", HAS_METHOD_SOURCE, + "SETUP_OR_TEARDOWN_METHOD", SETUP_OR_TEARDOWN_METHOD); @Override public Description matchMethod(MethodTree tree, VisitorState state) { - List diagnosticsMessages = new ArrayList<>(); - - if (MoreJUnitMatchers.TEST_METHOD.matches(tree, state)) { - diagnosticsMessages.add("TEST_METHOD"); - } - if (MoreJUnitMatchers.HAS_METHOD_SOURCE.matches(tree, state)) { - diagnosticsMessages.add("HAS_METHOD_SOURCE"); - } - if (MoreJUnitMatchers.SETUP_OR_TEARDOWN_METHOD.matches(tree, state)) { - diagnosticsMessages.add("SETUP_OR_TEARDOWN_METHOD"); - } + ImmutableSet matches = + METHOD_MATCHERS.entrySet().stream() + .filter(e -> e.getValue().matches(tree, state)) + .map(Map.Entry::getKey) + .collect(toImmutableSet()); - return diagnosticsMessages.isEmpty() + return matches.isEmpty() ? Description.NO_MATCH - : buildDescription(tree).setMessage(String.join(", ", diagnosticsMessages)).build(); + : buildDescription(tree).setMessage(String.join(", ", matches)).build(); + } + } + + /** + * A {@link BugChecker} that flags methods with a JUnit {@code @MethodSource} annotation by + * enumerating the associated value factory method names. + */ + @BugPattern(summary = "Interacts with `MoreJUnitMatchers` for testing purposes", severity = ERROR) + public static final class MethodSourceFactoryNamesTestChecker extends BugChecker + implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + AnnotationTree annotation = + Iterables.getOnlyElement(HAS_METHOD_SOURCE.multiMatchResult(tree, state).matchingNodes()); + + return buildDescription(tree) + .setMessage(MoreJUnitMatchers.getMethodSourceFactoryNames(annotation, tree).toString()) + .build(); } } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java index 34789d4426..c42ab96723 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java @@ -14,10 +14,10 @@ final class MoreMatchersTest { @Test - void matcher() { + void hasMetaAnnotation() { CompilationTestHelper.newInstance(TestMatcher.class, getClass()) .addSourceLines( - "/A.java", + "A.java", "import org.junit.jupiter.api.AfterAll;", "import org.junit.jupiter.api.RepeatedTest;", "import org.junit.jupiter.api.Test;", @@ -25,7 +25,7 @@ void matcher() { "import org.junit.jupiter.params.ParameterizedTest;", "", "class A {", - " private void negative1() {}", + " void negative1() {}", "", " @Test", " void negative2() {}", @@ -38,26 +38,25 @@ void matcher() { "", " // BUG: Diagnostic contains:", " @ParameterizedTest", - " void testBar() {}", + " void positive1() {}", "", " // BUG: Diagnostic contains:", " @RepeatedTest(2)", - " void testBaz() {}", + " void positive2() {}", "}") .doTest(); } - /** A {@link BugChecker} that delegates to `MoreMatchers#hasMetaAnnotation`. */ + /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasMetaAnnotation(String)} . */ @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR) public static final class TestMatcher extends BugChecker implements AnnotationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher DELEGATE = MoreMatchers.hasMetaAnnotation("org.junit.jupiter.api.TestTemplate"); @Override public Description matchAnnotation(AnnotationTree tree, VisitorState state) { - return DELEGATE.matches(tree, state) ? buildDescription(tree).build() : Description.NO_MATCH; + return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH; } } }