Open ssheikin opened 3 months ago
timtebeek
commented
3 months ago 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?
ssheikin
commented
3 months ago @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 https://github.com/openrewrite/rewrite-testing-frameworks/pull/520#discussion_r1636380378 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 https://github.com/trinodb/trino/commit/3eae0a831e8affcbbfd6948ed1b38034888bf1a7 , one of such cases could be within some big lambdas.
timtebeek
commented
3 months ago 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
commented
3 months ago 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
commented
3 months ago Picnic's TestNG rules converted to recipes seem to hold up well; We'll need some minor fixes still though like
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.
timtebeek
commented
3 months ago 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. 🥲
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
commented
3 months ago There is a complete checklist of all TestNG assertions here: https://github.com/Philzen/rewrite-TestNG-to-JUnit5/pull/3#issue-2347158743 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
commented
1 month ago Pretty soon we'll be able to leverage the Picnic recipes again, following the work done in
That should help along the work done here to get a first version merged.
Stephan202
commented
1 month ago Pretty soon we'll be able to leverage the Picnic recipes again
That time has arrived :tada:.
What's changed?
Add TestNG -> AssertJ rules (applying minimal changes to JUnit -> AssertJ)
What's your motivation?
https://github.com/trinodb/trino/pull/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