JUnit5's assertInstanceOf is not converted to AssertJ equivalent

timo-abele commented 2 months ago

What version of OpenRewrite are you using?

I am using the latest version as of 2024-09-21

How are you running OpenRewrite?

originally gradle build file, also verified via PR:

https://github.com/openrewrite/rewrite-testing-frameworks/pull/606

What is the smallest, simplest way to reproduce the problem?

full example: https://github.com/openrewrite/rewrite-testing-frameworks/pull/606/files#diff-bfebe6e8e82e88df037959cc55562fae9a10b6b050dd49036be913fd07f93295R45-R70

assertInstanceOf(Integer.class, 4);

What did you expect to see?

assertThat(4).isInstanceOf(Integer.class);

What did you see instead?

no change

What is the full stack trace of any errors you encountered?

https://github.com/openrewrite/rewrite-testing-frameworks/actions/runs/10971347200/job/30466378864?pr=606#step:5:1500

AssertJBestPracticesTest > convertsIsInstanceOf() FAILED
    java.lang.AssertionError: Recipe was expected to make a change but made no changes.
        at org.openrewrite.test.LargeSourceSetCheckingExpectedCycles.afterCycle(LargeSourceSetCheckingExpectedCycles.java:119)
        at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:98)
        at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
        at org.openrewrite.Recipe.run(Recipe.java:376)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:371)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:130)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:125)
        at org.openrewrite.java.testing.assertj.AssertJBestPracticesTest.convertsIsInstanceOf(AssertJBestPracticesTest.java:48)

Are you interested in contributing a fix to OpenRewrite?

No

Paulitos commented 2 months ago

Any hint @timtebeek? I saw the pull request from @timo-abele but seems like it didn't work.

timtebeek commented 2 months ago

hi @Paulitos ! Thanks for the offer to help; there's (at least) one of two ways we could fix this issue.

Refaster style recipe

The best would probably be to extend rewrite-templating to support this particular style of Refaster rule: https://github.com/PicnicSupermarket/error-prone-support/blob/6a13efded8f5c64e3b0f09d89e36ece02de36e6d/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java#L732-L743

  static final class AssertThatIsInstanceOf<T> {
    @BeforeTemplate
    void before(Object actual, Class<T> clazz) {
      assertInstanceOf(clazz, actual);
    }

    @AfterTemplate
    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
    void after(Object actual, Class<T> clazz) {
      assertThat(actual).isInstanceOf(clazz);
    }
  }

We already include the refaster rules generated there in our migration from JUnit to AssertJ: https://github.com/openrewrite/rewrite-testing-frameworks/blob/5c23f0798a6e417658443855b1f356666b333a8d/src/main/resources/META-INF/rewrite/assertj.yml#L472

This particular recipe is not converted because of a limitation to handling type parameters in rewrite-templating https://github.com/openrewrite/rewrite-templating/blob/44e803358b1ee3c08562a8a497aa1f1fd9cb90e8/src/main/java/org/openrewrite/java/template/processor/RefasterTemplateProcessor.java#L729-L732

I haven't yet explored what would be needed for a fix there; I suppose it's more involved and @knutwannheden might have some hints.

Imperative recipe support

If expanding Refaster style recipe support is tough, then we could add an imperative recipe to do the same replacement, similar to the examples seen here: https://github.com/openrewrite/rewrite-testing-frameworks/tree/v2.18.0/src/main/java/org/openrewrite/java/testing/assertj While certainly manageable with the existing examples, that would also mean we need to support that recipe going forward, whereas the Refaster style recipe is easier to maintain and support.

openrewrite / rewrite-testing-frameworks