From 3969d898da5b67a60fad2673a4f28ce6142c3b2a Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 24 Oct 2026 12:49:37 +0200 Subject: [PATCH] Track Hamcrest `not(..)` negation locally instead of on the `ExecutionContext` `RemoveNotMatcherVisitor` stored whether a matcher was negated on the `ExecutionContext`, keyed by the printed matcher, so a negation leaked into later assertions with the same matcher text: `assertThat(a, not(nullValue())); assertThat(b, nullValue());` became `assertNotNull(a); assertNotNull(b);`, and `not(not(x))` threw a `NullPointerException`. `HamcrestMatcherToJUnit5` and `HamcrestInstanceOfToJUnit5` now flip the negation for each `not(..)` layer they unwrap, and the hamcrest tests no longer disable the immutable execution context check that would have caught this. --- .../hamcrest/HamcrestInstanceOfToJUnit5.java | 4 +- .../hamcrest/HamcrestMatcherToJUnit5.java | 4 +- .../hamcrest/RemoveNotMatcherVisitor.java | 43 ++---------- .../HamcrestInstanceOfToJUnit5Test.java | 4 +- .../hamcrest/HamcrestMatcherToJUnit5Test.java | 70 ++++++++++++++++--- .../hamcrest/MigrateHamcrestToJUnitTest.java | 2 - .../hamcrest/RemoveNotMatcherTest.java | 4 +- 7 files changed, 75 insertions(+), 56 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5.java b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5.java index 55f00cb76..f87add2c0 100644 --- a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5.java +++ b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5.java @@ -70,15 +70,15 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Execution } J.MethodInvocation matcherInvocation = (J.MethodInvocation) hamcrestMatcher; + boolean logicalContext = true; while (RemoveNotMatcherVisitor.NOT_MATCHER.matches(matcherInvocation)) { + logicalContext = !logicalContext; maybeRemoveImport("org.hamcrest.Matchers.not"); maybeRemoveImport("org.hamcrest.CoreMatchers.not"); matcherInvocation = (J.MethodInvocation) new RemoveNotMatcherVisitor().visit(matcherInvocation, ctx, getCursor()); } if (INSTANCE_OF_MATCHER.matches(matcherInvocation) || IS_A_MATCHER.matches(matcherInvocation)) { - boolean logicalContext = RemoveNotMatcherVisitor.getLogicalContext(matcherInvocation, ctx); - String templateString = (logicalContext ? "assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)}" : "assertFalse(#{any(java.lang.Class)}.isAssignableFrom(#{any(java.lang.Object)}.getClass())") + diff --git a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5.java b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5.java index ab08eeabb..aa993a51f 100644 --- a/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5.java +++ b/src/main/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5.java @@ -152,7 +152,9 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu J.MethodInvocation matcherInvocation = (J.MethodInvocation) hamcrestMatcher; maybeRemoveImport("org.hamcrest.MatcherAssert.assertThat"); + boolean logicalContext = true; while (RemoveNotMatcherVisitor.NOT_MATCHER.matches(matcherInvocation)) { + logicalContext = !logicalContext; maybeRemoveImport("org.hamcrest.Matchers.not"); maybeRemoveImport("org.hamcrest.CoreMatchers.not"); matcherInvocation = (J.MethodInvocation) new RemoveNotMatcherVisitor().visit(matcherInvocation, ctx, getCursor()); @@ -165,8 +167,6 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } } - boolean logicalContext = RemoveNotMatcherVisitor.getLogicalContext(matcherInvocation, ctx); - Replacement replacement; try { replacement = Replacement.valueOf(matcherInvocation.getSimpleName().toUpperCase()); diff --git a/src/main/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherVisitor.java b/src/main/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherVisitor.java index 5d3aebfb4..a3ef34ff5 100644 --- a/src/main/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherVisitor.java +++ b/src/main/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherVisitor.java @@ -22,54 +22,25 @@ import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.tree.J; -import java.security.InvalidParameterException; import java.util.Objects; class RemoveNotMatcherVisitor extends JavaIsoVisitor { static final MethodMatcher NOT_MATCHER = new MethodMatcher("org.hamcrest.*Matchers not(..)"); - public static boolean getLogicalContext(J.MethodInvocation mi, ExecutionContext ctx) throws InvalidParameterException { - Object msg = ctx.getMessage(mi.toString()); - if (msg == null) { - return true; - } - if (msg instanceof Boolean) { - return (Boolean) msg; - } - throw new InvalidParameterException(); - } - @Override - @SuppressWarnings("ConstantConditions") public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx) { if (NOT_MATCHER.matches(mi)) { - boolean logicalContext; - if (ctx.pollMessage(mi.toString()) != null) { - logicalContext = ctx.getMessage(mi.toString()); - } else { - logicalContext = true; - } - maybeRemoveImport("org.hamcrest.Matchers.not"); - J.MethodInvocation result; if (Objects.requireNonNull(mi.getArguments().get(0).getType()).toString().startsWith("org.hamcrest")) { - result = mi.getArguments().get(0).withPrefix(mi.getPrefix()); - } else { - JavaTemplate template = JavaTemplate.builder("equalTo(#{any(java.lang.Object)})") - .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "hamcrest-3")) - .staticImports("org.hamcrest.Matchers.equalTo") - .build(); - maybeAddImport("org.hamcrest.Matchers", "equalTo"); - result = template.apply(getCursor(), mi.getCoordinates().replace(), mi.getArguments().get(0)); + return mi.getArguments().get(0).withPrefix(mi.getPrefix()); } - - ctx.putMessage(result.toString(), !logicalContext); // FIXME Do not store on the use execution context - - return result; - } - if (ctx.pollMessage(mi.toString()) == null) { - ctx.putMessage(mi.toString(), true); // FIXME Do not store on the use execution context + maybeAddImport("org.hamcrest.Matchers", "equalTo"); + return JavaTemplate.builder("equalTo(#{any(java.lang.Object)})") + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "hamcrest-3")) + .staticImports("org.hamcrest.Matchers.equalTo") + .build() + .apply(getCursor(), mi.getCoordinates().replace(), mi.getArguments().get(0)); } return super.visitMethodInvocation(mi, ctx); } diff --git a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5Test.java b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5Test.java index da7fbf9f0..e8d28841b 100644 --- a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5Test.java +++ b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestInstanceOfToJUnit5Test.java @@ -26,7 +26,6 @@ import java.util.concurrent.TimeUnit; import static org.openrewrite.java.Assertions.java; -import static org.openrewrite.test.TypeValidation.all; class HamcrestInstanceOfToJUnit5Test implements RewriteTest { @Override @@ -34,8 +33,7 @@ public void defaults(RecipeSpec spec) { spec .parser(JavaParser.fromJavaVersion() .classpathFromResources(new InMemoryExecutionContext(), "junit-jupiter-api-5", "hamcrest-3")) - .recipe(new HamcrestInstanceOfToJUnit5()) - .typeValidationOptions(all().immutableExecutionContext(false)); + .recipe(new HamcrestInstanceOfToJUnit5()); } @DocumentExample diff --git a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5Test.java b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5Test.java index 6261af4f3..a4665505f 100644 --- a/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5Test.java +++ b/src/test/java/org/openrewrite/java/testing/hamcrest/HamcrestMatcherToJUnit5Test.java @@ -28,7 +28,6 @@ import static org.openrewrite.java.Assertions.java; import static org.openrewrite.kotlin.Assertions.kotlin; -import static org.openrewrite.test.TypeValidation.all; class HamcrestMatcherToJUnit5Test implements RewriteTest { @Override @@ -128,7 +127,6 @@ void testEquals() { void notEqualToString() { //language=java rewriteRun( - spec -> spec.typeValidationOptions(all().immutableExecutionContext(false)), java( """ import org.junit.jupiter.api.Test; @@ -167,7 +165,6 @@ void testEquals() { void notWithBareValue() { //language=java rewriteRun( - spec -> spec.typeValidationOptions(all().immutableExecutionContext(false)), java( """ import org.junit.jupiter.api.Test; @@ -206,7 +203,6 @@ void testEquals() { void notFromCoreMatchers() { //language=java rewriteRun( - spec -> spec.typeValidationOptions(all().immutableExecutionContext(false)), java( """ import org.junit.jupiter.api.Test; @@ -241,6 +237,67 @@ void testEquals() { ); } + @Test + void negationDoesNotLeakIntoLaterAssertions() { + //language=java + rewriteRun( + java( + """ + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.not; + import static org.hamcrest.Matchers.nullValue; + + class ATest { + void test(Object a, Object b) { + assertThat(a, not(nullValue())); + assertThat(b, nullValue()); + } + } + """, + """ + import static org.junit.jupiter.api.Assertions.assertNotNull; + import static org.junit.jupiter.api.Assertions.assertNull; + + class ATest { + void test(Object a, Object b) { + assertNotNull(a); + assertNull(b); + } + } + """ + ) + ); + } + + @Test + void doubleNegation() { + //language=java + rewriteRun( + java( + """ + import static org.hamcrest.MatcherAssert.assertThat; + import static org.hamcrest.Matchers.not; + import static org.hamcrest.Matchers.nullValue; + + class ATest { + void test(Object a) { + assertThat(a, not(not(nullValue()))); + } + } + """, + """ + import static org.junit.jupiter.api.Assertions.assertNull; + + class ATest { + void test(Object a) { + assertNull(a); + } + } + """ + ) + ); + } + @Test void greaterThan() { //language=java @@ -549,7 +606,6 @@ void testNullValue() { void sameInstance() { //language=java rewriteRun( - spec -> spec.typeValidationOptions(all().immutableExecutionContext(false)), java( """ import org.junit.jupiter.api.Test; @@ -1062,9 +1118,7 @@ fun testEquals() { @Test void notEqualToKotlin() { rewriteRun( - // Unwrapping not() stores logical context on the ExecutionContext (see RemoveNotMatcherVisitor) - spec -> spec.typeValidationOptions(all().immutableExecutionContext(false)) - .parser(KotlinParser.builder() + spec -> spec.parser(KotlinParser.builder() .classpathFromResources(new InMemoryExecutionContext(), "junit-jupiter-api-5", "hamcrest-3")), //language=kotlin kotlin( diff --git a/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToJUnitTest.java b/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToJUnitTest.java index f5bbf8f09..d7d1ecf1e 100644 --- a/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToJUnitTest.java +++ b/src/test/java/org/openrewrite/java/testing/hamcrest/MigrateHamcrestToJUnitTest.java @@ -23,7 +23,6 @@ import org.openrewrite.test.RewriteTest; import static org.openrewrite.java.Assertions.java; -import static org.openrewrite.test.TypeValidation.all; class MigrateHamcrestToJUnitTest implements RewriteTest { @Override @@ -39,7 +38,6 @@ public void defaults(RecipeSpec spec) { void equalToString() { //language=java rewriteRun( - spec -> spec.typeValidationOptions(all().immutableExecutionContext(false)), java( """ import org.junit.jupiter.api.Test; diff --git a/src/test/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherTest.java b/src/test/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherTest.java index 52a69839a..9169a8c00 100644 --- a/src/test/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherTest.java +++ b/src/test/java/org/openrewrite/java/testing/hamcrest/RemoveNotMatcherTest.java @@ -24,7 +24,6 @@ import static org.openrewrite.java.Assertions.java; import static org.openrewrite.test.RewriteTest.toRecipe; -import static org.openrewrite.test.TypeValidation.all; class RemoveNotMatcherTest implements RewriteTest { @Override @@ -32,8 +31,7 @@ public void defaults(RecipeSpec spec) { spec .parser(JavaParser.fromJavaVersion() .classpathFromResources(new InMemoryExecutionContext(), "junit-jupiter-api-5", "hamcrest-3")) - .recipe(toRecipe(RemoveNotMatcherVisitor::new)) - .typeValidationOptions(all().immutableExecutionContext(false)); + .recipe(toRecipe(RemoveNotMatcherVisitor::new)); } @DocumentExample