From 3edb5c1cf466320abd86dde5bcefbf9bc1d427ba Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Sep 2019 14:18:21 -0400 Subject: [PATCH 1/7] PreferAssertj provides better hamcrest fixes Avoids using the less ergonomic HamcrestCondition in common cases. --- .../baseline/errorprone/PreferAssertj.java | 161 ++++++++- .../errorprone/PreferAssertjTests.java | 309 ++++++++++++++++-- 2 files changed, 446 insertions(+), 24 deletions(-) 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 273fa9cca..5fca46745 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 @@ -36,8 +36,11 @@ import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; +import com.sun.source.util.SimpleTreeVisitor; import java.util.List; +import java.util.Optional; import java.util.function.BiConsumer; +import java.util.stream.Collectors; /** * {@link PreferAssertj} provides an automated path from legacy test libraries to AssertJ. Our goal is to migrate @@ -310,18 +313,25 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState + argSource(tree, state, 1) + ", within(" + argSource(tree, state, 3) + "))")); } if (ASSERT_THAT.matches(tree, state)) { + Optional replacement = tree.getArguments().get(1) + .accept(HamcrestVisitor.INSTANCE, state); return withAssertThat(tree, state, (assertThat, fix) -> - fix.replace(tree, assertThat + "(" + argSource(tree, state, 0) + ").is(new " + fix.replace(tree, assertThat + "(" + argSource(tree, state, 0) + ")" + + replacement.orElseGet(() -> + ".is(new " + SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition") + "<>(" - + argSource(tree, state, 1) + "))")); + + argSource(tree, state, 1) + "))"))); } if (ASSERT_THAT_DESCRIPTION.matches(tree, state)) { + Optional replacement = tree.getArguments().get(2) + .accept(HamcrestVisitor.INSTANCE, state); return withAssertThat(tree, state, (assertThat, fix) -> fix.replace(tree, assertThat + "(" + argSource(tree, state, 1) - + ").describedAs(" + argSource(tree, state, 0) + ").is(new " + + ").describedAs(" + argSource(tree, state, 0) + ")" + + replacement.orElseGet(() -> ".is(new " + SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition") - + "<>(" + argSource(tree, state, 2) + "))")); + + "<>(" + argSource(tree, state, 2) + "))"))); } if (ASSERT_EQUALS_CATCHALL.matches(tree, state)) { int parameters = tree.getArguments().size(); @@ -427,4 +437,147 @@ private static String argSource(MethodInvocationTree invocation, VisitorState st checkArgument(index < arguments.size(), "Index is out of bounds"); return checkNotNull(state.getSourceForNode(arguments.get(index)), "Failed to find argument source"); } + + private static final class HamcrestVisitor extends SimpleTreeVisitor, VisitorState> { + private static final HamcrestVisitor INSTANCE = new HamcrestVisitor(false); + + private static final HamcrestVisitor NEGATED = new HamcrestVisitor(true); + + private static final ImmutableSet MATCHERS = ImmutableSet.of( + "org.hamcrest.Matchers", + "org.hamcrest.CoreMatchers", + "org.hamcrest.core.IsIterableContaining"); + + private static final Matcher IS_MATCHER = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .named("is") + .withParameters("org.hamcrest.Matcher"); + + private static final Matcher NOT_MATCHER = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .named("not") + .withParameters("org.hamcrest.Matcher"); + + private static final Matcher EQUALS = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("is", "equalTo", "equalToObject") + .withParameters(Object.class.getName()); + + private static final Matcher INSTANCE_OF = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("isA", "instanceOf") + .withParameters("java.lang.Class"); + + private static final Matcher NULL = Matchers.anyOf( + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .named("nullValue") + .withParameters(), + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .named("nullValue") + .withParameters("java.lang.Class")); + + private static final Matcher NOT_NULL = Matchers.anyOf( + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .named("notNullValue") + .withParameters(), + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .named("notNullValue") + .withParameters("java.lang.Class")); + + private static final Matcher HAS_ITEM = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("hasItem", "hasItemInArray") + .withParameters(Object.class.getName()); + + // Note: cannot match array/vararg arguments + private static final Matcher HAS_ITEMS = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("hasItems", "arrayContainingInAnyOrder"); + + private static final Matcher IS_EMPTY = Matchers.anyOf( + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("empty", "emptyIterable", "emptyArray", "anEmptyMap") + .withParameters(), + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("emptyCollectionOf", "emptyIterableOf") + .withParameters(Class.class.getName())); + + private static final Matcher HAS_SIZE = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("hasSize", "aMapWithSize", "arrayWithSize", "iterableWithSize") + .withParameters(int.class.getName()); + + private static final Matcher SAME_INSTANCE = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("sameInstance", "theInstance") + .withParameters(Object.class.getName()); + + private final boolean negated; + + private HamcrestVisitor(boolean negated) { + super(Optional.empty()); + this.negated = negated; + } + + @Override + @SuppressWarnings("CyclomaticComplexity") + public Optional visitMethodInvocation(MethodInvocationTree node, VisitorState state) { + // is(Matcher) is for readability, fall through to the next matcher + if (IS_MATCHER.matches(node, state)) { + return node.getArguments().get(0).accept(this, state); + } + if (NOT_MATCHER.matches(node, state)) { + return node.getArguments().get(0).accept(this.negated ? INSTANCE : NEGATED, state); + } + if (EQUALS.matches(node, state)) { + return Optional.of((negated ? ".isNotEqualTo(" : ".isEqualTo(") + + argSource(node, state, 0) + ")"); + } + if (INSTANCE_OF.matches(node, state)) { + return Optional.of((negated ? ".isNotInstanceOf(" : ".isInstanceOf(") + + argSource(node, state, 0) + ")"); + } + if (NULL.matches(node, state)) { + return Optional.of(negated ? ".isNotNull()" : ".isNull()"); + } + if (NOT_NULL.matches(node, state)) { + return Optional.of(negated ? ".isNull()" : ".isNotNull()"); + } + if (HAS_ITEM.matches(node, state)) { + return Optional.of((negated ? ".doesNotContain(" : ".contains(") + argSource(node, state, 0) + ')'); + } + if (IS_EMPTY.matches(node, state)) { + return Optional.of(negated ? ".isNotEmpty()" : ".isEmpty()"); + } + if (SAME_INSTANCE.matches(node, state)) { + return Optional.of((negated ? ".isNotSameAs(" : ".isSameAs(") + argSource(node, state, 0) + ')'); + } + if (HAS_SIZE.matches(node, state)) { + if (negated) { + // No top level method for negated size assertions + return Optional.empty(); + } + + return Optional.of(".hasSize(" + argSource(node, state, 0) + ')'); + } + if (HAS_ITEMS.matches(node, state) + && checkNotNull(ASTHelpers.getSymbol(node), "symbol").isVarArgs()) { + if (negated) { + // this negates to 'doesNotContainAll' which doesn't exist. AssertJ doesNotContain + // evaluates as 'does not contain any'. + return Optional.empty(); + } + return Optional.of(".contains(" + node.getArguments().stream() + .map(state::getSourceForNode) + .collect(Collectors.joining(", ")) + ')'); + } + return Optional.empty(); + } + } } 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 cda5aa7d5..187ac0170 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 @@ -159,11 +159,10 @@ public void fixAssertFalse_existingAssertThat() { "Test.java", "import static org.assertj.core.api.Assertions.assertThat;", "", - "import org.assertj.core.api.HamcrestCondition;", "import org.junit.Assert;", "class Test {", " void foo(boolean b) {", - " assertThat(true).is(new HamcrestCondition<>(org.hamcrest.CoreMatchers.equalTo(false)));", + " assertThat(true).isEqualTo(false);", " assertThat(b).isFalse();", " }", "}") @@ -962,11 +961,9 @@ public void fix_assertThatInt() { "Test.java", "import static org.assertj.core.api.Assertions.assertThat;", "import static org.hamcrest.Matchers.is;", - "", - "import org.assertj.core.api.HamcrestCondition;", "class Test {", " void foo(int value) {", - " assertThat(value).is(new HamcrestCondition<>(is(1)));", + " assertThat(value).isEqualTo(1);", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); @@ -988,63 +985,335 @@ public void fix_assertThatIntDescription() { "Test.java", "import static org.assertj.core.api.Assertions.assertThat;", "import static org.hamcrest.Matchers.is;", - "", - "import org.assertj.core.api.HamcrestCondition;", "class Test {", " void foo(int value) {", - " assertThat(value).describedAs(\"desc\").is(new HamcrestCondition<>(is(1)));", + " assertThat(value).describedAs(\"desc\").isEqualTo(1);", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test - public void fix_matcherAssertThatString() { + public void fix_matcherAssertThatString_startsWith() { test() .addInputLines( "Test.java", "import static org.hamcrest.MatcherAssert.assertThat;", - "import static org.hamcrest.Matchers.is;", + "import static org.hamcrest.Matchers.startsWith;", "class Test {", " void foo(String value) {", - " assertThat(value, is(\"str\"));", + " assertThat(value, startsWith(\"str\"));", " }", "}") .addOutputLines( "Test.java", "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.hamcrest.Matchers.is;", + "import static org.hamcrest.Matchers.startsWith;", "", "import org.assertj.core.api.HamcrestCondition;", "class Test {", " void foo(String value) {", - " assertThat(value).is(new HamcrestCondition<>(is(\"str\")));", + " assertThat(value).is(new HamcrestCondition<>(startsWith(\"str\")));", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } @Test - public void fix_matcherAssertThatStringDescription() { + public void fix_assertThat_instanceOf() { test() .addInputLines( "Test.java", "import static org.hamcrest.MatcherAssert.assertThat;", - "import static org.hamcrest.Matchers.is;", + "import static org.hamcrest.Matchers.*;", "class Test {", " void foo(String value) {", - " assertThat(\"desc\", value, is(\"str\"));", + " assertThat(value, instanceOf(String.class));", " }", "}") .addOutputLines( "Test.java", "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.hamcrest.Matchers.is;", - "", - "import org.assertj.core.api.HamcrestCondition;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value).isInstanceOf(String.class);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void fix_assertThat_is_instanceOf() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value, is(instanceOf(String.class)));", + " assertThat(value, instanceOf(String.class));", + " assertThat(value, is(not(instanceOf(String.class))));", + " assertThat(value, not(instanceOf(String.class)));", + " assertThat(\"desc\", value, is(instanceOf(String.class)));", + " assertThat(\"desc\", value, instanceOf(String.class));", + " assertThat(\"desc\", value, is(not(instanceOf(String.class))));", + " assertThat(\"desc\", value, not(instanceOf(String.class)));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value).isInstanceOf(String.class);", + " assertThat(value).isInstanceOf(String.class);", + " assertThat(value).isNotInstanceOf(String.class);", + " assertThat(value).isNotInstanceOf(String.class);", + " assertThat(value).describedAs(\"desc\").isInstanceOf(String.class);", + " assertThat(value).describedAs(\"desc\").isInstanceOf(String.class);", + " assertThat(value).describedAs(\"desc\").isNotInstanceOf(String.class);", + " assertThat(value).describedAs(\"desc\").isNotInstanceOf(String.class);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void fix_assertThat_equality() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value, is(\"str\"));", + " assertThat(value, equalTo(\"str\"));", + " assertThat(value, is(equalTo(\"str\")));", + " assertThat(value, org.hamcrest.CoreMatchers.equalToObject(\"str\"));", + " assertThat(value, not(not(equalTo(\"str\"))));", + " assertThat(value, not(is(\"str\")));", + " assertThat(value, not(equalTo(\"str\")));", + " assertThat(value, is(not(equalTo(\"str\"))));", + " assertThat(value, not(org.hamcrest.CoreMatchers.equalToObject(\"str\")));", + " assertThat(value, not(not(not(equalTo(\"str\")))));", + " assertThat(\"desc\", value, not(not(not(equalTo(\"str\")))));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value).isEqualTo(\"str\");", + " assertThat(value).isEqualTo(\"str\");", + " assertThat(value).isEqualTo(\"str\");", + " assertThat(value).isEqualTo(\"str\");", + " assertThat(value).isEqualTo(\"str\");", + " assertThat(value).isNotEqualTo(\"str\");", + " assertThat(value).isNotEqualTo(\"str\");", + " assertThat(value).isNotEqualTo(\"str\");", + " assertThat(value).isNotEqualTo(\"str\");", + " assertThat(value).isNotEqualTo(\"str\");", + " assertThat(value).describedAs(\"desc\").isNotEqualTo(\"str\");", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void fix_assertThat_nullness() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value, is(not(is(notNullValue(String.class)))));", + " assertThat(value, is(not(is(nullValue(String.class)))));", + " assertThat(value, is(nullValue(String.class)));", + " assertThat(value, nullValue());", + " assertThat(value, is(notNullValue(String.class)));", + " assertThat(value, notNullValue());", + " assertThat(\"desc\", value, is(not(is(notNullValue(String.class)))));", + " assertThat(\"desc\", value, is(not(is(nullValue(String.class)))));", + " assertThat(\"desc\", value, is(nullValue(String.class)));", + " assertThat(\"desc\", value, nullValue());", + " assertThat(\"desc\", value, is(notNullValue(String.class)));", + " assertThat(\"desc\", value, notNullValue());", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", "class Test {", " void foo(String value) {", - " assertThat(value).describedAs(\"desc\").is(new HamcrestCondition<>(is(\"str\")));", + " assertThat(value).isNull();", + " assertThat(value).isNotNull();", + " assertThat(value).isNull();", + " assertThat(value).isNull();", + " assertThat(value).isNotNull();", + " assertThat(value).isNotNull();", + " assertThat(value).describedAs(\"desc\").isNull();", + " assertThat(value).describedAs(\"desc\").isNotNull();", + " assertThat(value).describedAs(\"desc\").isNull();", + " assertThat(value).describedAs(\"desc\").isNull();", + " assertThat(value).describedAs(\"desc\").isNotNull();", + " assertThat(value).describedAs(\"desc\").isNotNull();", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void fix_assertThat_contains() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(Iterable value, String[] arrayValue) {", + " assertThat(value, hasItem(\"str\"));", + " assertThat(arrayValue, hasItemInArray(\"str\"));", + " assertThat(value, hasItems(\"one\", \"two\"));", + " assertThat(arrayValue, arrayContainingInAnyOrder(\"one\", \"two\"));", + " assertThat(value, not(hasItem(\"str\")));", + " assertThat(arrayValue, not(hasItemInArray(\"str\")));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(Iterable value, String[] arrayValue) {", + " assertThat(value).contains(\"str\");", + " assertThat(arrayValue).contains(\"str\");", + " assertThat(value).contains(\"one\", \"two\");", + " assertThat(arrayValue).contains(\"one\", \"two\");", + " assertThat(value).doesNotContain(\"str\");", + " assertThat(arrayValue).doesNotContain(\"str\");", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void fix_assertThat_empty() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "", + "import java.util.*;", + "class Test {", + " void foo(Iterable it, String[] ar, List li) {", + " assertThat(li, empty());", + " assertThat(li, emptyIterable());", + " assertThat(li, emptyCollectionOf(String.class));", + " assertThat(it, emptyIterable());", + " assertThat(it, emptyIterableOf(String.class));", + " assertThat(ar, emptyArray());", + " assertThat(li, is(not(empty())));", + " assertThat(li, not(emptyIterable()));", + " assertThat(li, not(emptyCollectionOf(String.class)));", + " assertThat(it, not(emptyIterable()));", + " assertThat(it, not(emptyIterableOf(String.class)));", + " assertThat(ar, not(emptyArray()));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", + "", + "import java.util.*;", + "class Test {", + " void foo(Iterable it, String[] ar, List li) {", + " assertThat(li).isEmpty();", + " assertThat(li).isEmpty();", + " assertThat(li).isEmpty();", + " assertThat(it).isEmpty();", + " assertThat(it).isEmpty();", + " assertThat(ar).isEmpty();", + " assertThat(li).isNotEmpty();", + " assertThat(li).isNotEmpty();", + " assertThat(li).isNotEmpty();", + " assertThat(it).isNotEmpty();", + " assertThat(it).isNotEmpty();", + " assertThat(ar).isNotEmpty();", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void fix_assertThat_size() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "", + "import java.util.*;", + "class Test {", + " void foo(Iterable it, String[] ar, List li) {", + " assertThat(li, hasSize(3));", + " assertThat(li, iterableWithSize(3));", + " assertThat(it, iterableWithSize(3));", + " assertThat(ar, arrayWithSize(3));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", + "", + "import java.util.*;", + "class Test {", + " void foo(Iterable it, String[] ar, List li) {", + " assertThat(li).hasSize(3);", + " assertThat(li).hasSize(3);", + " assertThat(it).hasSize(3);", + " assertThat(ar).hasSize(3);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void fix_assertThat_same() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(Object a, Object b) {", + " assertThat(a, theInstance(b));", + " assertThat(a, sameInstance(b));", + " assertThat(a, is(not(theInstance(b))));", + " assertThat(a, not(sameInstance(b)));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(Object a, Object b) {", + " assertThat(a).isSameAs(b);", + " assertThat(a).isSameAs(b);", + " assertThat(a).isNotSameAs(b);", + " assertThat(a).isNotSameAs(b);", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); From fb205b7817c92f6562b6bd2ffd0bda11eb532df5 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Sep 2019 18:18:21 +0000 Subject: [PATCH 2/7] Add generated changelog entries --- changelog/@unreleased/pr-850.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-850.v2.yml diff --git a/changelog/@unreleased/pr-850.v2.yml b/changelog/@unreleased/pr-850.v2.yml new file mode 100644 index 000000000..90d9bb5eb --- /dev/null +++ b/changelog/@unreleased/pr-850.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: PreferAssertj provides better replacements fixes + links: + - https://github.com/palantir/gradle-baseline/pull/850 From 32d66edd66acc0586a908b62d796ca01f089e887 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Sep 2019 15:10:10 -0400 Subject: [PATCH 3/7] Improve AssertJ assertEquals floating point with delta --- .../baseline/errorprone/PreferAssertj.java | 33 ++- .../errorprone/PreferAssertjTests.java | 200 +++--------------- 2 files changed, 53 insertions(+), 180 deletions(-) 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 5fca46745..8d260cb70 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 @@ -289,28 +289,40 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (ASSERT_EQUALS_FLOATING.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") - .replace(tree, assertThat + "(" + argSource(tree, state, 1) + ").isCloseTo(" - + argSource(tree, state, 0) + ", within(" + argSource(tree, state, 2) + "))")); + .replace(tree, assertThat + "(" + argSource(tree, state, 1) + + (isConstantZero(tree.getArguments().get(2)) + ? ").isEqualTo(" + argSource(tree, state, 0) + ')' + : (").isCloseTo(" + argSource(tree, state, 0) + + ", within(" + argSource(tree, state, 2) + "))")))); } if (ASSERT_EQUALS_FLOATING_DESCRIPTION.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") .replace(tree, assertThat + "(" + argSource(tree, state, 2) - + ").describedAs(" + argSource(tree, state, 0) + ").isCloseTo(" - + argSource(tree, state, 1) + ", within(" + argSource(tree, state, 3) + "))")); + + ").describedAs(" + argSource(tree, state, 0) + + (isConstantZero(tree.getArguments().get(3)) + ? ").isEqualTo(" + argSource(tree, state, 1) + ')' + : (").isCloseTo(" + argSource(tree, state, 1) + + ", within(" + argSource(tree, state, 3) + "))")))); } if (ASSERT_NOT_EQUALS_FLOATING.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") - .replace(tree, assertThat + "(" + argSource(tree, state, 1) + ").isNotCloseTo(" - + argSource(tree, state, 0) + ", within(" + argSource(tree, state, 2) + "))")); + .replace(tree, assertThat + "(" + argSource(tree, state, 1) + + (isConstantZero(tree.getArguments().get(2)) + ? ").isNotEqualTo(" + argSource(tree, state, 0) + ')' + : ").isNotCloseTo(" + argSource(tree, state, 0) + + ", within(" + argSource(tree, state, 2) + "))"))); } if (ASSERT_NOT_EQUALS_FLOATING_DESCRIPTION.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") .replace(tree, assertThat + "(" + argSource(tree, state, 2) - + ").describedAs(" + argSource(tree, state, 0) + ").isNotCloseTo(" - + argSource(tree, state, 1) + ", within(" + argSource(tree, state, 3) + "))")); + + ").describedAs(" + argSource(tree, state, 0) + + (isConstantZero(tree.getArguments().get(3)) + ? ").isNotEqualTo(" + argSource(tree, state, 1) + ')' + : ").isNotCloseTo(" + argSource(tree, state, 1) + + ", within(" + argSource(tree, state, 3) + "))"))); } if (ASSERT_THAT.matches(tree, state)) { Optional replacement = tree.getArguments().get(1) @@ -424,6 +436,11 @@ private static boolean useStaticAssertjImport(VisitorState state) { return true; } + private static boolean isConstantZero(Tree tree) { + Object constantValue = ASTHelpers.constValue(tree); + return constantValue instanceof Number && ((Number) constantValue).doubleValue() == 0D; + } + private static boolean isExpressionSameType(VisitorState state, MemberSelectTree memberSelectTree, String type) { return ASTHelpers.isSameType( ASTHelpers.getType(memberSelectTree.getExpression()), 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 187ac0170..8d8f6cc36 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 @@ -422,192 +422,48 @@ public void fix_failDescriptionNonStaticImport() { } @Test - public void fix_assertEqualsFloat() { + public void fix_assertEqualsFloating() { test() .addInputLines( "Test.java", "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(float value) {", - " assertEquals(.1f, value, .01f);", - " }", - "}") - .addOutputLines( - "Test.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.assertj.core.api.Assertions.within;", - "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(float value) {", - " assertThat(value).isCloseTo(.1f, within(.01f));", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - public void fix_assertEqualsFloatDescription() { - test() - .addInputLines( - "Test.java", - "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(float value) {", - " assertEquals(\"desc\", .1f, value, .01f);", - " }", - "}") - .addOutputLines( - "Test.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.assertj.core.api.Assertions.within;", - "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(float value) {", - " assertThat(value).describedAs(\"desc\").isCloseTo(.1f, within(.01f));", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - public void fix_assertEqualsDouble() { - test() - .addInputLines( - "Test.java", - "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(double value) {", - " assertEquals(.1D, value, .01D);", - " }", - "}") - .addOutputLines( - "Test.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.assertj.core.api.Assertions.within;", - "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(double value) {", - " assertThat(value).isCloseTo(.1D, within(.01D));", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - public void fix_assertEqualsDoubleDescription() { - test() - .addInputLines( - "Test.java", - "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(double value) {", - " assertEquals(\"desc\", .1D, value, .01D);", - " }", - "}") - .addOutputLines( - "Test.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.assertj.core.api.Assertions.within;", - "import static org.junit.Assert.assertEquals;", - "class Test {", - " void foo(double value) {", - " assertThat(value).describedAs(\"desc\").isCloseTo(.1D, within(.01D));", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - public void fix_assertNotEqualsFloat() { - test() - .addInputLines( - "Test.java", - "import static org.junit.Assert.assertNotEquals;", - "class Test {", - " void foo(float value) {", - " assertNotEquals(.1f, value, .01f);", - " }", - "}") - .addOutputLines( - "Test.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.assertj.core.api.Assertions.within;", - "import static org.junit.Assert.assertNotEquals;", - "class Test {", - " void foo(float value) {", - " assertThat(value).isNotCloseTo(.1f, within(.01f));", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - public void fix_assertNotEqualsFloatDescription() { - test() - .addInputLines( - "Test.java", - "import static org.junit.Assert.assertNotEquals;", - "class Test {", - " void foo(float value) {", - " assertNotEquals(\"desc\", .1f, value, .01f);", - " }", - "}") - .addOutputLines( - "Test.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.assertj.core.api.Assertions.within;", "import static org.junit.Assert.assertNotEquals;", "class Test {", - " void foo(float value) {", - " assertThat(value).describedAs(\"desc\").isNotCloseTo(.1f, within(.01f));", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - public void fix_assertNotEqualsDouble() { - test() - .addInputLines( - "Test.java", - "import static org.junit.Assert.assertNotEquals;", - "class Test {", - " void foo(double value) {", - " assertNotEquals(.1D, value, .01D);", - " }", - "}") - .addOutputLines( - "Test.java", - "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.assertj.core.api.Assertions.within;", - "import static org.junit.Assert.assertNotEquals;", - "class Test {", - " void foo(double value) {", - " assertThat(value).isNotCloseTo(.1D, within(.01D));", - " }", - "}") - .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); - } - - @Test - public void fix_assertNotEqualsDoubleDescription() { - test() - .addInputLines( - "Test.java", - "import static org.junit.Assert.assertNotEquals;", - "class Test {", - " void foo(double value) {", - " assertNotEquals(\"desc\", .1D, value, .01D);", + " void foo(float fl, double db) {", + " assertEquals(.1f, fl, .01f);", + " assertEquals(\"desc\", .1f, fl, .01f);", + " assertEquals(.1D, db, .01D);", + " assertEquals(\"desc\", .1D, db, .01D);", + " assertEquals(\"desc\", .1D, db, 0);", + " assertEquals(.1D, db, 0D);", + " assertNotEquals(.1f, fl, .01f);", + " assertNotEquals(\"desc\", .1f, fl, .01f);", + " assertNotEquals(.1D, db, .01D);", + " assertNotEquals(\"desc\", .1D, db, .01D);", + " assertNotEquals(\"desc\", .1D, db, 0.0);", + " assertNotEquals(.1D, db, 0D);", " }", "}") .addOutputLines( "Test.java", "import static org.assertj.core.api.Assertions.assertThat;", "import static org.assertj.core.api.Assertions.within;", + "import static org.junit.Assert.assertEquals;", "import static org.junit.Assert.assertNotEquals;", "class Test {", - " void foo(double value) {", - " assertThat(value).describedAs(\"desc\").isNotCloseTo(.1D, within(.01D));", + " void foo(float fl, double db) {", + " assertThat(fl).isCloseTo(.1f, within(.01f));", + " assertThat(fl).describedAs(\"desc\").isCloseTo(.1f, within(.01f));", + " assertThat(db).isCloseTo(.1D, within(.01D));", + " assertThat(db).describedAs(\"desc\").isCloseTo(.1D, within(.01D));", + " assertThat(db).describedAs(\"desc\").isEqualTo(.1D);", + " assertThat(db).isEqualTo(.1D);", + " assertThat(fl).isNotCloseTo(.1f, within(.01f));", + " assertThat(fl).describedAs(\"desc\").isNotCloseTo(.1f, within(.01f));", + " assertThat(db).isNotCloseTo(.1D, within(.01D));", + " assertThat(db).describedAs(\"desc\").isNotCloseTo(.1D, within(.01D));", + " assertThat(db).describedAs(\"desc\").isNotEqualTo(.1D);", + " assertThat(db).isNotEqualTo(.1D);", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); From 0a5ad9d8ccb38392160fd82f1701336c969c79e7 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Sep 2019 15:43:08 -0400 Subject: [PATCH 4/7] String formt floating delta refactors --- .../baseline/errorprone/PreferAssertj.java | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) 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 8d260cb70..786c4ed4c 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 @@ -289,40 +289,48 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (ASSERT_EQUALS_FLOATING.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") - .replace(tree, assertThat + "(" + argSource(tree, state, 1) - + (isConstantZero(tree.getArguments().get(2)) - ? ").isEqualTo(" + argSource(tree, state, 0) + ')' - : (").isCloseTo(" + argSource(tree, state, 0) - + ", within(" + argSource(tree, state, 2) + "))")))); + .replace(tree, String.format("%s(%s)%s", + assertThat, + argSource(tree, state, 1), + isConstantZero(tree.getArguments().get(2)) + ? String.format(".isEqualTo(%s)", argSource(tree, state, 0)) + : String.format(".isCloseTo(%s, within(%s))", + argSource(tree, state, 0), argSource(tree, state, 2))))); } if (ASSERT_EQUALS_FLOATING_DESCRIPTION.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") - .replace(tree, assertThat + "(" + argSource(tree, state, 2) - + ").describedAs(" + argSource(tree, state, 0) - + (isConstantZero(tree.getArguments().get(3)) - ? ").isEqualTo(" + argSource(tree, state, 1) + ')' - : (").isCloseTo(" + argSource(tree, state, 1) - + ", within(" + argSource(tree, state, 3) + "))")))); + .replace(tree, String.format("%s(%s).describedAs(%s)%s", + assertThat, + argSource(tree, state, 2), + argSource(tree, state, 0), + isConstantZero(tree.getArguments().get(3)) + ? String.format(".isEqualTo(%s)", argSource(tree, state, 1)) + : String.format(".isCloseTo(%s, within(%s))", + argSource(tree, state, 1), argSource(tree, state, 3))))); } if (ASSERT_NOT_EQUALS_FLOATING.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") - .replace(tree, assertThat + "(" + argSource(tree, state, 1) - + (isConstantZero(tree.getArguments().get(2)) - ? ").isNotEqualTo(" + argSource(tree, state, 0) + ')' - : ").isNotCloseTo(" + argSource(tree, state, 0) - + ", within(" + argSource(tree, state, 2) + "))"))); + .replace(tree, String.format("%s(%s)%s", + assertThat, + argSource(tree, state, 1), + isConstantZero(tree.getArguments().get(2)) + ? String.format(".isNotEqualTo(%s)", argSource(tree, state, 0)) + : String.format(".isNotCloseTo(%s, within(%s))", + argSource(tree, state, 0), argSource(tree, state, 2))))); } if (ASSERT_NOT_EQUALS_FLOATING_DESCRIPTION.matches(tree, state)) { return withAssertThat(tree, state, (assertThat, fix) -> fix .addStaticImport("org.assertj.core.api.Assertions.within") - .replace(tree, assertThat + "(" + argSource(tree, state, 2) - + ").describedAs(" + argSource(tree, state, 0) - + (isConstantZero(tree.getArguments().get(3)) - ? ").isNotEqualTo(" + argSource(tree, state, 1) + ')' - : ").isNotCloseTo(" + argSource(tree, state, 1) - + ", within(" + argSource(tree, state, 3) + "))"))); + .replace(tree, String.format("%s(%s).describedAs(%s)%s", + assertThat, + argSource(tree, state, 2), + argSource(tree, state, 0), + isConstantZero(tree.getArguments().get(3)) + ? String.format(".isNotEqualTo(%s)", argSource(tree, state, 1)) + : String.format(".isNotCloseTo(%s, within(%s))", + argSource(tree, state, 1), argSource(tree, state, 3))))); } if (ASSERT_THAT.matches(tree, state)) { Optional replacement = tree.getArguments().get(1) From ca5b9f942e8a7ca23e2dd2da3769c605f24e6423 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Sep 2019 16:07:18 -0400 Subject: [PATCH 5/7] Migrate String startsWith, endsWith, and contains --- .../baseline/errorprone/PreferAssertj.java | 31 ++++++++++--- .../errorprone/PreferAssertjTests.java | 44 +++++++++++++++++-- 2 files changed, 66 insertions(+), 9 deletions(-) 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 786c4ed4c..1faa4e966 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 @@ -513,10 +513,15 @@ private static final class HamcrestVisitor extends SimpleTreeVisitor HAS_ITEM = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) - .namedAnyOf("hasItem", "hasItemInArray") - .withParameters(Object.class.getName()); + private static final Matcher CONTAINS = Matchers.anyOf( + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("hasItem", "hasItemInArray") + .withParameters(Object.class.getName()), + MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .named("containsString") + .withParameters(String.class.getName())); // Note: cannot match array/vararg arguments private static final Matcher HAS_ITEMS = MethodMatchers.staticMethod() @@ -543,6 +548,16 @@ private static final class HamcrestVisitor extends SimpleTreeVisitor STARTS_WITH = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("startsWith") + .withParameters(String.class.getName()); + + private static final Matcher ENDS_WITH = MethodMatchers.staticMethod() + .onClassAny(MATCHERS) + .namedAnyOf("endsWith") + .withParameters(String.class.getName()); + private final boolean negated; private HamcrestVisitor(boolean negated) { @@ -574,7 +589,7 @@ public Optional visitMethodInvocation(MethodInvocationTree node, Visitor if (NOT_NULL.matches(node, state)) { return Optional.of(negated ? ".isNull()" : ".isNotNull()"); } - if (HAS_ITEM.matches(node, state)) { + if (CONTAINS.matches(node, state)) { return Optional.of((negated ? ".doesNotContain(" : ".contains(") + argSource(node, state, 0) + ')'); } if (IS_EMPTY.matches(node, state)) { @@ -583,6 +598,12 @@ public Optional visitMethodInvocation(MethodInvocationTree node, Visitor if (SAME_INSTANCE.matches(node, state)) { return Optional.of((negated ? ".isNotSameAs(" : ".isSameAs(") + argSource(node, state, 0) + ')'); } + if (STARTS_WITH.matches(node, state)) { + return Optional.of((negated ? ".doesNotStartWith(" : ".startsWith(") + argSource(node, state, 0) + ')'); + } + if (ENDS_WITH.matches(node, state)) { + return Optional.of((negated ? ".doesNotEndWith(" : ".endsWith(") + argSource(node, state, 0) + ')'); + } if (HAS_SIZE.matches(node, state)) { if (negated) { // No top level method for negated size assertions 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 8d8f6cc36..c6c064015 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 @@ -855,21 +855,21 @@ public void fix_matcherAssertThatString_startsWith() { .addInputLines( "Test.java", "import static org.hamcrest.MatcherAssert.assertThat;", - "import static org.hamcrest.Matchers.startsWith;", + "import static org.hamcrest.Matchers.hasToString;", "class Test {", " void foo(String value) {", - " assertThat(value, startsWith(\"str\"));", + " assertThat(value, hasToString(\"str\"));", " }", "}") .addOutputLines( "Test.java", "import static org.assertj.core.api.Assertions.assertThat;", - "import static org.hamcrest.Matchers.startsWith;", + "import static org.hamcrest.Matchers.hasToString;", "", "import org.assertj.core.api.HamcrestCondition;", "class Test {", " void foo(String value) {", - " assertThat(value).is(new HamcrestCondition<>(startsWith(\"str\")));", + " assertThat(value).is(new HamcrestCondition<>(hasToString(\"str\")));", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); @@ -1042,6 +1042,7 @@ public void fix_assertThat_contains() { " assertThat(arrayValue, arrayContainingInAnyOrder(\"one\", \"two\"));", " assertThat(value, not(hasItem(\"str\")));", " assertThat(arrayValue, not(hasItemInArray(\"str\")));", + " assertThat(arrayValue[0], containsString(\"str\"));", " }", "}") .addOutputLines( @@ -1056,6 +1057,7 @@ public void fix_assertThat_contains() { " assertThat(arrayValue).contains(\"one\", \"two\");", " assertThat(value).doesNotContain(\"str\");", " assertThat(arrayValue).doesNotContain(\"str\");", + " assertThat(arrayValue[0]).contains(\"str\");", " }", "}") .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); @@ -1175,6 +1177,40 @@ public void fix_assertThat_same() { .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } + @Test + public void fix_assertThat_strings() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value, containsString(\"str\"));", + " assertThat(value, not(containsString(\"str\")));", + " assertThat(value, startsWith(\"str\"));", + " assertThat(value, not(startsWith(\"str\")));", + " assertThat(value, endsWith(\"str\"));", + " assertThat(value, not(endsWith(\"str\")));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.Matchers.*;", + "class Test {", + " void foo(String value) {", + " assertThat(value).contains(\"str\");", + " assertThat(value).doesNotContain(\"str\");", + " assertThat(value).startsWith(\"str\");", + " assertThat(value).doesNotStartWith(\"str\");", + " assertThat(value).endsWith(\"str\");", + " assertThat(value).doesNotEndWith(\"str\");", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + private BugCheckerRefactoringTestHelper test() { return BugCheckerRefactoringTestHelper.newInstance(new PreferAssertj(), getClass()); } From ddffa0c444b31ae9c28b36a33dc75a149078d3df Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Sep 2019 17:28:45 -0400 Subject: [PATCH 6/7] Allow direct references to hamcrest matchers --- .../baseline/errorprone/PreferAssertj.java | 64 +++++++++++++------ .../errorprone/PreferAssertjTests.java | 34 ++++++++++ 2 files changed, 77 insertions(+), 21 deletions(-) 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 1faa4e966..70fb2a6f9 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 @@ -30,6 +30,8 @@ import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.predicates.TypePredicates; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.ImportTree; @@ -37,6 +39,7 @@ import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.source.util.SimpleTreeVisitor; +import com.sun.tools.javac.code.Type; import java.util.List; import java.util.Optional; import java.util.function.BiConsumer; @@ -468,93 +471,112 @@ private static final class HamcrestVisitor extends SimpleTreeVisitor MATCHERS = ImmutableSet.of( - "org.hamcrest.Matchers", - "org.hamcrest.CoreMatchers", - "org.hamcrest.core.IsIterableContaining"); + private static final TypePredicate MATCHERS = new TypePredicate() { + + private final TypePredicate matcherPredicate = TypePredicates.isDescendantOf("org.hamcrest.Matcher"); + private final TypePredicate[] predicates = new TypePredicate[] { + TypePredicates.isExactType("org.hamcrest.Matchers"), + TypePredicates.isExactType("org.hamcrest.CoreMatchers"), + // Allows uses of direct imports to be migrated as well, + // e.g. 'org.hamcrest.core.Is.is'. + (TypePredicate) (type, state) -> matcherPredicate.apply(type, state) + // Limit to Hamcrest packages to avoid interaction with non-standard library code + && type.toString().startsWith("org.hamcrest.") + }; + + @Override + public boolean apply(Type type, VisitorState state) { + for (TypePredicate predicate : predicates) { + if (predicate.apply(type, state)) { + return true; + } + } + return false; + } + }; private static final Matcher IS_MATCHER = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .named("is") .withParameters("org.hamcrest.Matcher"); private static final Matcher NOT_MATCHER = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .named("not") .withParameters("org.hamcrest.Matcher"); private static final Matcher EQUALS = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("is", "equalTo", "equalToObject") .withParameters(Object.class.getName()); private static final Matcher INSTANCE_OF = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("isA", "instanceOf") .withParameters("java.lang.Class"); private static final Matcher NULL = Matchers.anyOf( MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .named("nullValue") .withParameters(), MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .named("nullValue") .withParameters("java.lang.Class")); private static final Matcher NOT_NULL = Matchers.anyOf( MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .named("notNullValue") .withParameters(), MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .named("notNullValue") .withParameters("java.lang.Class")); private static final Matcher CONTAINS = Matchers.anyOf( MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("hasItem", "hasItemInArray") .withParameters(Object.class.getName()), MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .named("containsString") .withParameters(String.class.getName())); // Note: cannot match array/vararg arguments private static final Matcher HAS_ITEMS = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("hasItems", "arrayContainingInAnyOrder"); private static final Matcher IS_EMPTY = Matchers.anyOf( MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("empty", "emptyIterable", "emptyArray", "anEmptyMap") .withParameters(), MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("emptyCollectionOf", "emptyIterableOf") .withParameters(Class.class.getName())); private static final Matcher HAS_SIZE = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("hasSize", "aMapWithSize", "arrayWithSize", "iterableWithSize") .withParameters(int.class.getName()); private static final Matcher SAME_INSTANCE = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("sameInstance", "theInstance") .withParameters(Object.class.getName()); private static final Matcher STARTS_WITH = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("startsWith") .withParameters(String.class.getName()); private static final Matcher ENDS_WITH = MethodMatchers.staticMethod() - .onClassAny(MATCHERS) + .onClass(MATCHERS) .namedAnyOf("endsWith") .withParameters(String.class.getName()); 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 c6c064015..58d448133 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 @@ -1211,6 +1211,40 @@ public void fix_assertThat_strings() { .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } + @Test + public void fix_assertThat_wrongImport() { + test() + .addInputLines( + "Test.java", + "import static org.hamcrest.MatcherAssert.assertThat;", + "import static org.hamcrest.core.Is.is;", + "import static org.hamcrest.core.IsEqual.equalTo;", + "import static org.hamcrest.core.IsNot.not;", + "class Test {", + " void foo(String value) {", + " assertThat(value, is(\"str\"));", + " assertThat(value, equalTo(\"str\"));", + " assertThat(value, not(is(\"str\")));", + " assertThat(value, not(equalTo(\"str\")));", + " }", + "}") + .addOutputLines( + "Test.java", + "import static org.assertj.core.api.Assertions.assertThat;", + "import static org.hamcrest.core.Is.is;", + "import static org.hamcrest.core.IsEqual.equalTo;", + "import static org.hamcrest.core.IsNot.not;", + "class Test {", + " void foo(String value) {", + " assertThat(value).isEqualTo(\"str\");", + " assertThat(value).isEqualTo(\"str\");", + " assertThat(value).isNotEqualTo(\"str\");", + " assertThat(value).isNotEqualTo(\"str\");", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + private BugCheckerRefactoringTestHelper test() { return BugCheckerRefactoringTestHelper.newInstance(new PreferAssertj(), getClass()); } From 0f86a5286675669c76ebc8ed77c59efd4137712b Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 18 Sep 2019 21:28:45 +0000 Subject: [PATCH 7/7] Add generated changelog entries --- changelog/@unreleased/pr-850.v2.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog/@unreleased/pr-850.v2.yml b/changelog/@unreleased/pr-850.v2.yml index 90d9bb5eb..889db9536 100644 --- a/changelog/@unreleased/pr-850.v2.yml +++ b/changelog/@unreleased/pr-850.v2.yml @@ -1,5 +1,5 @@ -type: improvement -improvement: +type: fix +fix: description: PreferAssertj provides better replacements fixes links: - https://github.com/palantir/gradle-baseline/pull/850