diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java index e55873fc6..74e4a6283 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java +++ b/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 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 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); diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferAssertjTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferAssertjTests.java index 7b773e800..35aeabeb3 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferAssertjTests.java +++ b/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()); } diff --git a/changelog/@unreleased/pr-1052.v2.yml b/changelog/@unreleased/pr-1052.v2.yml new file mode 100644 index 000000000..92f456899 --- /dev/null +++ b/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