Skip to content

Adding dependency guards to the Junit 4 -> 5 migration for when the POM or build.gradle have a dependency on org.testng:testng. #760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Aug 5, 2025

Conversation

steve-aom-elliott
Copy link
Contributor

What's changed?

There was already a code guard for usage of org.testng..*, but not a guard for if there was a dependency on it in pom.xml or build.gradle. This meant it was able to make changes to some of those files regarding exclusions that left it in a half-way state.

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

@steve-aom-elliott steve-aom-elliott self-assigned this Jul 7, 2025
@steve-aom-elliott steve-aom-elliott added bug Something isn't working recipe Recipe request labels Jul 7, 2025
@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Jul 7, 2025
@steve-aom-elliott
Copy link
Contributor Author

@steve-aom-elliott steve-aom-elliott force-pushed the extend-testng-guard-for-junit4-to-junit5 branch from f4645f4 to e5ee3de Compare July 14, 2025 19:15
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite Jul 14, 2025
Copy link
Member

@sambsnyd sambsnyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there are both gradle and maven variations of a recipe we often create one which bundles the two together in rewrite-java-dependencies

@steve-aom-elliott
Copy link
Contributor Author

steve-aom-elliott commented Jul 14, 2025

I'll see if I can round out a PR for that as well and then switch this over to that. It's worth noting that given scope and configuration don't overlap in terms of allowable values, the existing shared DependencyInsight in the rewrite-java-dependencies is a bit of an odd one, given it only takes Maven scopes and passes null for configuration.

Comment on lines 60 to 63
- org.openrewrite.gradle.search.DoesNotIncludeDependency:
groupId: org.testng
artifactId: testng
- org.openrewrite.maven.search.DoesNotIncludeDependency:
groupId: org.testng
artifactId: testng
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we introduce an overarching recipe?
org.openrewrite.java.dependencies.DoesNotIncludeDependency?

@steve-aom-elliott steve-aom-elliott force-pushed the extend-testng-guard-for-junit4-to-junit5 branch from e5ee3de to c869d70 Compare July 16, 2025 15:30
@steve-aom-elliott
Copy link
Contributor Author

Added recipe in rewrite-java-dependencies as org.openrewrite.java.dependencies.search.DoesNotIncludeDependency

@steve-aom-elliott steve-aom-elliott force-pushed the extend-testng-guard-for-junit4-to-junit5 branch from c869d70 to 6db5765 Compare July 24, 2025 16:07
@steve-aom-elliott steve-aom-elliott marked this pull request as ready for review July 24, 2025 16:24
@timtebeek timtebeek self-requested a review July 25, 2025 13:31
Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single test you had here originally included a class with Test from TestNG, as well as both Maven and Gradle build files; that's a bit odd, because we wouldn't even need the Maven/Gradle build files for the test to pass as the presence of the class should already disqualify the DoesNotUseType precondition. I've separated that out into three distinct tests, with two merely using JUnit + testng dependency, which I'd have expected to pass. They're failing though and making changes, which if I understood to intention of this PR correctly should not happen. Is there anything wrong with the test setup or preconditions?

@steve-aom-elliott steve-aom-elliott force-pushed the extend-testng-guard-for-junit4-to-junit5 branch from d7d91b4 to 941d017 Compare July 28, 2025 21:46
@steve-aom-elliott
Copy link
Contributor Author

Working on fixes for higher level tests. I imagine they're situations where I need to wrap the test in a mavenProject, though maybe this is an indicator that I need to make the guard more flexible for situations where for some reason there's no surrounding project for a Java file.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/test/java/org/openrewrite/java/testing/testng/TestNgToAssertJTest.java
    • lines 70-71

@timtebeek

This comment was marked as outdated.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/test/java/org/openrewrite/java/testing/testng/TestNgToAssertJTest.java
    • lines 70-71

- org.openrewrite.java.search.DoesNotUseType:
fullyQualifiedTypeName: org.testng..*
includeImplicit: true
- org.openrewrite.java.testing.junit5.TestNgGuard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The likeliness of this precondition now matching in the wild is quite a bit larger. I'm wondering if we should ask whoever reported this issue to take on this precondition in their recipes, as opposed to including it here.

In practical terms I'd still hope it to be rare that TestNG is present on the classpath in anyway, so we could still adopt this PR as is, but figured point this out before a merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like entry point for them is org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_1 which is fairly high up the chain (about 10 levels above the eventual org.openrewrite.java.testing.junit5.JUnit4to5Migration composite recipe), hmm. The composite recipe that we have the condition on here itself calls 47 other recipes, so it's quite the expansion to alter that uniquely. I know that the composite recipe also leads back up to org.openrewrite.java.spring.boot3.SpringBoot33BestPractices, org.openrewrite.java.testing.assertj.Assertj and org.openrewrite.java.testing.junit5.JUnit5BestPractices.

Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved save for the small caveat pointed out in

if @sambsnyd agrees then we can merge, to have some shared responsibility.


@Override
public TreeVisitor<?, ExecutionContext> getScanner(Set<JavaProject> acc) {
return new ModuleHasDependency("org.testng", "testng*", null, null).getScanner(acc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we could add a flag to ModuleHasDependency to invert its own marking behavior, then we wouldn't need specialized "module doesn't have dependency" recipes like this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to using the new option on ModuleHasDependency instead of a separate class in the latest push.

steve-aom-elliott and others added 15 commits July 31, 2025 17:17
…OM or build.gradle have a dependency on `org.testng:testng`. Relies on openrewrite/rewrite#5725
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
… an enclosing project for a source file, which was a majority of our existing tests.

- If it figures out it's a loose file not in a `JavaProject`, it can't ensure project scope anyway, so falls back to solely whether the file itself has a TestNG dependency or uses TestNG types instead for those
Assuming classes can not be present without those dependencies
@steve-aom-elliott steve-aom-elliott force-pushed the extend-testng-guard-for-junit4-to-junit5 branch from f8fbc37 to b504c95 Compare July 31, 2025 21:25
@steve-aom-elliott
Copy link
Contributor Author

steve-aom-elliott commented Jul 31, 2025

Failing at the moment because it seems openrewrite/rewrite-java-dependencies isn't seeing the new version of Gradle ChangeDependency when it's trying to make a build: https://github.com/openrewrite/rewrite-java-dependencies/actions/runs/16660315340/job/47155660481#step:6:350

@steve-aom-elliott
Copy link
Contributor Author

So the reason it's failing again turns out to actually be because ModuleHasDependency won't mark files at all if they aren't part of modules, which to a degree makes sense based on the recipe itself, but a number of our tests don't wrap their java blocks in a mavenProject, meaning they won't have a JavaProject marker on them when it reaches the recipe. Trying to figure out the best approach here, because while in the real world, you're basically always going to have a module present, our tests are different situation and having to decide to wrap certain tests, especially those many levels up for composite recipes, feels like it's going to be onerous.

@steve-aom-elliott
Copy link
Contributor Author

@steve-aom-elliott
Copy link
Contributor Author

@sambsnyd Looks like this is back to green if you'd be able to give a final look over with it using ModuleHasDependency instead of a separate recipe for the guard.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/test/java/org/openrewrite/java/testing/mockito/MockitoJUnitRunnerToExtensionTest.java
    • lines 21-20
    • lines 38-58

@timtebeek timtebeek merged commit 9a2082a into main Aug 5, 2025
2 checks passed
@timtebeek timtebeek deleted the extend-testng-guard-for-junit4-to-junit5 branch August 5, 2025 13:25
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Aug 5, 2025
mergify bot added a commit to robfrank/linklift that referenced this pull request Aug 15, 2025
…rom 3.11.0 to 3.15.0 [skip ci]

Bumps [org.openrewrite.recipe:rewrite-testing-frameworks](https://github.com/openrewrite/rewrite-testing-frameworks) from 3.11.0 to 3.15.0.
Release notes

*Sourced from [org.openrewrite.recipe:rewrite-testing-frameworks's releases](https://github.com/openrewrite/rewrite-testing-frameworks/releases).*

> 3.15.0
> ------
>
> What's Changed
> --------------
>
> * Include additional AssertJ migrations by [`@​timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-testing-frameworks#773](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/773)
> * Include recipes in larger composites where possible by [`@​timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-testing-frameworks#774](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/774)
> * Retain comments when chaining assertions by [`@​timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-testing-frameworks#775](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/775)
> * refactor: Static imports for Collections and Collectors by [`@​timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-testing-frameworks#779](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/779)
> * Added implementation to extract any expression arguments from last statement in assertThrows by [`@​JohannisK`](https://github.com/JohannisK) in [openrewrite/rewrite-testing-frameworks#724](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/724)
> * Don't clean up mockito imports for Kotlin by [`@​jevanlingen`](https://github.com/jevanlingen) in [openrewrite/rewrite-testing-frameworks#782](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/782)
> * Adding dependency guards to the Junit 4 -> 5 migration for when the POM or build.gradle have a dependency on `org.testng:testng`. by [`@​steve-aom-elliott`](https://github.com/steve-aom-elliott) in [openrewrite/rewrite-testing-frameworks#760](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/760)
> * rename settings.local.json to settings.json by [`@​zieka`](https://github.com/zieka) in [openrewrite/rewrite-testing-frameworks#783](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/783)
> * Fix ClassCastException in PowerMockitoWhenNewToMockito by [`@​timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-testing-frameworks#786](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/786)
> * MockitoJUnitRunnerToExtension: Handle existing MockitoExtension by [`@​amishra-u`](https://github.com/amishra-u) in [openrewrite/rewrite-testing-frameworks#788](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/788)
>
> **Full Changelog**: <openrewrite/rewrite-testing-frameworks@v3.14.0...v3.15.0>
>
> 3.14.1
> ------
>
> What's Changed
> --------------
>
> * OpenRewrite v8.59.1: <https://github.com/openrewrite/rewrite>
>
> **Full Changelog**: <https://github.com/openrewrite/rewrite-testing-frameworks>
>
> 3.14.0
> ------
>
> What's Changed
> --------------
>
> * Collapse more consecutive assertions by [`@​timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-testing-frameworks#769](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/769)
> * refactor: Unwrap else block after return or throw statement by [`@​timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-testing-frameworks#771](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/771)
> * Remove all PowerMock imports after migration by [`@​JohannisK`](https://github.com/JohannisK) in [openrewrite/rewrite-testing-frameworks#770](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/770)
> * Remove `assertTrue(true)` and `assertFalse(false)` by [`@​aet2505`](https://github.com/aet2505) in [openrewrite/rewrite-testing-frameworks#767](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/767)
>
> New Contributors
> ----------------
>
> * [`@​aet2505`](https://github.com/aet2505) made their first contribution in [openrewrite/rewrite-testing-frameworks#767](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/767)
>
> **Full Changelog**: <openrewrite/rewrite-testing-frameworks@v3.13.0...v3.14.0>
>
> 3.13.0
> ------
>
> What's Changed
> --------------
>
> * Removing `NoInitializationForInjectMock` recipe due to side effects when using recipe by [`@​steve-aom-elliott`](https://github.com/steve-aom-elliott) in [openrewrite/rewrite-testing-frameworks#765](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/765)
> * Adding functionality to `MockitoWhenOnStaticToMockStatic` for recognizing an existing try-with-resources `MockedStatic<T>` can be reused instead of creating a new one. by [`@​steve-aom-elliott`](https://github.com/steve-aom-elliott) in [openrewrite/rewrite-testing-frameworks#766](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/766)
> * Inconvertible Mockito `when` situation involving lambda and static method. by [`@​steve-aom-elliott`](https://github.com/steve-aom-elliott) in [openrewrite/rewrite-testing-frameworks#763](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/763)
> * Changing order of `mockito-all` -> `mockito-core` change vs `org.mockito:*` upgrade, and made the former upgrade to `3.x` at the same time, as there aren't aligned versions between the two. by [`@​steve-aom-elliott`](https://github.com/steve-aom-elliott) in [openrewrite/rewrite-testing-frameworks#768](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/768)
>
> New Contributors
> ----------------
>
> * [`@​steve-aom-elliott`](https://github.com/steve-aom-elliott) made their first contribution in [openrewrite/rewrite-testing-frameworks#765](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/765)
>
> **Full Changelog**: <openrewrite/rewrite-testing-frameworks@v3.12.0...v3.13.0>
>
> 3.12.0
> ------
>
> What's Changed
> --------------
>
> * Fix for JUnit test method throws clause preservation by [`@​knutwannheden`](https://github.com/knutwannheden) in [openrewrite/rewrite-testing-frameworks#755](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/755)
> * refactor: org.openrewrite.mavencentral by [`@​Laurens-W`](https://github.com/Laurens-W) in [openrewrite/rewrite-testing-frameworks#756](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/pull/756)

... (truncated)


Commits

* [`76c6888`](openrewrite/rewrite-testing-frameworks@76c6888) MockitoJUnitRunnerToExtension: Handle existing MockitoExtension ([#788](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/issues/788))
* [`75d2019`](openrewrite/rewrite-testing-frameworks@75d2019) Retain `Throwable` in `SimplifyTestThrows`
* [`0ea47e9`](openrewrite/rewrite-testing-frameworks@0ea47e9) Include `SimplifyTestThrows` with `JUnit5BestPractices`
* [`efb0a4e`](openrewrite/rewrite-testing-frameworks@efb0a4e) Add AssertJ change type recipes for classes deprecated in v3.27.4
* [`f203a20`](openrewrite/rewrite-testing-frameworks@f203a20) Update documentation examples
* [`69d7716`](openrewrite/rewrite-testing-frameworks@69d7716) Fix ClassCastException in PowerMockitoWhenNewToMockito ([#786](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/issues/786))
* [`dc6f272`](openrewrite/rewrite-testing-frameworks@dc6f272) rename settings.local.json to settings.json ([#783](https://redirect.github.com/openrewrite/rewrite-testing-frameworks/issues/783))
* [`96fca52`](openrewrite/rewrite-testing-frameworks@96fca52) Documentation example for MockitoJUnitRunnerToExtensionTest
* [`9a2082a`](openrewrite/rewrite-testing-frameworks@9a2082a) Adding dependency guards to the Junit 4 -> 5 migration for when the POM or bu...
* [`74366cf`](openrewrite/rewrite-testing-frameworks@74366cf) OpenRewrite recipe best practices
* Additional commits viewable in [compare view](openrewrite/rewrite-testing-frameworks@v3.11.0...v3.15.0)
  
[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility\_score?dependency-name=org.openrewrite.recipe:rewrite-testing-frameworks&package-manager=maven&previous-version=3.11.0&new-version=3.15.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
Dependabot commands and options
  
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot show  ignore conditions` will show all of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working recipe Recipe request test provided
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants