new JUnit5SuiteMisuse error-prone check

README.md

@@ -168,6 +168,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
 - `SafeLoggingExceptionMessageFormat`: SafeLoggable exceptions do not interpolate parameters.
 - `StrictUnusedVariable`: Functions shouldn't have unused parameters.
 - `StringBuilderConstantParameters`: StringBuilder with a constant number of parameters should be replaced by simple concatenation.
+- `JUnit5SuiteMisuse`: When migrating from JUnit4 -> JUnit5, classes annotated with `@RunWith(Suite.class)` are dangerous because if they reference any JUnit5 test classes, these tests will silently not run!
 
 ## com.palantir.baseline-checkstyle
 Checkstyle rules can be suppressed on a per-line or per-block basis. (It is good practice to first consider formatting

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsage.java

@@ -32,6 +32,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "DangerousJsonTypeInfoUsage",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.ERROR,
         summary = "Disallow usage of Jackson's JsonTypeInfo.Id.CLASS annotation for security reasons, "
                 + "cf. https://github.com/FasterXML/jackson-databind/issues/1599")

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousParallelStreamUsage.java

@@ -30,6 +30,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "DangerousParallelStreamUsage",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.WARNING,
         summary = "Discourage usage of .parallel() in Java streams.")
 public final class DangerousParallelStreamUsage extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousStringInternUsage.java

@@ -30,6 +30,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "DangerousStringInternUsage",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.WARNING,
         summary = "Disallow String.intern() invocations.")
 public final class DangerousStringInternUsage extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThreadPoolExecutorUsage.java

@@ -19,6 +19,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "DangerousThreadPoolExecutorUsage",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.ERROR,
         summary = "Disallow direct ThreadPoolExecutor usages.")
 public final class DangerousThreadPoolExecutorUsage extends BugChecker implements BugChecker.NewClassTreeMatcher {

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormat.java

@@ -31,6 +31,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "GuavaPreconditionsMessageFormat",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.ERROR,
         summary = "Guava Preconditions.checkX() methods must use print-f style formatting.")
 public final class GuavaPreconditionsMessageFormat extends PreconditionsMessageFormat {

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JUnit5RuleUsage.java

@@ -31,6 +31,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "JUnit5RuleUsage",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = BugPattern.SeverityLevel.ERROR,
         summary = "Using Rule/ClassRules in Junit5 tests results in the rules silently not executing")
 public final class JUnit5RuleUsage extends BugChecker implements BugChecker.ClassTreeMatcher {
@@ -41,7 +43,7 @@ public final class JUnit5RuleUsage extends BugChecker implements BugChecker.Clas
             "org.junit.jupiter.migrationsupport.rules.EnableRuleMigrationSupport";
 
     private static final Matcher<ClassTree> hasMigrationSupport = Matchers.hasAnnotation(RULE_MIGRATION_SUPPORT);
-    private static final Matcher<ClassTree> hasJunit5TestCases =
+    static final Matcher<ClassTree> hasJunit5TestCases =
             Matchers.hasMethod(Matchers.hasAnnotationOnAnyOverriddenMethod(JUNIT5_TEST_ANNOTATION));
     private static final Matcher<ClassTree> hasJunit4Rules = hasVariable(
             Matchers.anyOf(hasAnnotationOnVariable(JUNIT4_CLASS_RULE), hasAnnotationOnVariable(JUNIT4_RULE)));

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JUnit5SuiteMisuse.java

@@ -0,0 +1,117 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.baseline.errorprone;
+
+import com.google.auto.service.AutoService;
+import com.google.common.collect.ImmutableList;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.matchers.AnnotationMatcherUtils;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matchers;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.AnnotationTree;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.tree.JCTree;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+@AutoService(BugChecker.class)
+@BugPattern(
+        name = "JUnit5SuiteMisuse",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
+        severity = BugPattern.SeverityLevel.ERROR,
+        summary = "Referencing JUnit5 tests from JUnit4 Suites will silently not work")
+public final class JUnit5SuiteMisuse extends BugChecker
+        implements BugChecker.ClassTreeMatcher, BugChecker.AnnotationTreeMatcher {
+
+    private static final long serialVersionUID = 1L;
+
+    // We remember classes and validate them later because error-prone doesn't let us arbitrarily explore classes we
+    // discover when reading the @SuiteClasses annotation.
+    private static final Set<Type.ClassType> knownJUnit5TestClasses = new HashSet<>();
+    private static final Set<Type.ClassType> referencedBySuites = new HashSet<>();
+
+    @Override
+    public Description matchClass(ClassTree tree, VisitorState state) {
+        if (!JUnit5RuleUsage.hasJunit5TestCases.matches(tree, state)) {
+            return Description.NO_MATCH;
+        }
+
+        Type.ClassType type = ASTHelpers.getType(tree);
+        knownJUnit5TestClasses.add(type); // accumulate these so we can check them when visiting suiteclasses
+
+        if (referencedBySuites.contains(type)) {
+            return buildDescription(tree)
+                    .setMessage("Class uses JUnit5 tests but is referenced by a JUnit4 SuiteClasses")
+                    .build();
+        }
+
+        return Description.NO_MATCH;
+    }
+
+    @Override
+    public Description matchAnnotation(AnnotationTree tree, VisitorState state) {
+        if (!Matchers.isSameType("org.junit.runners.Suite.SuiteClasses").matches(tree, state)) {
+            return Description.NO_MATCH;
+        }
+
+        for (JCTree.JCFieldAccess field : getReferencedClasses(tree, state)) {
+            Type.ClassType classType = (Type.ClassType) field.selected.type;
+            referencedBySuites.add(classType);
+
+            if (knownJUnit5TestClasses.contains(classType)) {
+                return buildDescription(tree)
+                        .setMessage("Don't reference JUnit5 test classes from JUnit4 SuiteClasses annotation")
+                        .build();
+            }
+        }
+
+        return Description.NO_MATCH;
+    }
+
+    private static List<JCTree.JCFieldAccess> getReferencedClasses(AnnotationTree tree, VisitorState state) {
+        ExpressionTree value = AnnotationMatcherUtils.getArgument(tree, "value");
+
+        if (value == null) {
+            return Collections.emptyList();
+        }
+
+        if (value instanceof JCTree.JCFieldAccess) {
+            return Collections.singletonList((JCTree.JCFieldAccess) value);
+        }
+
+        if (value instanceof JCTree.JCNewArray) {
+            ImmutableList.Builder<JCTree.JCFieldAccess> list = ImmutableList.builder();
+            for (JCTree.JCExpression elem : ((JCTree.JCNewArray) value).elems) {
+                list.add((JCTree.JCFieldAccess) elem);
+            }
+            return list.build();
+        }
+
+        throw new UnsupportedOperationException(String.format(
+                "Unable to get referenced classes for %s of type %s",
+                state.getSourceForNode(tree),
+                value.getClass()));
+    }
+}

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NonComparableStreamSort.java

@@ -34,6 +34,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "NonComparableStreamSort",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.ERROR,
         summary = "Stream.sorted() should only be called on streams of Comparable types.")
 public final class NonComparableStreamSort extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreconditionsConstantMessage.java

@@ -33,6 +33,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "PreconditionsConstantMessage",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.ERROR,
         summary = "Allow only constant messages to Preconditions.checkX() methods")
 public final class PreconditionsConstantMessage extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StringBuilderConstantParameters.java

@@ -45,6 +45,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "StringBuilderConstantParameters",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.WARNING,
         summary = "StringBuilder with a constant number of parameters should be replaced by simple concatenation")
 public final class StringBuilderConstantParameters

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ValidateConstantMessage.java

@@ -34,6 +34,8 @@
 @AutoService(BugChecker.class)
 @BugPattern(
         name = "ValidateConstantMessage",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
         severity = SeverityLevel.ERROR,
         summary = "Allow only constant messages to Validate.X() methods")
 public final class ValidateConstantMessage extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JUnit5SuiteMisuseTest.java

@@ -0,0 +1,174 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.baseline.errorprone;
+
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.Before;
+import org.junit.Test;
+
+public class JUnit5SuiteMisuseTest {
+
+    private CompilationTestHelper compilationHelper;
+
+    @Before
+    public void before() {
+        compilationHelper = CompilationTestHelper.newInstance(JUnit5SuiteMisuse.class, getClass());
+    }
+
+    @Test
+    public void multiple_junit4_references_pass() {
+        compilationHelper.addSourceLines(
+                "Container.java",
+                "import org.junit.runner.RunWith;",
+                "import org.junit.runners.Suite;",
+                "class Container {",
+                "  @RunWith(Suite.class)",
+                "  @Suite.SuiteClasses({FooTest.class, BarTest.class})",
+                "  public static class MySuite {}",
+                "",
+                "  public static class FooTest {",
+                "    @org.junit.Test public void my_test() {}",
+                "  }",
+                "",
+                "  public static class BarTest {",
+                "    @org.junit.Test public void my_test() {}",
+                "  }",
+                "}")
+                .doTest();
+    }
+
+    @Test
+    public void single_junit4_reference_passes() {
+        compilationHelper.addSourceLines(
+                "Container.java",
+                "import org.junit.runner.RunWith;",
+                "import org.junit.runners.Suite;",
+                "class Container {",
+                "  @RunWith(Suite.class)",
+                "  @Suite.SuiteClasses(FooTest.class)",
+                "  public static class MySuite {}",
+                "",
+                "  public static class FooTest {",
+                "    @org.junit.Test public void my_test() {}",
+                "  }",
+                "}")
+                .doTest();
+    }
+
+    @Test
+    public void single_junit4_reference_passes_different_order() {
+        compilationHelper.addSourceLines(
+                "Container.java",
+                "import org.junit.runner.RunWith;",
+                "import org.junit.runners.Suite;",
+                "class Container {",
+                "  public static class FooTest {",
+                "    @org.junit.Test public void my_test() {}",
+                "  }",
+                "",
+                "  @RunWith(Suite.class)",
+                "  @Suite.SuiteClasses(FooTest.class)",
+                "  public static class MySuite {}",
+                "}")
+                .doTest();
+    }
+
+    @Test
+    public void multiple_junit5_references_fail() {
+        compilationHelper.addSourceLines(
+                "Container.java",
+                "import org.junit.runner.RunWith;",
+                "import org.junit.runners.Suite;",
+                "class Container {",
+                "  @RunWith(Suite.class)",
+                "  @Suite.SuiteClasses({FooTest.class, BarTest.class})",
+                "  public static class MySuite {}",
+                "",
+                "  // BUG: Diagnostic contains: JUnit5SuiteMisuse",
+                "  public static class FooTest {",
+                "    @org.junit.jupiter.api.Test public void my_test() {}",
+                "  }",
+                "",
+                "  // BUG: Diagnostic contains: JUnit5SuiteMisuse",
+                "  public static class BarTest {",
+                "    @org.junit.jupiter.api.Test public void my_test() {}",
+                "  }",
+                "}")
+                .doTest();
+    }
+
+    @Test
+    public void multiple_junit5_references_fail_different_order() {
+        compilationHelper.addSourceLines(
+                "Container.java",
+                "import org.junit.runner.RunWith;",
+                "import org.junit.runners.Suite;",
+                "class Container {",
+                "  public static class FooTest {",
+                "    @org.junit.jupiter.api.Test public void my_test() {}",
+                "  }",
+                "",
+                "  public static class BarTest {",
+                "    @org.junit.jupiter.api.Test public void my_test() {}",
+                "  }",
+                "",
+                "  @RunWith(Suite.class)",
+                "  // BUG: Diagnostic contains: JUnit5SuiteMisuse",
+                "  @Suite.SuiteClasses({FooTest.class, BarTest.class})",
+                "  public static class MySuite {}",
+                "}")
+                .doTest();
+    }
+
+    @Test
+    public void single_junit5_reference_fails() {
+        compilationHelper.addSourceLines(
+                "Container.java",
+                "import org.junit.runner.RunWith;",
+                "import org.junit.runners.Suite;",
+                "class Container {",
+                "  @RunWith(Suite.class)",
+                "  @Suite.SuiteClasses(FooTest.class)",
+                "  public static class MySuite {}",
+                "",
+                "  // BUG: Diagnostic contains: JUnit5SuiteMisuse",
+                "  public static class FooTest {",
+                "    @org.junit.jupiter.api.Test public void my_test() {}",
+                "  }",
+                "}")
+                .doTest();
+    }
+
+    @Test
+    public void single_junit5_reference_fails_different_order() {
+        compilationHelper.addSourceLines(
+                "Container.java",
+                "import org.junit.runner.RunWith;",
+                "import org.junit.runners.Suite;",
+                "class Container {",
+                "  public static class FooTest {",
+                "    @org.junit.jupiter.api.Test public void my_test() {}",
+                "  }",
+                "",
+                "  @RunWith(Suite.class)",
+                "  // BUG: Diagnostic contains: JUnit5SuiteMisuse",
+                "  @Suite.SuiteClasses(FooTest.class)",
+                "  public static class MySuite {}",
+                "}")
+                .doTest();
+    }
+}