diff --git a/README.md b/README.md index 457983bb4..45e69b4b0 100644 --- a/README.md +++ b/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! - `PreferAssertj`: Prefer AssertJ fluent assertions. ## com.palantir.baseline-checkstyle diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsage.java index 2530720b7..07c5244cf 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousJsonTypeInfoUsage.java +++ b/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") diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousParallelStreamUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousParallelStreamUsage.java index 4fa157949..0e47193ef 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousParallelStreamUsage.java +++ b/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 { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousStringInternUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousStringInternUsage.java index f664f775d..8a8799968 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousStringInternUsage.java +++ b/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 { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThreadPoolExecutorUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThreadPoolExecutorUsage.java index 6a7f5aad5..9aa56d5eb 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThreadPoolExecutorUsage.java +++ b/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 { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormat.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormat.java index 887845561..27683acfb 100755 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GuavaPreconditionsMessageFormat.java +++ b/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 { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JUnit5RuleUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JUnit5RuleUsage.java index bb7288991..90e45efa2 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JUnit5RuleUsage.java +++ b/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 hasMigrationSupport = Matchers.hasAnnotation(RULE_MIGRATION_SUPPORT); - private static final Matcher hasJunit5TestCases = + static final Matcher hasJunit5TestCases = Matchers.hasMethod(Matchers.hasAnnotationOnAnyOverriddenMethod(JUNIT5_TEST_ANNOTATION)); private static final Matcher hasJunit4Rules = hasVariable( Matchers.anyOf(hasAnnotationOnVariable(JUNIT4_CLASS_RULE), hasAnnotationOnVariable(JUNIT4_RULE))); diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JUnit5SuiteMisuse.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/JUnit5SuiteMisuse.java new file mode 100644 index 000000000..aa0e2de5b --- /dev/null +++ b/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 knownJUnit5TestClasses = new HashSet<>(); + private static final Set 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 annotation") + .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 (Type referencedClass : getReferencedClasses(tree, state)) { + Type.ClassType classType = (Type.ClassType) referencedClass; + 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 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).selected.type); + } + + if (value instanceof JCTree.JCNewArray) { + ImmutableList.Builder list = ImmutableList.builder(); + for (JCTree.JCExpression elem : ((JCTree.JCNewArray) value).elems) { + list.add(((JCTree.JCFieldAccess) elem).selected.type); + } + return list.build(); + } + + throw new UnsupportedOperationException(String.format( + "Unable to get referenced classes for %s of type %s", + state.getSourceForNode(tree), + value.getClass())); + } +} diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NonComparableStreamSort.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NonComparableStreamSort.java index dac3cef8b..d282dd7fb 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/NonComparableStreamSort.java +++ b/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 { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreconditionsConstantMessage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreconditionsConstantMessage.java index 05e1a9e36..4f3b1237e 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreconditionsConstantMessage.java +++ b/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 { diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StringBuilderConstantParameters.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StringBuilderConstantParameters.java index b0dd8bda1..c55649a75 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StringBuilderConstantParameters.java +++ b/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 diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ValidateConstantMessage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ValidateConstantMessage.java index 30e4685c8..ff601b3ce 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ValidateConstantMessage.java +++ b/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 { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JUnit5SuiteMisuseTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/JUnit5SuiteMisuseTest.java new file mode 100644 index 000000000..b793d1aed --- /dev/null +++ b/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(); + } +} diff --git a/changelog/@unreleased/pr-843.v2.yml b/changelog/@unreleased/pr-843.v2.yml new file mode 100644 index 000000000..9f6a51fc6 --- /dev/null +++ b/changelog/@unreleased/pr-843.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: ErrorProne will now detect dangerous usage of `@RunWith(Suite.class)` + that references JUnit5 classes, as this can cause tests to silently not run! + links: + - https://github.com/palantir/gradle-baseline/pull/843