-
Notifications
You must be signed in to change notification settings - Fork 89
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
Conversation
|
f4645f4
to
e5ee3de
Compare
There was a problem hiding this 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
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 |
- org.openrewrite.gradle.search.DoesNotIncludeDependency: | ||
groupId: org.testng | ||
artifactId: testng | ||
- org.openrewrite.maven.search.DoesNotIncludeDependency: | ||
groupId: org.testng | ||
artifactId: testng |
There was a problem hiding this comment.
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
?
e5ee3de
to
c869d70
Compare
Added recipe in |
c869d70
to
6db5765
Compare
There was a problem hiding this 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?
d7d91b4
to
941d017
Compare
src/main/java/org/openrewrite/java/testing/junit5/TestNgGuard.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/TestNgGuard.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/TestNgGuard.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit5/TestNgGuardTest.java
Outdated
Show resolved
Hide resolved
Working on fixes for higher level tests. I imagine they're situations where I need to wrap the test in a |
There was a problem hiding this 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
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
…OM or build.gradle have a dependency on `org.testng:testng`. Relies on openrewrite/rewrite#5725
…in `rewrite-java-dependencies`
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
…ency` for the TestNG guard for JUnit
f8fbc37
to
b504c95
Compare
|
So the reason it's failing again turns out to actually be because |
|
@sambsnyd Looks like this is back to green if you'd be able to give a final look over with it using |
There was a problem hiding this 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
…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) [](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)
DoesNotIncludeDependency
for Gradle (Maven had one already), to allow for checking whether a dependency is not included. rewrite#5725What'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