PreferAssertj disallows assert statements in test code.

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

@@ -21,6 +21,7 @@
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.auto.service.AutoService;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.errorprone.BugPattern;
@@ -35,6 +36,7 @@
 import com.google.errorprone.predicates.TypePredicate;
 import com.google.errorprone.predicates.TypePredicates;
 import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.AssertTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.ImportTree;
 import com.sun.source.tree.MemberSelectTree;
@@ -47,6 +49,7 @@
 import java.util.Optional;
 import java.util.function.BiConsumer;
 import java.util.stream.Collectors;
+import javax.annotation.Nullable;
 import javax.lang.model.type.TypeKind;
 
 /**
@@ -63,7 +66,8 @@
         providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
         severity = BugPattern.SeverityLevel.SUGGESTION,
         summary = "Prefer AssertJ fluent assertions")
-public final class PreferAssertj extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {
+public final class PreferAssertj
+        extends BugChecker implements BugChecker.AssertTreeMatcher, BugChecker.MethodInvocationTreeMatcher {
 
     private static final ImmutableSet<String> LEGACY_ASSERT_CLASSES = ImmutableSet.of(
             "org.hamcrest.MatcherAssert",
@@ -413,6 +417,40 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
         return Description.NO_MATCH;
     }
 
+    @Override
+    public Description matchAssert(AssertTree tree, VisitorState state) {
+        if (!TestCheckUtils.isTestCode(state)) {
+            return Description.NO_MATCH;
+        }
+        SuggestedFix.Builder fix = SuggestedFix.builder();
+        return buildDescription(tree)
+                .setMessage("Prefer AssertJ fluent assertions instead of the 'assert' keyword in tests. "
+                        + "Assertions can be disabled which is never expected in tests.")
+                .addFix(fix.replace(tree, String.format("%s(%s)%s.isTrue();",
+                        qualifyAssertThat(fix, state),
+                        state.getSourceForNode(tree.getCondition()),
+                        getAssertDescription(tree.getDetail(), state)))
+                        .build())
+                .build();
+    }
+
+    /** Returns the describedAs invocation if a detail is present, otherwise an empty string. */
+    private static String getAssertDescription(@Nullable ExpressionTree detail, VisitorState state) {
+        if (detail == null) {
+            return "";
+        }
+        String detailSource = state.getSourceForNode(detail);
+        if (Strings.isNullOrEmpty(detailSource)) {
+            return "";
+        }
+        if (state.getTypes().isAssignable(
+                ASTHelpers.getResultType(detail), state.getTypeFromString(String.class.getName()))) {
+            return ".describedAs(" + detailSource + ')';
+        }
+        // Lazily allow the value to be evaluated on failures
+        return ".describedAs(\"%s\", " + detailSource + ')';
+    }
+
     /**
      * Provides a qualified 'assertThat' name. We attempt to use a static import if possible, otherwise fall back
      * to as qualified as possible.
@@ -423,16 +461,7 @@ private Description withAssertThat(
             int actualIndex,
             BiConsumer<String, SuggestedFix.Builder> assertThat) {
         SuggestedFix.Builder fix = SuggestedFix.builder();
-        String qualified;
-        if (useStaticAssertjImport(state)) {
-            fix.removeStaticImport("org.junit.Assert.assertThat")
-                    .removeStaticImport("org.hamcrest.MatcherAssert.assertThat")
-                    .addStaticImport("org.assertj.core.api.Assertions.assertThat");
-            qualified = "assertThat";
-        } else {
-            qualified = SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.Assertions.assertThat");
-        }
-
+        String qualified = qualifyAssertThat(fix, state);
         String actualArgumentString = argSource(tree, state, actualIndex);
         ExpressionTree actualArgument = tree.getArguments().get(actualIndex);
         if (isIterableMap(actualArgument, state)) {
@@ -446,6 +475,17 @@ private Description withAssertThat(
                 .build();
     }
 
+    private static String qualifyAssertThat(SuggestedFix.Builder fix, VisitorState state) {
+        if (useStaticAssertjImport(state)) {
+            fix.removeStaticImport("org.junit.Assert.assertThat")
+                    .removeStaticImport("org.hamcrest.MatcherAssert.assertThat")
+                    .addStaticImport("org.assertj.core.api.Assertions.assertThat");
+            return "assertThat";
+        } else {
+            return SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.Assertions.assertThat");
+        }
+    }
+
     private static boolean isIterableMap(ExpressionTree value, VisitorState state) {
         return isSubtype(value, "java.lang.Iterable", state)
                 && isSubtype(value, "java.util.Map", state);

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

@@ -41,6 +41,9 @@ public void fix_booleanAsserts() {
                         "    Assert.assertTrue(b);",
                         "    Assert.assertTrue(\"desc\", b);",
                         "    assertThat(\"desc\", b);",
+                        "    assert b;",
+                        "    assert b : \"desc\";",
+                        "    assert b : 123;",
                         "  }",
                         "}")
                 .addOutputLines(
@@ -55,6 +58,9 @@ public void fix_booleanAsserts() {
                         "    assertThat(b).isTrue();",
                         "    assertThat(b).describedAs(\"desc\").isTrue();",
                         "    assertThat(b).describedAs(\"desc\").isTrue();",
+                        "    assertThat(b).isTrue();",
+                        "    assertThat(b).describedAs(\"desc\").isTrue();",
+                        "    assertThat(b).describedAs(\"%s\", 123).isTrue();",
                         "  }",
                         "}")
                 .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
@@ -950,6 +956,19 @@ public void fix_assert_iterableMap() {
                 .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
 
+    @Test
+    public void assertsIgnoredInProductionCode() {
+        test().addInputLines("Test.java",
+                "class Test {",
+                "  void execute(String input) {",
+                "    assert input != null : \"input should not be null\";",
+                "    assert input.length() == 5;",
+                "  }",
+                "}")
+                .expectUnchanged()
+                .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+    }
+
     private RefactoringValidator test() {
         return RefactoringValidator.of(new PreferAssertj(), getClass());
     }

changelog/@unreleased/pr-1052.v2.yml

@@ -0,0 +1,8 @@
+type: improvement
+improvement:
+  description: |-
+    PreferAssertj disallows `assert` statements in test code.
+
+    Tests should use more specific AssertJ checks, which cannot be disabled by turning off asserts. Arguably the `assert` keyword should never be used, preferring preconditions. This way production environments cannot reach code paths that are impossible to test.
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/1052