Keep an explicit JUnit owner when migrating a qualified assertTrue(x instanceof Y)

[martinfrancois]

Suggested review order: 37 of 52 (Score: 2)
Review first: openrewrite/rewrite-static-analysis#974

What's changed?

A call is qualified when the source writes a qualifier in front of the method name, as in Assertions.assertTrue(...); the recipe's code calls that qualifier the owner. For such a call, AssertTrueInstanceofToAssertInstanceOf now writes a qualifier in front of the assertInstanceOf call it emits as well, so the emitted call resolves to JUnit's assertInstanceOf, as the input call resolved to JUnit's assertTrue.

Before this change, the recipe emitted an unqualified assertInstanceOf call plus a static import of org.junit.jupiter.api.Assertions.assertInstanceOf for every input call, qualified or not. An unqualified call binds to a method declared in or inherited by the enclosing class before a static import of that name is considered (JLS 6.5.7.1), so where the calling class has its own assertInstanceOf, the emitted call runs that one.

Before

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);
    }
}

Actual after the recipe

Using main today. It compiles, but the emitted call now runs the assertInstanceOf method declared in Test, not JUnit's:

import static org.junit.jupiter.api.Assertions.assertInstanceOf;

class Test {
    static void assertInstanceOf(Class<?> type, Object value) {
        throw new AssertionError("wrong owner");
    }

    void test(Object value) {
        assertInstanceOf(String.class, value);
    }
}

Expected after the recipe

The emitted call runs JUnit's assertInstanceOf:

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);
    }
}

One rule now covers both the JUnit 4 and the JUnit 5 input:

• An unqualified call is migrated as before this change, to an unqualified assertInstanceOf plus a static import.
• A call qualified by Assertions itself, as a simple or fully qualified name, keeps that qualifier. There assertInstanceOf necessarily resolves to JUnit's declaration.
• A call qualified by anything else - org.junit.Assert, a subclass of Assertions, a variable - is migrated to the fully qualified org.junit.jupiter.api.Assertions.assertInstanceOf(...). Reusing such a qualifier is unsafe: a subclass can hide assertInstanceOf with a static declaration of its own (JLS 8.4.8.2), and the simple name Assertions can resolve to a member type, a field, or a same-package type, which qualifiedJUnit4NotCapturedByNestedAssertionsClass and qualifiedJUnit4DoesNotShadowSamePackageAssertionsHelper pin down. When dropping the qualifier leaves the import of the qualifier's own type unused, that import is now removed too.

What's your motivation?

Recipe: org.openrewrite.java.testing.junit5.AssertTrueInstanceofToAssertInstanceOf.

The output that main produces today compiles, so nothing fails at build time; what changes is the behaviour at run time. Call test("value") on the example above: against the input it returns normally, because "value" instanceof String is true and JUnit's assertTrue passes, and against main's output it throws AssertionError("wrong owner").

Which of the two ways a real suite goes wrong depends on the calling class's own assertInstanceOf. One that returns without checking makes the assertion vacuous, so the test keeps passing while no longer testing what it used to; one that throws, as here, fails the test with its own message instead of JUnit's. A migration must not change which method a call binds to. Reproduced on 3.43.0 and on a snapshot from main at 96ec8d6.

Anything in particular you'd like reviewers to focus on?

No existing test expectation changed: the six existing tests are untouched, including the one marked @DocumentExample, so the generated META-INF/rewrite/examples.yml needs no edit. All six of those inputs call assertTrue through a static import, with no qualifier, so the new handling of qualifiers never fires on them.

Three limits:

• An unqualified input call is still not protected: the recipe emits an unqualified call for it both before and after this change, so an assertInstanceOf declared in or inherited by the calling class still wins over the static import there. Qualifying those calls too is a separate decision, left out here.
• For an instance qualifier such as getAssertions().assertTrue(...), the receiver expression is dropped, so any side effect of getAssertions() is lost. The recipe already did that.
• If a field or a variable named org is in scope at the call site, the fully qualified output does not compile, because the leading org then resolves to that field or variable rather than to the package (JLS 6.5.2). This adds no new kind of failure: wherever such a name is in scope, every other fully qualified name starting with org. is already unusable in the same way.

Two behaviours here are inherited rather than new. A file carrying no type information matches neither of the recipe's two MethodMatchers and is left alone, which noChangeWhenAssertTrueIsNotAttributed pins down; and a JUnit 5 call whose qualifier is not attributed fails the isAssertionsClassReference check, so it takes the fully qualified path.

Any additional context

This change adds 10 test methods to AssertTrueInstanceofToAssertInstanceOfTest, taking the focused class from 6 to 16 executions. Those methods cover 16 scenarios. Without the code change, 8 methods fail and represent 14 failing scenarios. qualifiedCallsUseJupiterOwner contains the same-class shadowing, JUnit 4, message, supplier, generic, nested and array cases in one source fixture. The other tests cover inherited shadowing, qualified subtypes and instances, same-package shadowing, stable output and missing attribution.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch