From 2c5179f8e61fd8533e360c0f95a688c9594b55d6 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 16:02:31 +0200 Subject: [PATCH 1/6] AssertTrueInstanceofToAssertInstanceOf: keep an explicit JUnit owner A qualified assertTrue call was rewritten to an unqualified assertInstanceOf plus a static import. Java resolves a compatible same-class or inherited method before a static import, so a test that declares its own assertInstanceOf silently invoked application code instead of JUnit's assertion; the qualified input no longer established that JUnit owns the call. Keep an explicit owner whenever the input was qualified. A JUnit 5 selector is retained only when it names the Assertions class itself, spelled as a simple or fully qualified type name, since a subtype or an instance-typed selector can hide assertInstanceOf with a declaration of its own; those fall back to the unqualified call with a static import, byte-identical to what main emits today, and the selector's now-unused class import is removed. A qualified JUnit 4 call is rewritten to the fully qualified org.junit.jupiter.api.Assertions.assertInstanceOf, because a bare Assertions simple name can itself be shadowed by a member type, a field, or a same-package type. An input that was already unqualified stays unqualified, so a same-named local declaration can still capture it; that case is deliberately out of scope. No existing test expectation changes, because every upstream fixture uses a static import. New tests cover the hiding same-class, inherited, and subtype owners, the instance-typed selector, both JUnit 4 forms, and generic, nested, and array targets. --- ...ssertTrueInstanceofToAssertInstanceOf.java | 67 ++- ...tTrueInstanceofToAssertInstanceOfTest.java | 472 ++++++++++++++++++ 2 files changed, 532 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java index 073c5cbe2..c8f696b9f 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java @@ -16,6 +16,7 @@ package org.openrewrite.java.testing.junit5; import lombok.Getter; +import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.Tree; @@ -29,6 +30,7 @@ import org.openrewrite.java.tree.JLeftPadded; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.Space; +import org.openrewrite.java.tree.TypeUtils; import org.openrewrite.java.tree.TypedTree; import org.openrewrite.marker.Markers; @@ -55,9 +57,25 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu TypedTree clazz; Expression expression; Expression reason; + Expression select = mi.getSelect(); + Expression retainedSelect; + String owner; if (junit5Matcher.matches(mi)) { maybeRemoveImport("org.junit.jupiter.api.Assertions.assertTrue"); + // Reuse the selector only when it names the `Assertions` class itself; there + // `assertInstanceOf` necessarily resolves to JUnit's declaration. A subtype can + // hide `assertInstanceOf` with a declaration of its own, and an instance-typed + // selector resolves against the variable's static type, so both fall back to + // the unqualified call with a static import, as for an unqualified input. + retainedSelect = isAssertionsClassReference(select) ? select : null; + owner = retainedSelect == null ? "" : "Assertions."; + if (retainedSelect == null && select != null) { + JavaType.FullyQualified selectType = TypeUtils.asFullyQualified(select.getType()); + if (selectType != null) { + maybeRemoveImport(selectType.getFullyQualifiedName()); + } + } Expression argument = mi.getArguments().get(0); if (mi.getArguments().size() == 1) { reason = null; @@ -76,6 +94,16 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } } else if (junit4Matcher.matches(mi)) { maybeRemoveImport("org.junit.Assert.assertTrue"); + // The selector denotes `Assert`, which does not declare `assertInstanceOf`. A + // qualified call keeps an explicit owner as the fully qualified + // `org.junit.jupiter.api.Assertions.assertInstanceOf`, because a bare + // `Assertions` simple name could itself be shadowed by a member type, an + // inherited member type, a field, or a same-package type. + retainedSelect = null; + owner = select == null ? "" : "org.junit.jupiter.api.Assertions."; + if (select != null) { + maybeRemoveImport("org.junit.Assert"); + } Expression argument; if (mi.getArguments().size() == 1) { reason = null; @@ -99,18 +127,43 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } - JavaTemplate template = JavaTemplate - .builder("assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)}" + (reason != null ? ", #{any(java.lang.String)})" : ")")) - .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "junit-jupiter-api-5", "junit-4")) - .staticImports("org.junit.jupiter.api.Assertions.assertInstanceOf") - .build(); + // An unqualified call is left unqualified; a same-named local declaration can still capture + // it, which this change does not address (see the PR description). + JavaTemplate.Builder templateBuilder = JavaTemplate + .builder(owner + "assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)}" + (reason != null ? ", #{any(java.lang.String)})" : ")")) + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "junit-jupiter-api-5", "junit-4")); + if (owner.isEmpty()) { + templateBuilder.staticImports("org.junit.jupiter.api.Assertions.assertInstanceOf"); + maybeAddImport("org.junit.jupiter.api.Assertions", "assertInstanceOf"); + } else if (retainedSelect != null) { + templateBuilder.imports("org.junit.jupiter.api.Assertions"); + } - maybeAddImport("org.junit.jupiter.api.Assertions", "assertInstanceOf"); TypedTree rawClazz = clazz instanceof J.ParameterizedType ? ((J.ParameterizedType) clazz).getClazz() : clazz; Expression classLiteral = toClassLiteral(rawClazz); - return reason != null ? + JavaTemplate template = templateBuilder.build(); + J.MethodInvocation replacement = reason != null ? template.apply(getCursor(), mi.getCoordinates().replace(), classLiteral, expression, reason) : template.apply(getCursor(), mi.getCoordinates().replace(), classLiteral, expression); + return retainedSelect == null ? replacement : replacement.withSelect(retainedSelect.withPrefix(Space.EMPTY)); + } + + /** + * True only when the selector is a reference to the {@code org.junit.jupiter.api.Assertions} + * class itself, spelled as a simple or fully qualified type name. A reference to a subtype or + * to a variable is rejected, because the emitted {@code assertInstanceOf} would resolve + * against that type, which may hide JUnit's method with a declaration of its own. + */ + private boolean isAssertionsClassReference(@Nullable Expression select) { + if (select instanceof J.Identifier) { + return ((J.Identifier) select).getFieldType() == null && + TypeUtils.isOfClassType(select.getType(), "org.junit.jupiter.api.Assertions"); + } + if (select instanceof J.FieldAccess) { + return ((J.FieldAccess) select).getName().getFieldType() == null && + TypeUtils.isOfClassType(select.getType(), "org.junit.jupiter.api.Assertions"); + } + return false; } private Expression toClassLiteral(TypedTree clazz) { diff --git a/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java b/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java index e4d26a2af..0fca25032 100644 --- a/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java +++ b/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java @@ -21,6 +21,7 @@ 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; @@ -262,4 +263,475 @@ void test() { """ )); } + + @Test + void qualifiedJUnit5NotCapturedBySameClassMethod() { + //language=java + rewriteRun( + java( + """ + class Test { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String); + } + } + """, + """ + class Test { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedJUnit5NotCapturedByInheritedMethod() { + //language=java + rewriteRun( + java( + """ + class BaseAssert { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + } + """ + ), + java( + """ + class ATest extends BaseAssert { + void test(Object value) { + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String); + } + } + """, + """ + class ATest extends BaseAssert { + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedJUnit5WithReasonNotCapturedByIncompatibleSameNameMethod() { + //language=java + rewriteRun( + java( + """ + class ATest { + static void assertInstanceOf(String message) { + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String, "not a String"); + } + } + """, + """ + class ATest { + static void assertInstanceOf(String message) { + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, "not a String"); + } + } + """ + )); + } + + @Test + void qualifiedJUnit5WithSupplierReason() { + //language=java + rewriteRun( + java( + """ + import org.junit.jupiter.api.Assertions; + + class ATest { + static void assertInstanceOf(Class type, Object value, java.util.function.Supplier message) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + Assertions.assertTrue(value instanceof String, () -> "not a String"); + } + } + """, + """ + import org.junit.jupiter.api.Assertions; + + class ATest { + static void assertInstanceOf(Class type, Object value, java.util.function.Supplier message) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + Assertions.assertInstanceOf(String.class, value, () -> "not a String"); + } + } + """ + )); + } + + @Test + void qualifiedJUnit5SubtypeSelectorHidingAssertInstanceOf() { + //language=java + rewriteRun( + java( + """ + public class BaseAssert extends org.junit.jupiter.api.Assertions { + public static T assertInstanceOf(Class expectedType, Object actualValue) { + throw new AssertionError("wrong owner"); + } + } + """ + ), + java( + """ + class ATest { + void test(Object value) { + BaseAssert.assertTrue(value instanceof String); + } + } + """, + """ + import static org.junit.jupiter.api.Assertions.assertInstanceOf; + + class ATest { + void test(Object value) { + assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedJUnit5SubtypeSelectorHidingAssertInstanceOfWithReason() { + //language=java + rewriteRun( + java( + """ + public class BaseAssert extends org.junit.jupiter.api.Assertions { + public static T assertInstanceOf(Class expectedType, Object actualValue, String message) { + throw new AssertionError("wrong owner"); + } + } + """ + ), + java( + """ + class ATest { + void test(Object value) { + BaseAssert.assertTrue(value instanceof String, "not a String"); + } + } + """, + """ + import static org.junit.jupiter.api.Assertions.assertInstanceOf; + + class ATest { + void test(Object value) { + assertInstanceOf(String.class, value, "not a String"); + } + } + """ + )); + } + + @Test + void qualifiedJUnit5InstanceSelector() { + //language=java + rewriteRun( + java( + """ + public class BaseAssert extends org.junit.jupiter.api.Assertions { + public static T assertInstanceOf(Class expectedType, Object actualValue) { + throw new AssertionError("wrong owner"); + } + } + """ + ), + java( + """ + class ATest { + void test(BaseAssert base, Object value) { + base.assertTrue(value instanceof String); + } + } + """, + """ + import static org.junit.jupiter.api.Assertions.assertInstanceOf; + + class ATest { + void test(BaseAssert base, Object value) { + assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedJUnit5SubtypeSelector() { + //language=java + rewriteRun( + java( + """ + package com.sample; + + public class MyAssertions extends org.junit.jupiter.api.Assertions { + } + """ + ), + java( + """ + package com.sample.test; + + import com.sample.MyAssertions; + + class ATest { + void test(Object value) { + MyAssertions.assertTrue(value instanceof String); + } + } + """, + """ + package com.sample.test; + + import static org.junit.jupiter.api.Assertions.assertInstanceOf; + + class ATest { + void test(Object value) { + assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedJUnit4() { + //language=java + rewriteRun( + java( + """ + import org.junit.Assert; + + class ATest { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + Assert.assertTrue(value instanceof String); + } + } + """, + """ + class ATest { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedJUnit4WithReason() { + //language=java + rewriteRun( + java( + """ + class ATest { + static void assertInstanceOf(Class type, Object value, String message) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.Assert.assertTrue("not a String", value instanceof String); + } + } + """, + """ + class ATest { + static void assertInstanceOf(Class type, Object value, String message) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, "not a String"); + } + } + """ + )); + } + + @Test + void qualifiedJUnit4NotCapturedByNestedAssertionsClass() { + //language=java + rewriteRun( + java( + """ + class ATest { + static class Assertions { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + } + + void test(Object value) { + org.junit.Assert.assertTrue(value instanceof String); + } + } + """, + """ + class ATest { + static class Assertions { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedJUnit4DoesNotShadowSamePackageAssertionsHelper() { + //language=java + rewriteRun( + java( + """ + package com.example; + + public class Assertions { + public static void assertThat(Object value) { + } + } + """ + ), + java( + """ + package com.example; + + class ATest { + void test(Object value) { + Assertions.assertThat(value); + org.junit.Assert.assertTrue(value instanceof String); + } + } + """, + """ + package com.example; + + class ATest { + void test(Object value) { + Assertions.assertThat(value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void qualifiedOwnerWithGenericNestedAndArrayTargets() { + //language=java + rewriteRun( + java( + """ + import java.util.List; + import java.util.Map; + + class ATest { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertTrue(value instanceof List); + org.junit.jupiter.api.Assertions.assertTrue(value instanceof Map.Entry); + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String[]); + } + } + """, + """ + import java.util.List; + import java.util.Map; + + class ATest { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(List.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(Map.Entry.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String[].class, value); + } + } + """ + )); + } + + @Test + void qualifiedOutputIsStableOnASecondRun() { + //language=java + rewriteRun( + java( + """ + class Test { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + } + } + """ + )); + } + + @Test + void noChangeWhenAssertTrueIsNotAttributed() { + //language=java + rewriteRun( + spec -> spec + .parser(JavaParser.fromJavaVersion()) + .typeValidationOptions(TypeValidation.none()), + java( + """ + class Test { + void test(Object value) { + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String); + } + } + """ + )); + } } From ab2500b54da3b6bae0f86a5ca8869b9a78210e61 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 09:38:57 +0200 Subject: [PATCH 2/6] Qualify the emitted call for every qualified input, not only Assertions selectors --- ...ssertTrueInstanceofToAssertInstanceOf.java | 54 +++++++++---------- ...tTrueInstanceofToAssertInstanceOfTest.java | 52 +++++++++++++----- 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java index c8f696b9f..37ca0892d 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java @@ -58,23 +58,20 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu Expression expression; Expression reason; Expression select = mi.getSelect(); - Expression retainedSelect; - String owner; + // A qualified call keeps an explicit owner, so that the emitted `assertInstanceOf` resolves to + // JUnit's declaration rather than to one the calling class declares or inherits. The selector is + // reused only when it names `Assertions` itself; any other selector is replaced by the fully + // qualified name, as a subtype can hide `assertInstanceOf` with a declaration of its own, and a + // bare `Assertions` could be shadowed by a member type, an inherited member type, a field, or a + // same-package type. + Expression retainedSelect = isAssertionsClassReference(select) ? select : null; + String owner = retainedSelect != null ? "Assertions." : + select == null ? "" : "org.junit.jupiter.api.Assertions."; if (junit5Matcher.matches(mi)) { maybeRemoveImport("org.junit.jupiter.api.Assertions.assertTrue"); - // Reuse the selector only when it names the `Assertions` class itself; there - // `assertInstanceOf` necessarily resolves to JUnit's declaration. A subtype can - // hide `assertInstanceOf` with a declaration of its own, and an instance-typed - // selector resolves against the variable's static type, so both fall back to - // the unqualified call with a static import, as for an unqualified input. - retainedSelect = isAssertionsClassReference(select) ? select : null; - owner = retainedSelect == null ? "" : "Assertions."; - if (retainedSelect == null && select != null) { - JavaType.FullyQualified selectType = TypeUtils.asFullyQualified(select.getType()); - if (selectType != null) { - maybeRemoveImport(selectType.getFullyQualifiedName()); - } + if (retainedSelect == null) { + maybeRemoveSelectTypeImport(select); } Expression argument = mi.getArguments().get(0); if (mi.getArguments().size() == 1) { @@ -94,16 +91,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } } else if (junit4Matcher.matches(mi)) { maybeRemoveImport("org.junit.Assert.assertTrue"); - // The selector denotes `Assert`, which does not declare `assertInstanceOf`. A - // qualified call keeps an explicit owner as the fully qualified - // `org.junit.jupiter.api.Assertions.assertInstanceOf`, because a bare - // `Assertions` simple name could itself be shadowed by a member type, an - // inherited member type, a field, or a same-package type. - retainedSelect = null; - owner = select == null ? "" : "org.junit.jupiter.api.Assertions."; - if (select != null) { - maybeRemoveImport("org.junit.Assert"); - } + maybeRemoveSelectTypeImport(select); Expression argument; if (mi.getArguments().size() == 1) { reason = null; @@ -127,8 +115,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } - // An unqualified call is left unqualified; a same-named local declaration can still capture - // it, which this change does not address (see the PR description). + // An unqualified call is left unqualified; a same-named declaration in scope can still capture it JavaTemplate.Builder templateBuilder = JavaTemplate .builder(owner + "assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)}" + (reason != null ? ", #{any(java.lang.String)})" : ")")) .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "junit-jupiter-api-5", "junit-4")); @@ -148,11 +135,18 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu return retainedSelect == null ? replacement : replacement.withSelect(retainedSelect.withPrefix(Space.EMPTY)); } + private void maybeRemoveSelectTypeImport(@Nullable Expression select) { + if (select != null) { + JavaType.FullyQualified selectType = TypeUtils.asFullyQualified(select.getType()); + if (selectType != null) { + maybeRemoveImport(selectType.getFullyQualifiedName()); + } + } + } + /** - * True only when the selector is a reference to the {@code org.junit.jupiter.api.Assertions} - * class itself, spelled as a simple or fully qualified type name. A reference to a subtype or - * to a variable is rejected, because the emitted {@code assertInstanceOf} would resolve - * against that type, which may hide JUnit's method with a declaration of its own. + * @return whether the selector references {@code org.junit.jupiter.api.Assertions} itself, spelled as + * a simple or fully qualified type name; a subtype or a variable is rejected. */ private boolean isAssertionsClassReference(@Nullable Expression select) { if (select instanceof J.Identifier) { diff --git a/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java b/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java index 0fca25032..5bc980cde 100644 --- a/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java +++ b/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java @@ -409,11 +409,9 @@ void test(Object value) { } """, """ - import static org.junit.jupiter.api.Assertions.assertInstanceOf; - class ATest { void test(Object value) { - assertInstanceOf(String.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); } } """ @@ -442,11 +440,9 @@ void test(Object value) { } """, """ - import static org.junit.jupiter.api.Assertions.assertInstanceOf; - class ATest { void test(Object value) { - assertInstanceOf(String.class, value, "not a String"); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, "not a String"); } } """ @@ -475,11 +471,9 @@ void test(BaseAssert base, Object value) { } """, """ - import static org.junit.jupiter.api.Assertions.assertInstanceOf; - class ATest { void test(BaseAssert base, Object value) { - assertInstanceOf(String.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); } } """ @@ -513,11 +507,9 @@ void test(Object value) { """ package com.sample.test; - import static org.junit.jupiter.api.Assertions.assertInstanceOf; - class ATest { void test(Object value) { - assertInstanceOf(String.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); } } """ @@ -586,6 +578,42 @@ void test(Object value) { )); } + @Test + void qualifiedJUnit4SubtypeSelector() { + //language=java + rewriteRun( + java( + """ + package com.sample; + + public class MyAssert extends org.junit.Assert { + } + """ + ), + java( + """ + package com.sample.test; + + import com.sample.MyAssert; + + class ATest { + void test(Object value) { + MyAssert.assertTrue(value instanceof String); + } + } + """, + """ + package com.sample.test; + + class ATest { + void test(Object value) { + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + } + } + """ + )); + } + @Test void qualifiedJUnit4NotCapturedByNestedAssertionsClass() { //language=java From 7c15799bdc8eb3c76edba1a6fa13eec73ade636c Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 12 Aug 2026 00:35:36 +0200 Subject: [PATCH 3/6] Trim commentary --- .../AssertTrueInstanceofToAssertInstanceOf.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java index 37ca0892d..a32b73599 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java @@ -58,12 +58,9 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu Expression expression; Expression reason; Expression select = mi.getSelect(); - // A qualified call keeps an explicit owner, so that the emitted `assertInstanceOf` resolves to - // JUnit's declaration rather than to one the calling class declares or inherits. The selector is - // reused only when it names `Assertions` itself; any other selector is replaced by the fully - // qualified name, as a subtype can hide `assertInstanceOf` with a declaration of its own, and a - // bare `Assertions` could be shadowed by a member type, an inherited member type, a field, or a - // same-package type. + // Keep an explicit owner so the emitted call resolves to JUnit's declaration rather than one the + // calling class declares or inherits. Only a selector naming `Assertions` itself is reused; a + // subtype can hide `assertInstanceOf`, and a bare `Assertions` can be shadowed several ways Expression retainedSelect = isAssertionsClassReference(select) ? select : null; String owner = retainedSelect != null ? "Assertions." : select == null ? "" : "org.junit.jupiter.api.Assertions."; @@ -145,8 +142,8 @@ private void maybeRemoveSelectTypeImport(@Nullable Expression select) { } /** - * @return whether the selector references {@code org.junit.jupiter.api.Assertions} itself, spelled as - * a simple or fully qualified type name; a subtype or a variable is rejected. + * @return whether the selector names {@code org.junit.jupiter.api.Assertions} itself, simple or fully + * qualified; a subtype or a variable is rejected. */ private boolean isAssertionsClassReference(@Nullable Expression select) { if (select instanceof J.Identifier) { From 5eea4fa460f2d80a871131e2977d5182441c9fd2 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 15:16:18 +0200 Subject: [PATCH 4/6] Consolidate qualified assertion tests --- ...tTrueInstanceofToAssertInstanceOfTest.java | 256 ++++-------------- 1 file changed, 55 insertions(+), 201 deletions(-) diff --git a/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java b/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java index 5bc980cde..47c329fd2 100644 --- a/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java +++ b/src/test/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOfTest.java @@ -265,29 +265,81 @@ void test() { } @Test - void qualifiedJUnit5NotCapturedBySameClassMethod() { + void qualifiedCallsUseJupiterOwner() { //language=java rewriteRun( java( """ - class Test { + import java.util.function.Supplier; + + class ATest { static void assertInstanceOf(Class type, Object value) { throw new AssertionError("wrong owner"); } + static void assertInstanceOf(String message) { + } + + static void assertInstanceOf(Class type, Object value, String message) { + throw new AssertionError("wrong owner"); + } + + static void assertInstanceOf(Class type, Object value, Supplier message) { + throw new AssertionError("wrong owner"); + } + + static class Assertions { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + } + void test(Object value) { org.junit.jupiter.api.Assertions.assertTrue(value instanceof String); + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String, "not a String"); + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String, () -> "not a String"); + org.junit.Assert.assertTrue(value instanceof String); + org.junit.Assert.assertTrue("not a String", value instanceof String); + org.junit.jupiter.api.Assertions.assertTrue(value instanceof java.util.List); + org.junit.jupiter.api.Assertions.assertTrue(value instanceof java.util.Map.Entry); + org.junit.jupiter.api.Assertions.assertTrue(value instanceof String[]); } } """, """ - class Test { + import java.util.function.Supplier; + + class ATest { static void assertInstanceOf(Class type, Object value) { throw new AssertionError("wrong owner"); } + static void assertInstanceOf(String message) { + } + + static void assertInstanceOf(Class type, Object value, String message) { + throw new AssertionError("wrong owner"); + } + + static void assertInstanceOf(Class type, Object value, Supplier message) { + throw new AssertionError("wrong owner"); + } + + static class Assertions { + static void assertInstanceOf(Class type, Object value) { + throw new AssertionError("wrong owner"); + } + } + void test(Object value) { org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, "not a String"); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, () -> "not a String"); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, "not a String"); + org.junit.jupiter.api.Assertions.assertInstanceOf(java.util.List.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(java.util.Map.Entry.class, value); + org.junit.jupiter.api.Assertions.assertInstanceOf(String[].class, value); } } """ @@ -325,68 +377,6 @@ void test(Object value) { )); } - @Test - void qualifiedJUnit5WithReasonNotCapturedByIncompatibleSameNameMethod() { - //language=java - rewriteRun( - java( - """ - class ATest { - static void assertInstanceOf(String message) { - } - - void test(Object value) { - org.junit.jupiter.api.Assertions.assertTrue(value instanceof String, "not a String"); - } - } - """, - """ - class ATest { - static void assertInstanceOf(String message) { - } - - void test(Object value) { - org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, "not a String"); - } - } - """ - )); - } - - @Test - void qualifiedJUnit5WithSupplierReason() { - //language=java - rewriteRun( - java( - """ - import org.junit.jupiter.api.Assertions; - - class ATest { - static void assertInstanceOf(Class type, Object value, java.util.function.Supplier message) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - Assertions.assertTrue(value instanceof String, () -> "not a String"); - } - } - """, - """ - import org.junit.jupiter.api.Assertions; - - class ATest { - static void assertInstanceOf(Class type, Object value, java.util.function.Supplier message) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - Assertions.assertInstanceOf(String.class, value, () -> "not a String"); - } - } - """ - )); - } - @Test void qualifiedJUnit5SubtypeSelectorHidingAssertInstanceOf() { //language=java @@ -516,68 +506,6 @@ void test(Object value) { )); } - @Test - void qualifiedJUnit4() { - //language=java - rewriteRun( - java( - """ - import org.junit.Assert; - - class ATest { - static void assertInstanceOf(Class type, Object value) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - Assert.assertTrue(value instanceof String); - } - } - """, - """ - class ATest { - static void assertInstanceOf(Class type, Object value) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); - } - } - """ - )); - } - - @Test - void qualifiedJUnit4WithReason() { - //language=java - rewriteRun( - java( - """ - class ATest { - static void assertInstanceOf(Class type, Object value, String message) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - org.junit.Assert.assertTrue("not a String", value instanceof String); - } - } - """, - """ - class ATest { - static void assertInstanceOf(Class type, Object value, String message) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value, "not a String"); - } - } - """ - )); - } - @Test void qualifiedJUnit4SubtypeSelector() { //language=java @@ -614,40 +542,6 @@ void test(Object value) { )); } - @Test - void qualifiedJUnit4NotCapturedByNestedAssertionsClass() { - //language=java - rewriteRun( - java( - """ - class ATest { - static class Assertions { - static void assertInstanceOf(Class type, Object value) { - throw new AssertionError("wrong owner"); - } - } - - void test(Object value) { - org.junit.Assert.assertTrue(value instanceof String); - } - } - """, - """ - class ATest { - static class Assertions { - static void assertInstanceOf(Class type, Object value) { - throw new AssertionError("wrong owner"); - } - } - - void test(Object value) { - org.junit.jupiter.api.Assertions.assertInstanceOf(String.class, value); - } - } - """ - )); - } - @Test void qualifiedJUnit4DoesNotShadowSamePackageAssertionsHelper() { //language=java @@ -686,46 +580,6 @@ void test(Object value) { )); } - @Test - void qualifiedOwnerWithGenericNestedAndArrayTargets() { - //language=java - rewriteRun( - java( - """ - import java.util.List; - import java.util.Map; - - class ATest { - static void assertInstanceOf(Class type, Object value) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - org.junit.jupiter.api.Assertions.assertTrue(value instanceof List); - org.junit.jupiter.api.Assertions.assertTrue(value instanceof Map.Entry); - org.junit.jupiter.api.Assertions.assertTrue(value instanceof String[]); - } - } - """, - """ - import java.util.List; - import java.util.Map; - - class ATest { - static void assertInstanceOf(Class type, Object value) { - throw new AssertionError("wrong owner"); - } - - void test(Object value) { - org.junit.jupiter.api.Assertions.assertInstanceOf(List.class, value); - org.junit.jupiter.api.Assertions.assertInstanceOf(Map.Entry.class, value); - org.junit.jupiter.api.Assertions.assertInstanceOf(String[].class, value); - } - } - """ - )); - } - @Test void qualifiedOutputIsStableOnASecondRun() { //language=java From 9542d21d3337506189b4166f149c07c7edfb59e1 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 23:05:36 +0200 Subject: [PATCH 5/6] Name the assertion qualifier fragments and the unqualified-call sentinel --- .../AssertTrueInstanceofToAssertInstanceOf.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java index a32b73599..b9ccbdc2c 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java @@ -38,6 +38,11 @@ import static java.util.Collections.singletonList; public class AssertTrueInstanceofToAssertInstanceOf extends Recipe { + + // Template prefixes for the emitted call's qualifier, including the trailing dot + private static final String SIMPLE_ASSERTIONS_QUALIFIER = "Assertions."; + private static final String FULLY_QUALIFIED_ASSERTIONS_QUALIFIER = "org.junit.jupiter.api.Assertions."; + @Getter final String displayName = "`assertTrue(x instanceof y)` to `assertInstanceOf(y.class, x)`"; @@ -62,8 +67,9 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu // calling class declares or inherits. Only a selector naming `Assertions` itself is reused; a // subtype can hide `assertInstanceOf`, and a bare `Assertions` can be shadowed several ways Expression retainedSelect = isAssertionsClassReference(select) ? select : null; - String owner = retainedSelect != null ? "Assertions." : - select == null ? "" : "org.junit.jupiter.api.Assertions."; + boolean unqualifiedCall = retainedSelect == null && select == null; + String owner = retainedSelect != null ? SIMPLE_ASSERTIONS_QUALIFIER : + unqualifiedCall ? "" : FULLY_QUALIFIED_ASSERTIONS_QUALIFIER; if (junit5Matcher.matches(mi)) { maybeRemoveImport("org.junit.jupiter.api.Assertions.assertTrue"); @@ -116,7 +122,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu JavaTemplate.Builder templateBuilder = JavaTemplate .builder(owner + "assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)}" + (reason != null ? ", #{any(java.lang.String)})" : ")")) .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "junit-jupiter-api-5", "junit-4")); - if (owner.isEmpty()) { + if (unqualifiedCall) { templateBuilder.staticImports("org.junit.jupiter.api.Assertions.assertInstanceOf"); maybeAddImport("org.junit.jupiter.api.Assertions", "assertInstanceOf"); } else if (retainedSelect != null) { From 2850ce9aa0d821388adcfaaa6ddb63ebba1b8da3 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 23:42:44 +0200 Subject: [PATCH 6/6] Assemble the template from complete snippets and plain qualifier names --- .../AssertTrueInstanceofToAssertInstanceOf.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java index b9ccbdc2c..df7e702b1 100644 --- a/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java +++ b/src/main/java/org/openrewrite/java/testing/junit5/AssertTrueInstanceofToAssertInstanceOf.java @@ -39,9 +39,11 @@ public class AssertTrueInstanceofToAssertInstanceOf extends Recipe { - // Template prefixes for the emitted call's qualifier, including the trailing dot - private static final String SIMPLE_ASSERTIONS_QUALIFIER = "Assertions."; - private static final String FULLY_QUALIFIED_ASSERTIONS_QUALIFIER = "org.junit.jupiter.api.Assertions."; + private static final String SIMPLE_ASSERTIONS_QUALIFIER = "Assertions"; + private static final String FULLY_QUALIFIED_ASSERTIONS_QUALIFIER = "org.junit.jupiter.api.Assertions"; + + private static final String ASSERT_INSTANCE_OF_TEMPLATE = "assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)})"; + private static final String ASSERT_INSTANCE_OF_WITH_REASON_TEMPLATE = "assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)}, #{any(java.lang.String)})"; @Getter final String displayName = "`assertTrue(x instanceof y)` to `assertInstanceOf(y.class, x)`"; @@ -68,8 +70,6 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu // subtype can hide `assertInstanceOf`, and a bare `Assertions` can be shadowed several ways Expression retainedSelect = isAssertionsClassReference(select) ? select : null; boolean unqualifiedCall = retainedSelect == null && select == null; - String owner = retainedSelect != null ? SIMPLE_ASSERTIONS_QUALIFIER : - unqualifiedCall ? "" : FULLY_QUALIFIED_ASSERTIONS_QUALIFIER; if (junit5Matcher.matches(mi)) { maybeRemoveImport("org.junit.jupiter.api.Assertions.assertTrue"); @@ -119,8 +119,13 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu // An unqualified call is left unqualified; a same-named declaration in scope can still capture it + String templateCode = reason != null ? ASSERT_INSTANCE_OF_WITH_REASON_TEMPLATE : ASSERT_INSTANCE_OF_TEMPLATE; + if (!unqualifiedCall) { + String qualifier = retainedSelect != null ? SIMPLE_ASSERTIONS_QUALIFIER : FULLY_QUALIFIED_ASSERTIONS_QUALIFIER; + templateCode = qualifier + "." + templateCode; + } JavaTemplate.Builder templateBuilder = JavaTemplate - .builder(owner + "assertInstanceOf(#{any(java.lang.Class)}, #{any(java.lang.Object)}" + (reason != null ? ", #{any(java.lang.String)})" : ")")) + .builder(templateCode) .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "junit-jupiter-api-5", "junit-4")); if (unqualifiedCall) { templateBuilder.staticImports("org.junit.jupiter.api.Assertions.assertInstanceOf");