Address review comments

PicnicSupermarket · Nov 4, 2022 · 93576b4 · 93576b4
1 parent 7d6b4c3
commit 93576b4
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 23 deletions.
diff --git a/...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java b/...ntrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
@@ -18,8 +18,8 @@
 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.JUnit.HAS_METHOD_SOURCE;
-import static tech.picnic.errorprone.bugpatterns.util.JUnit.TEST_METHOD;
+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;
@@ -46,15 +46,20 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Stream;
-import tech.picnic.errorprone.bugpatterns.util.JUnit;
 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>A canonical JUnit factory method is one which - 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.
+ * <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`
+ *       suffix, and
+ *   <li>has a comment which connects the return statement to the names of the parameters in the
+ *       corresponding test method.
+ * </ul>
  */
 @AutoService(BugChecker.class)
 @BugPattern(
@@ -92,7 +97,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
     }
 
     Optional<String> factoryMethodName =
-        JUnit.extractSingleFactoryMethodName(methodSourceAnnotation);
+        MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation);
 
     if (factoryMethodName.isEmpty()) {
       /* If a test has multiple factory methods, not all of them can be given the desired name. */

diff --git a/...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java b/...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java
@@ -13,8 +13,8 @@
 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.JUnit.SETUP_OR_TEARDOWN_METHOD;
-import static tech.picnic.errorprone.bugpatterns.util.JUnit.TEST_METHOD;
+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.auto.service.AutoService;
 import com.google.common.collect.ImmutableSet;

diff --git a/...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java b/...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java
@@ -1,14 +1,12 @@
 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.ClassTree;
 import com.sun.source.tree.ImportTree;
-import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.Tree;
 import java.util.Optional;
-import javax.lang.model.element.Name;
 
 /**
  * A set of helper methods for finding conflicts which would be caused by applying certain fixes.
@@ -54,15 +52,6 @@ public static Optional<String> findMethodRenameBlocker(String methodName, Visito
     return Optional.empty();
   }
 
-  private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {
-    return state.findEnclosing(ClassTree.class).getMembers().stream()
-        .filter(MethodTree.class::isInstance)
-        .map(MethodTree.class::cast)
-        .map(MethodTree::getName)
-        .map(Name::toString)
-        .anyMatch(methodName::equals);
-  }
-
   private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) {
     return state.getPath().getCompilationUnit().getImports().stream()
         .filter(ImportTree::isStatic)

diff --git a/...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java b/...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java
@@ -3,8 +3,10 @@
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
 import com.google.common.collect.ImmutableList;
+import com.google.errorprone.VisitorState;
 import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.MethodTree;
+import javax.lang.model.element.Name;
 
 /** A set of helper methods for working with the AST. */
 public final class MoreASTHelpers {
@@ -24,4 +26,13 @@ public static ImmutableList<MethodTree> findMethods(ClassTree enclosingClass, St
         .filter(method -> method.getName().contentEquals(methodName))
         .collect(toImmutableList());
   }
+
+  static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {
+    return state.findEnclosing(ClassTree.class).getMembers().stream()
+        .filter(MethodTree.class::isInstance)
+        .map(MethodTree.class::cast)
+        .map(MethodTree::getName)
+        .map(Name::toString)
+        .anyMatch(methodName::equals);
+  }
 }
diff --git a/...ic/errorprone/bugpatterns/util/JUnit.java → ...e/bugpatterns/util/MoreJUnitMatchers.java b/...ic/errorprone/bugpatterns/util/JUnit.java → ...e/bugpatterns/util/MoreJUnitMatchers.java
@@ -20,7 +20,7 @@
 import javax.lang.model.type.TypeKind;
 
 /** A set of JUnit-specific helpers for working with the AST. */
-public final class JUnit {
+public final class MoreJUnitMatchers {
   /** Matches JUnit test methods. */
   public static final MultiMatcher<MethodTree, AnnotationTree> TEST_METHOD =
       annotations(
@@ -45,7 +45,7 @@ public final class JUnit {
   public static final Matcher<MethodTree> HAS_METHOD_SOURCE =
       allOf(annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")));
 
-  private JUnit() {}
+  private MoreJUnitMatchers() {}
 
   /**
    * Extracts the name of the JUnit factory method from a {@link