From fe8a4c27e8df6b76222b132fc62c6fd90ee91815 Mon Sep 17 00:00:00 2001
From: Rick Ossendrijver <rick.ossendrijver@gmail.com>
Date: Thu, 10 Nov 2022 21:31:35 +0100
Subject: [PATCH] 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.
  *
- * <p>At Picnic, we consider a JUnit factory method canonical if it
+ * <p>At Picnic, we consider a JUnit factory method canonical if it:
  *
  * <ul>
  *   <li>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<String> factoryMethodName =
-        MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation);
+    Optional<ImmutableList<MethodTree>> 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<MethodTree> 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<Description> descriptions =
+        ImmutableList.<Description>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<Description> 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<Description> getSuggestedFixes(
-      MethodTree tree,
-      AnnotationTree methodSourceAnnotation,
-      MethodTree factoryMethod,
-      VisitorState state) {
-    ImmutableList<Description> factoryMethodNameFixes =
-        getFactoryMethodNameFixes(tree, methodSourceAnnotation, factoryMethod, state);
-
-    ImmutableList<Description> commentFixes =
-        getReturnStatementCommentFixes(tree, factoryMethod, state);
-
-    return ImmutableList.<Description>builder()
-        .addAll(factoryMethodNameFixes)
-        .addAll(commentFixes)
-        .build();
-  }
-
   private ImmutableList<Description> 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<Description> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> method6TestCases() {",
-            "    List<Arguments> 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<Arguments> method7TestCases() {",
-            "    List<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> 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<Arguments> method6TestCases() {",
-            "    List<Arguments> 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<Arguments> method7TestCases() {",
-            "    List<Arguments> arguments = List.of(arguments(1, true, \"A\"), arguments(2, false, \"B\"));",
-            "    /* { foo, bar, baz } */",
-            "    return arguments.stream();",
-            "  }",
             "}")
         .doTest(TestMode.TEXT_MATCH);
   }