diff --git a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJ.java b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJ.java index 87c26800f..a4ced82de 100644 --- a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJ.java +++ b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJ.java @@ -36,7 +36,7 @@ @AllArgsConstructor public class HamcrestMatcherToAssertJ extends Recipe { private static final MethodMatcher ASSERT_THAT_MATCHER = new MethodMatcher("org.hamcrest.MatcherAssert assertThat(..)"); - private static final MethodMatcher SUB_MATCHER = new MethodMatcher("org.hamcrest.*Matchers *(org.hamcrest.Matcher)"); + private static final String HAMCREST_MATCHER = "org.hamcrest.Matcher"; private static final MethodMatcher CLOSE_TO_MATCHER = new MethodMatcher("org.hamcrest.Matchers closeTo(..)"); @Option(displayName = "Hamcrest matcher", @@ -91,12 +91,15 @@ private J.MethodInvocation replace(J.MethodInvocation mi, ExecutionContext ctx) Expression reasonArgument = mia.size() == 3 ? mia.get(0) : null; Expression actualArgument = mia.get(mia.size() - 2); Expression matcherArgument = mia.get(mia.size() - 1); - if (!matchersMatcher.matches(matcherArgument) || SUB_MATCHER.matches(matcherArgument)) { + if (!matchersMatcher.matches(matcherArgument) || takesMatcher((J.MethodInvocation) matcherArgument)) { return mi; } if (argumentType != null && !TypeUtils.isAssignableTo(argumentType, actualArgument.getType())) { return mi; } + if (passesIncompatibleVarargsArray((J.MethodInvocation) matcherArgument, actualArgument.getType())) { + return mi; + } String actual = typeToIndicator(actualArgument.getType()); J.MethodInvocation matcherArgumentMethod = (J.MethodInvocation) matcherArgument; @@ -194,4 +197,41 @@ private String typeToIndicator(@Nullable JavaType type) { return "#{any(java.lang.Object)}"; } } + + static boolean takesMatcher(J.MethodInvocation method) { + JavaType.Method methodType = method.getMethodType(); + return (methodType != null && methodType.getParameterTypes().stream().anyMatch(HamcrestMatcherToAssertJ::isOrHoldsMatcher)) || + method.getArguments().stream().anyMatch(arg -> isOrHoldsMatcher(arg.getType())); + } + + private static boolean isOrHoldsMatcher(@Nullable JavaType type) { + if (type instanceof JavaType.Array) { + return isOrHoldsMatcher(((JavaType.Array) type).getElemType()); + } + if (type instanceof JavaType.Parameterized && TypeUtils.isAssignableTo("java.lang.Iterable", type)) { + return ((JavaType.Parameterized) type).getTypeParameters().stream().anyMatch(param -> TypeUtils.isAssignableTo(HAMCREST_MATCHER, param)); + } + return TypeUtils.isAssignableTo(HAMCREST_MATCHER, type); + } + + private static boolean passesIncompatibleVarargsArray(J.MethodInvocation matcher, @Nullable JavaType actualType) { + JavaType.Method methodType = matcher.getMethodType(); + JavaType elementType = elementType(actualType); + if (methodType == null || !methodType.hasFlags(Flag.Varargs) || elementType == null) { + return false; + } + JavaType lastArgumentType = matcher.getArguments().get(matcher.getArguments().size() - 1).getType(); + return lastArgumentType instanceof JavaType.Array && !TypeUtils.isAssignableTo(elementType, ((JavaType.Array) lastArgumentType).getElemType()); + } + + private static @Nullable JavaType elementType(@Nullable JavaType type) { + if (type instanceof JavaType.Array) { + return ((JavaType.Array) type).getElemType(); + } + if (type instanceof JavaType.Parameterized && TypeUtils.isAssignableTo("java.lang.Iterable", type)) { + List typeParameters = ((JavaType.Parameterized) type).getTypeParameters(); + return typeParameters.size() == 1 ? typeParameters.get(0) : null; + } + return null; + } } diff --git a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJ.java b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJ.java index 79376aafe..966ab03e0 100644 --- a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJ.java +++ b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJ.java @@ -20,6 +20,7 @@ import lombok.NoArgsConstructor; import org.jspecify.annotations.Nullable; import org.openrewrite.*; +import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaParser; import org.openrewrite.java.JavaTemplate; @@ -33,14 +34,12 @@ import java.util.List; import static java.util.stream.Collectors.joining; -import static java.util.stream.Collectors.toList; @NoArgsConstructor @AllArgsConstructor public class HamcrestNotMatcherToAssertJ extends Recipe { private static final MethodMatcher ASSERT_THAT_MATCHER = new MethodMatcher("org.hamcrest.MatcherAssert assertThat(..)"); private static final MethodMatcher NOT_MATCHER = new MethodMatcher("org.hamcrest.*Matchers not(org.hamcrest.Matcher)"); - private static final MethodMatcher SUB_MATCHER = new MethodMatcher("org.hamcrest.*Matchers *(org.hamcrest.Matcher)"); @Option(displayName = "Hamcrest Matcher", description = "The Hamcrest `not(Matcher)` to migrate to AssertJ.", @@ -73,36 +72,29 @@ private class MigrateToAssertJVisitor extends JavaIsoVisitor { @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { J.MethodInvocation mi = super.visitMethodInvocation(method, ctx); - if (ASSERT_THAT_MATCHER.matches(mi)) { - Expression notMethodInvocation = mi.getArguments().get(mi.getArguments().size() - 1); - if (!NOT_MATCHER.matches(notMethodInvocation)) { - return mi; - } - Expression matcherArgument = ((J.MethodInvocation) notMethodInvocation).getArguments().get(0); - if (mi.getArguments().size() == 2) { - return handleTwoArgumentCase(mi, matcherArgument, ctx); - } - if (mi.getArguments().size() == 3) { - return handleThreeArgumentCase(mi, matcherArgument, ctx); - } + if (!ASSERT_THAT_MATCHER.matches(mi)) { + return mi; } - return mi; - } - - private J.MethodInvocation handleTwoArgumentCase(J.MethodInvocation mi, Expression matcherArgument, ExecutionContext ctx) { - Expression actualArgument = mi.getArguments().get(0); - if (!matchersMatcher.matches(matcherArgument) || SUB_MATCHER.matches(matcherArgument)) { + List mia = mi.getArguments(); + Expression notMethodInvocation = mia.get(mia.size() - 1); + if (!NOT_MATCHER.matches(notMethodInvocation)) { return mi; } - String actual = typeToIndicator(actualArgument.getType()); - List originalArguments = ((J.MethodInvocation) matcherArgument).getArguments().stream() - .filter(a -> !(a instanceof J.Empty)) - .collect(toList()); + Expression matcherArgument = ((J.MethodInvocation) notMethodInvocation).getArguments().get(0); + if (!matchersMatcher.matches(matcherArgument) || HamcrestMatcherToAssertJ.takesMatcher((J.MethodInvocation) matcherArgument)) { + return mi; + } + Expression reasonArgument = mia.size() == 3 ? mia.get(0) : null; + Expression actualArgument = mia.get(mia.size() - 2); + List originalArguments = ListUtils.filter(((J.MethodInvocation) matcherArgument).getArguments(), a -> !(a instanceof J.Empty)); String argumentsTemplate = originalArguments.stream() .map(a -> typeToIndicator(a.getType())) .collect(joining(", ")); - JavaTemplate template = JavaTemplate.builder(String.format("assertThat(%s).%s(%s)", - actual, assertion, argumentsTemplate)) + JavaTemplate template = JavaTemplate.builder(String.format("assertThat(%s)%s.%s(%s)", + typeToIndicator(actualArgument.getType()), + reasonArgument != null ? ".as(#{any(String)})" : "", + assertion, + argumentsTemplate)) .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "assertj-core-3")) .staticImports("org.assertj.core.api.Assertions.assertThat") .build(); @@ -116,37 +108,9 @@ private J.MethodInvocation handleTwoArgumentCase(J.MethodInvocation mi, Expressi List templateArguments = new ArrayList<>(); templateArguments.add(actualArgument); - templateArguments.addAll(originalArguments); - return template.apply(getCursor(), mi.getCoordinates().replace(), templateArguments.toArray()); - } - - private J.MethodInvocation handleThreeArgumentCase(J.MethodInvocation mi, Expression matcherArgument, ExecutionContext ctx) { - Expression reasonArgument = mi.getArguments().get(0); - Expression actualArgument = mi.getArguments().get(1); - if (!matchersMatcher.matches(matcherArgument) || SUB_MATCHER.matches(matcherArgument)) { - return mi; + if (reasonArgument != null) { + templateArguments.add(reasonArgument); } - String actual = typeToIndicator(actualArgument.getType()); - List originalArguments = ((J.MethodInvocation) matcherArgument).getArguments().stream() - .filter(a -> !(a instanceof J.Empty)) - .collect(toList()); - String argumentsTemplate = originalArguments.stream() - .map(a -> typeToIndicator(a.getType())) - .collect(joining(", ")); - JavaTemplate template = JavaTemplate.builder(String.format("assertThat(%s).as(#{any(String)}).%s(%s)", - actual, assertion, argumentsTemplate)) - .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "assertj-core-3")) - .staticImports("org.assertj.core.api.Assertions.assertThat") - .build(); - maybeRemoveImport("org.hamcrest.Matchers.not"); - maybeRemoveImport("org.hamcrest.Matchers." + notMatcher); - maybeRemoveImport("org.hamcrest.MatcherAssert"); - maybeRemoveImport("org.hamcrest.MatcherAssert.assertThat"); - maybeAddImport("org.assertj.core.api.Assertions", "assertThat"); - - List templateArguments = new ArrayList<>(); - templateArguments.add(actualArgument); - templateArguments.add(reasonArgument); templateArguments.addAll(originalArguments); return template.apply(getCursor(), mi.getCoordinates().replace(), templateArguments.toArray()); } diff --git a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJTest.java b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJTest.java index aa1ede3a6..f8e4fdffb 100644 --- a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJTest.java +++ b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToAssertJTest.java @@ -23,7 +23,6 @@ import org.openrewrite.java.JavaParser; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; -import org.openrewrite.test.TypeValidation; import static org.openrewrite.java.Assertions.java; @@ -136,6 +135,111 @@ void test() { """ )); } + + @Test + void containsInAnyOrderWithObjectArray() { + rewriteRun( + spec -> spec.recipe(new HamcrestMatcherToAssertJ("containsInAnyOrder", "containsExactlyInAnyOrder", null)), + //language=java + java( + """ + import java.util.List; + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.containsInAnyOrder; + + class ATest { + void test(List list, List states) { + assertThat(list, containsInAnyOrder(states.toArray())); + } + } + """ + )); + } + + @Test + void containsVarargsMatchers() { + rewriteRun( + spec -> spec.recipe(new HamcrestMatcherToAssertJ("contains", "containsExactly", null)), + //language=java + java( + """ + import java.util.List; + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.contains; + import static org.hamcrest.Matchers.equalTo; + import static org.hamcrest.Matchers.startsWith; + + class ATest { + void test(List list) { + assertThat(list, contains(equalTo("a"), startsWith("b"))); + assertThat("reason", list, contains(equalTo("a"), startsWith("b"))); + } + } + """ + )); + } + + @Test + void containsListOfMatchers() { + rewriteRun( + spec -> spec.recipe(new HamcrestMatcherToAssertJ("contains", "containsExactly", null)), + //language=java + java( + """ + import java.util.List; + import org.hamcrest.Matcher; + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.contains; + + class ATest { + void test(List list, List> matchers) { + assertThat(list, contains(matchers)); + } + } + """ + )); + } + + @Test + void arrayContainingMatchers() { + rewriteRun( + spec -> spec.recipe(new HamcrestMatcherToAssertJ("arrayContaining", "containsExactly", null)), + //language=java + java( + """ + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.arrayContaining; + import static org.hamcrest.Matchers.equalTo; + + class ATest { + void test(String[] array) { + assertThat(array, arrayContaining(equalTo("a"), equalTo("b"))); + } + } + """ + )); + } + + @Test + void hasEntryMatchers() { + rewriteRun( + spec -> spec.recipe(new HamcrestMatcherToAssertJ("hasEntry", "containsEntry", null)), + //language=java + java( + """ + import java.util.Map; + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.equalTo; + import static org.hamcrest.Matchers.hasEntry; + + class ATest { + void test(Map map) { + assertThat(map, hasEntry(equalTo("a"), equalTo("b"))); + } + } + """ + )); + } } @Nested @@ -383,45 +487,31 @@ void test() { ); } - @SuppressWarnings("DataFlowIssue") @Test void containsInAnyOrderWithArray() { rewriteRun( - spec -> spec.recipe(new HamcrestMatcherToAssertJ("containsInAnyOrder", "containsExactlyInAnyOrder", null)) - .afterTypeValidationOptions(TypeValidation.all().methodInvocations(false)), + spec -> spec.recipe(new HamcrestMatcherToAssertJ("containsInAnyOrder", "containsExactlyInAnyOrder", null)), //language=java java( - """ - import org.junit.jupiter.api.Test; - - import java.util.ArrayList; + """ import java.util.List; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; class ATest { - @Test - void test() { - List list = new ArrayList<>(); - List states = null; - assertThat(list, containsInAnyOrder(states.toArray())); + void test(List list, List states) { + assertThat(list, containsInAnyOrder(states.toArray(new String[0]))); } } """, """ - import org.junit.jupiter.api.Test; - - import java.util.ArrayList; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; class ATest { - @Test - void test() { - List list = new ArrayList<>(); - List states = null; - assertThat(list).containsExactlyInAnyOrder(states.toArray()); + void test(List list, List states) { + assertThat(list).containsExactlyInAnyOrder(states.toArray(new String[0])); } } """ diff --git a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJTest.java b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJTest.java index 779f6b2b9..c2ebcb04b 100644 --- a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJTest.java +++ b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestNotMatcherToAssertJTest.java @@ -157,4 +157,60 @@ void test() { ) ); } + + @Test + void notMatcherWithReasonFromCoreMatchers() { + rewriteRun( + spec -> spec.recipe(new HamcrestNotMatcherToAssertJ("equalTo", "isNotEqualTo")), + //language=java + java( + """ + import static org.hamcrest.CoreMatchers.equalTo; + import static org.hamcrest.CoreMatchers.not; + import static org.hamcrest.MatcherAssert.assertThat; + + class ATest { + void test(String str1, String str2) { + assertThat("Reason", str1, not(equalTo(str2))); + } + } + """, + """ + import static org.assertj.core.api.Assertions.assertThat; + + class ATest { + void test(String str1, String str2) { + assertThat(str1).as("Reason").isNotEqualTo(str2); + } + } + """ + ) + ); + } + + @Test + void doNotConvertNestedMatchers() { + rewriteRun( + spec -> spec.recipe(new HamcrestNotMatcherToAssertJ("hasItems", "doesNotContain")), + //language=java + java( + """ + import java.util.List; + + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.equalTo; + import static org.hamcrest.Matchers.hasItems; + import static org.hamcrest.Matchers.not; + import static org.hamcrest.Matchers.startsWith; + + class ATest { + void test(List list) { + assertThat(list, not(hasItems(equalTo("a"), startsWith("b")))); + assertThat("Reason", list, not(hasItems(equalTo("a"), startsWith("b")))); + } + } + """ + ) + ); + } } diff --git a/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToAssertJTest.java b/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToAssertJTest.java index dc5eef067..c96f20448 100644 --- a/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToAssertJTest.java +++ b/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToAssertJTest.java @@ -929,5 +929,32 @@ void bar(%1$s type) { ) ); } + + @Test + void nestedMatchersAreNotPassedToAssertJ() { + //language=java + rewriteRun( + java( + """ + import java.util.List; + import java.util.Map; + + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.*; + + class A { + void test(List list, String[] array, Map map) { + assertThat(list, contains(equalTo("a"), startsWith("b"))); + assertThat(list, containsInAnyOrder(equalTo("a"), startsWith("b"))); + assertThat(list, hasItems(equalTo("a"), startsWith("b"))); + assertThat(list, not(hasItems(equalTo("a"), startsWith("b")))); + assertThat(array, arrayContaining(equalTo("a"), startsWith("b"))); + assertThat(map, hasEntry(equalTo("a"), startsWith("b"))); + } + } + """ + ) + ); + } } }