Add TestNG -> AssertJ rules

[ssheikin]

What's changed?

Add TestNG -> AssertJ rules
(applying minimal changes to JUnit -> AssertJ)

What's your motivation?

trinodb/trino#22305

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

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

I didn't find any better alternative.

Any additional context

This solution was good enough, maybe it will be helpful for someone.
If someone willing to polish it to production-ready state - please be welcomed to take over this PR.

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 used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Awesome work here @ssheikin ! Only saw your PR just now as drafts don't immediately pop up as a notification for me; is there anything you're looking to add before this is ready to review?

[MBoegers]

I could be missing something! But couldn't this be a Refaster Teamplate Recipe?
I think this could work quite generically:

public class MigrateToAssertEquals {
    @BeforeTemplate
    void before(Object actual, Object expected) {
        Assert.assertEquals(actual, expected);
    }
    @AfterTemplate
    void after(Object actual, Object expected) {
        Assertions.assertThat(actual).isEqualsTo(expected);
    }
}

[Philzen]

couldn't this be a Refaster Teamplate Recipe?

@MBoegers guess that would work fine. On the other hand @ssheikin's explicit implementation is a bit more performant, as it returns the method directly, while the code generated by refaster will continue visiting any tree node below it. One could ask though why refaster does that in the first place 🤔

[MBoegers]

Indeed, from a performance point recipe can benefit from an imperative style. But from a maintainability and understandability point refaster template recipes win, at least for me.
Let's shift the discussion about Refaster Template Recipe speed-ups to the Slack ;)

[Philzen]

@MBoegers have created openrewrite/rewrite-templating#95 as a followup.

[ssheikin]

@timtebeek This solution is a straight-forward copy-paste, which is kind of a 'works for me'.
It lacks of testing for all of the possibilities which TestNG could have. So it's not guaranteed to work in each and every case.
As it's based on JUnit recipes, maybe both of these solutions should reuse common abstract parent, for maintainability.
If #520 (comment) works, so it's much more preferred as requires less custom error-prone code.
Also, it, does not work for some cases. Actually in the same cases when it does not work for JUnit.
Analysing trinodb/trino@3eae0a8 , one of such cases could be within some big lambdas.

[timtebeek]

Thanks for the response and great start @ssheikin ; reading between the lines I get the sense that this is donation of recipes that you'd be glad for us to take over, but not immediately planning any future work on this yourself; is that correct? If so that'd be perfectly fine(!); just wondering what kind of collaboration to expect on this going forward. Would you be ok for us to rework this into something we can merge more easily?

Indeed my thoughts would be to do what we can with Refaster recipes, as some of those are already available, and they greatly simplify the recipes that we'd have to maintain. Adding some tests to this PR (which I'm open to doing) should help test out that theory.

The case you listed about assertions in Lambdas is interesting; We'd need to add some unit tests for those specifically, as I suspect there might be an issue with how tree elements are visited that incorrectly skips lambda blocks within other assertion method arguments.

Enough fun areas to explore I guess, and thanks again for kicking this off with a first PR! :)

[ssheikin]

Thank you for kind words. I'm very happy that during the research to migrate large codebase I've stumbled upon openrewrite and used it successfully. I'd be glad to know more about internal kitchen, but I've limited free cycles now to move this PR over the line. Absolutely! Please take over this effort.

that incorrectly skips lambda blocks

It's not exactly like that. Sometimes lambda blocks are visited and even some occurrences inside are migrated, but not necessarily all. Unfortunately, I have not noticed a pattern, and not sure if such cases are present in trino codebase, to share with you.

[timtebeek]

Picnic's TestNG rules converted to recipes seem to hold up well; We'll need some minor fixes still though like

• Allow JavaTemplate to throw new Exception rewrite#4260
• Troubleshoot mismatched AssertJ AbstractStringAssert.isEqualTo("") rewrite-templating#91
• Add Picnic AssertJ rules to AssertJ best practices #527
• openrewrite/rewrite-third-party@474b346

Once those are resolved we can reevaluate which conversions still need an explicit recipe, and which we can leave to the Refaster recipes we already have.

Overall great start; happy to have a first thing going, and look forward where this will go when we combine it with more recipes to add/change dependencies, and convert the running infra surrounding TestNG.

[ssheikin]

Looks like when args.get(2) is 0 then isFloatingPointType does not properly resolve type
assertEquals(Float.MAX_VALUE, config.getFloatOption(), 0);
->
assertThat(Float.MAX_VALUE).as(0).isEqualTo(config.getFloatOption());

https://github.com/airlift/airlift/pull/1194/files#diff-f4ec6d967f355397849228565bddf5982dbcfc629ec05b6cf8f4e5cfb68ba700R190

[ssheikin]

for
assertEquals(digest.valueAt(0), value, 0.001f);
it generates
assertThat(digest.valueAt(0)).isCloseTo(value, within(0.001f));
which does not compile, because previously float was implicitly converted to double
It has to be
assertThat(digest.valueAt(0)).isCloseTo(value, within(0.001));
airlift/airlift@7f935b3 (#1194)

[ssheikin]

Should it rewrite to hasSameElementsAs for Set?

    @Test
    public void test()
    {
        Set<Integer> actual = com.google.common.collect.ImmutableSortedSet.of(2, 1);
        Set<Integer> expected =  com.google.common.collect.ImmutableSet.of(2, 1);
        org.testng.Assert.assertEquals(actual, expected);
        org.assertj.core.api.Assertions.assertThat(actual).hasSameElementsAs(expected);
        org.assertj.core.api.Assertions.assertThat(actual).isEqualTo(expected); // fails with
        // java.lang.AssertionError:
        // expected: [2, 1]
        //  but was: [1, 2]
        // when comparing elements using Ordering.natural()
    }

[timtebeek]

Unfortunately it seems that for now we can't leverage the Picnic recipes, as those introduce a Java 17 dependency which we don't want to enforce upon our users just yet. 🥲

• rewrite-third-party requires Java 17 rewrite-third-party#11

That means we can likely proceed with the recipe produced here, or reimplement some of those Refaster recipes here, but compile them with Java 8 instead.

[Philzen]

There is a complete checklist of all TestNG assertions here: Philzen/rewrite-TestNG-to-JUnit5#3 (comment) which may be re-used to track feature-completeness of this recipe.

Have just implemented migrations to JUnit5 assertions for nearly all of them (with some rather hideously complex stream-based replacements for those that had no direct equivalents, where i often thought to myself „this could be so much prettier using assertJ") – therefore i'd be up for contributing to this recipe soon.

[timtebeek]

Pretty soon we'll be able to leverage the Picnic recipes again, following the work done in

• Use error-prone-contrib AssertJ recipes #559
• Package OpenRewrite recipes in separate JAR, targeting Java 8 PicnicSupermarket/error-prone-support#1270
• JavaTemplateSemanticallyEqual fails to match AssertJ's assertThat("foo").isEqualTo("") rewrite#4423

That should help along the work done here to get a first version merged.

[Stephan202]

Pretty soon we'll be able to leverage the Picnic recipes again

That time has arrived 🎉.