Expand GradleDependency Trait to change dependencies in jvm-test-suite DSL & buildscript #4376

sihutch · 2024-08-02T16:21:57Z

What's changed?

Dependencies configured within a test suite are not updated by the org.openrewrite.gradle.ChangeDependency recipe.

This PR has only a failing test case. I would be happy to try to contribute a fix but I would need a lot of pointers to get me started.

What's your motivation?

We are heavy users of the Test Suites and would like to be able to upgrade their dependencies using OpenRewrite.

rewrite-gradle/src/test/java/org/openrewrite/gradle/ChangeDependencyTest.java

timtebeek · 2024-08-02T16:25:22Z

Great to see this replicated, thanks! Did you already step through using this test as well?

sihutch · 2024-08-02T16:29:34Z

No problem. I haven't stepped through yet. I am about to go on vacation for a week so will hopefully pick it up on my return unless someone else does.

timtebeek · 2024-08-02T16:30:46Z

Ah thanks a lot for logging the issue before logging off; enjoy your time off! We'll see if some of the usual suspects gets to it before you return. ;)

…st-case-for-changing-dependencies-in-jvm-test-suites

… test pass

…JVM Test Suite plugin to make the failing test pass.

…-suites' of github.com:sihutch/rewrite into failing-test-case-for-changing-dependencies-in-jvm-test-suites

shanman190 · 2024-08-08T14:32:24Z

@sihutch and @ryanwalker, I just noticed that both of you are working on the same update to the same recipe. Would one of you like to perform the updates on ChangeDependency while the other does the same for UpgradeDependency? This way you'd both get the experience and it'd help out 2 of the many Gradle recipes needing this particular update.

This is Ryan's PR that he's working on presently:

Fix ChangeDependency to handle unknown configurations #4354

sihutch · 2024-08-10T10:37:42Z

@shanman190 @ryanwalker - I am happy to wait for Ryan to complete his PR on ChangeDependency and then work for UpgradeDependency. Once we have a working solution, it should be straightforward to extract any commonality.

…in-jvm-test-suites

…st-suites

…d in jvm-testsuites

…suite

… in jvm-testsuites

…n jvm-testsuites

…in jvm-testsuites

…ed in the buildScript

shanman190 · 2024-09-06T18:44:06Z

rewrite-gradle/src/main/java/org/openrewrite/gradle/ChangeDependencyArtifactId.java

+
+                GradleDependency.Matcher gradleDependencyMatcher = new GradleDependency.Matcher();
+
+                if (!gradleDependencyMatcher.get(getCursor()).isPresent() || !(StringUtils.isBlank(configuration) || m.getSimpleName().equals(configuration))) {


An additional enhancement here would be to expose configuration as another selector for the matcher. This isn't strictly necessary as part of this PR though, but I wanted to mention it while I was here.

shanman190 · 2024-09-06T18:54:08Z

@sihutch,

For tests 1-3, I'd expect for these to work. I'll try to debug these a little bit locally.
For tests 4 & 5, these feel like they are no longer valid, so I'd be all for removing them.

EDIT:

DependencyUseStringNotationTest#basicMapLiteral: This is failing because the GradleDependency.Matcher doesn't handle the G.MapLiteral type. This should be an easy problem to resolve as it's mostly just delegating to the G.MapEntry path.
ChangeDependencyConfigurationTest#worksForProjectDependencies: The GradleDependency.Matcher isn't handling project dependencies because those aren't ResolvedDependency instances.
UpgradeDependencyVersionTest#versionInParentSubprojectDefinitionWithPropertiesFiles: This test is failing because of the Gradle model. The GradleProject marker contains the configurations for the current project. Since this particular test is placing a dependency across all subprojects -- which as well using allprojects, subprojects, or project(String) { ... } are all considered bad practices now -- results in the GradleDependency.Matcher not being able to find the ResolvedDependency because it's not this project's dependency.

shanman190 · 2024-09-06T19:58:46Z

@timtebeek, do you have any ideas or opinions on how one might address test cases 2 or 3 from my analysis above?

For the project dependency, we could build enough of a ResolvedDependency to make it work or what Gradle does internally is they have another type to represent a project dependency that is separate from the external dependency.

The subprojects block dependencies is a harder problem as we've gotten by so far by abusing the current project, but that's not really sustainable. Gradle's configuration cache/project isolation feature will help as teams adopt it as it explicitly disallows this from happening on modern projects.

…in-jvm-test-suites # Conflicts: # rewrite-gradle/src/test/java/org/openrewrite/gradle/UpgradeDependencyVersionTest.java

sihutch · 2024-09-07T14:43:00Z

@shanman190

DependencyUseStringNotationTest#basicMapLiteral: This is failing because the GradleDependency.Matcher doesn't handle the G.MapLiteral type. This should be an easy problem to resolve as it's mostly just delegating to the G.MapEntry path.

I've pushed a fix for this.

I have also removed tests 4 and 5

I have resolved merge conflicts, but this has resulted in a new failing test case

UpgradeDependencyVersionTest#dependenciesBlockInFreestandingScript

shanman190 · 2024-09-07T14:48:42Z

Yeah, I was just looking at that new test last night. For the standalone scripts, they aren't carrying a GradleProject marker at all at the moment. @sambsnyd was going to see about collecting project markers during the scan phase, but that is still a work in progress.

shanman190 · 2024-09-07T15:08:59Z

I'm wondering if we won't have to make a concession for these 3 failing tests by just retaining the old path as a fallback. This would ultimately only effect the UpgradeDependencyVersion and ChangeDependencyConfiguration recipes.

sihutch · 2024-09-07T16:26:04Z

I would definitely do this. It seems a shame to hold back the otherwise useful functionality.

@shanman190 in my original PR I I reduced a predicate class that fell back to the old method in the case where the trait failed. Do you think it is worth introducing such a mechanism again for all cases?

shanman190 · 2024-09-07T22:37:59Z

@sihutch, rather than the predicate, I think I'd probably update all of the references to first check with the GradleDependency.Matcher, then if that fails to use the MethodMatcher("DependencyHandlerSpec *(..)"). Calling this out specifically, but the UpgradeDependencyVersuon#isLikelyDependencyConfiguration(..) could likely be removed entirely as well. The GradleDependency.Matcher and MethodMatcher should provide stronger guarantees across the board.

As it relates to the predicate, I don't think the increase in API surface area is worth the indirection at the moment given the above.

…n and UpgradeDependencyVersion cases

sihutch · 2024-09-08T08:57:00Z

@shanman190 I have implemented the fallback for the UpgradeDependencyVersion and ChangeDependencyConfiguration recipes/ All tests are passing now.

shanman190 · 2024-09-08T13:26:51Z

Awesome! I'll probably do a small cleanup commit tomorrow, but then we'll get this merged.

github-actions

Some suggestions could not be made:

rewrite-gradle/src/main/java/org/openrewrite/gradle/UpgradeDependencyVersion.java
- lines 375-374

rewrite-gradle/src/test/java/org/openrewrite/gradle/UpgradeDependencyVersionTest.java

…endencyVersionTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…in-jvm-test-suites

github-actions

Some suggestions could not be made:

rewrite-gradle/src/main/java/org/openrewrite/gradle/UpgradeDependencyVersion.java
- lines 375-374

timtebeek

Thanks a lot for the detailed work here! I've adapted the PR title before that becomes the squashed commit message; I hope I covered the essential well there.

Sure nice to see complexity concentrated into that GradleDependency Trait, and reused from each of the individual recipes.

Thanks as well to @shanman190 for the continued guidance on this PR; I'll let you merge since you mentioned you were planning a slight polish as well.

sihutch · 2024-09-09T07:59:53Z

Thanks @timtebeek. It was a good learning experience, and I appreciate the detailed guidance from @shanman190.

I am a platform team member managing change for hundreds of projects. This is my first time using OpenRewrite for migration, and it is proving very useful. Thanks to the team. I hope to make more contributions in the future.

…in-jvm-test-suites

shanman190 · 2024-09-09T22:12:30Z

@sihutch, thanks again for all of the work put in on this!

sihutch · 2024-09-10T05:28:06Z

@shanman190, we are heavy users of test suites so this improvement is great for us. Thanks for your patience and detailed responses. I really appreciate it and have learned a lot through the process.

Create a failing test for dependency change in the jvm-test-suite DSL

a3ee8e2

sihutch mentioned this pull request Aug 2, 2024

Change Dependency recipes do not work with dependencies defined in jvm-test-suites #4373

Closed

timtebeek reviewed Aug 2, 2024

View reviewed changes

rewrite-gradle/src/test/java/org/openrewrite/gradle/ChangeDependencyTest.java Outdated Show resolved Hide resolved

rewrite-gradle/src/test/java/org/openrewrite/gradle/ChangeDependencyTest.java Outdated Show resolved Hide resolved

timtebeek marked this pull request as draft August 2, 2024 16:24

Remove unused imports

46a491e

Simon Hutchinson added 11 commits August 6, 2024 12:05

Merge branch 'main' of github.com:openrewrite/rewrite into failing-te…

2f1f644

…st-case-for-changing-dependencies-in-jvm-test-suites

Add just just enough from the "testing" extension to make the failing…

2a3f6c5

… test pass

Remove unused import

5cd29c4

Remove extends ScriptHandler

068d7b6

Restore accidentally deleted file

12b6cc5

Format with Intellij formatter

a2d2993

Add just enough code for closures added to the Gradle Project by the …

4deaebd

…JVM Test Suite plugin to make the failing test pass.

Merge branch 'failing-test-case-for-changing-dependencies-in-jvm-test…

53c03d7

…-suites' of github.com:sihutch/rewrite into failing-test-case-for-changing-dependencies-in-jvm-test-suites

Fix merge

c7213ec

Restore file

9394808

Fix formatting

a8f21fd

Simon Hutchinson added 9 commits August 27, 2024 17:18

Merge branch 'main' into failing-test-case-for-changing-dependencies-…

e3f0338

…in-jvm-test-suites

Ensure that ChangeDependency works with dependendencies within jvm-te…

be5cfe3

…st-suites

Ensure that ChangeDependencyArtifactId works with dependencies define…

63a64c7

…d in jvm-testsuites

Ensure ChangeDependencyClassifier works with jvm-testsuites

151aacb

Ensure ChangeDependencyClassifier works with dependencies in jvm-test…

8afddbc

…suite

Ensure that ChangeDependencyExtension works with dependencies defined…

b6c525d

… in jvm-testsuites

Ensure that ChangeDependencyGroupId works with dependencies defined i…

1fbcae7

…n jvm-testsuites

Ensure that DependencyUseMapNotation works with dependencies defined …

31e9f3d

…in jvm-testsuites

Ensure that DependencyUseStringNotation works with dependencies defin…

e2514e4

…ed in the buildScript

Further test fixes

4374501

shanman190 reviewed Sep 6, 2024

View reviewed changes

Simon Hutchinson added 3 commits September 7, 2024 13:54

Remove "invalid" no-repo tests.

f86f864

Handle G.MapLiteral expression in GradleDependency.Matcher

299bc69

Merge branch 'main' into failing-test-case-for-changing-dependencies-…

e42210f

…in-jvm-test-suites # Conflicts: # rewrite-gradle/src/test/java/org/openrewrite/gradle/UpgradeDependencyVersionTest.java

Fall back to dependency dsl matching for ChangeDependencyConfiguratio…

4802efd

…n and UpgradeDependencyVersion cases

Minor polish to nullability annotations

8c0c856

github-actions bot reviewed Sep 8, 2024

View reviewed changes

rewrite-gradle/src/test/java/org/openrewrite/gradle/UpgradeDependencyVersionTest.java Outdated Show resolved Hide resolved

timtebeek changed the title ~~Create a failing test for dependency change in the jvm-test-suite DSL~~ Expand GradleDependency Trait to change dependencies in jvm-test-suite DSL & buildscript Sep 8, 2024

timtebeek added the enhancement New feature or request label Sep 8, 2024

timtebeek and others added 2 commits September 9, 2024 00:06

Update rewrite-gradle/src/test/java/org/openrewrite/gradle/UpgradeDep…

6a9748c

…endencyVersionTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into failing-test-case-for-changing-dependencies-…

ec994e9

…in-jvm-test-suites

github-actions bot reviewed Sep 8, 2024

View reviewed changes

timtebeek approved these changes Sep 8, 2024

View reviewed changes

Simon Hutchinson and others added 3 commits September 9, 2024 16:33

Merge branch 'main' into failing-test-case-for-changing-dependencies-…

d16c201

…in-jvm-test-suites

Make sure that all dependency recipes work for freestanding scripts

649c979

Fix broken tests

9d35ed2

shanman190 merged commit 63ce3b3 into openrewrite:main Sep 9, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand GradleDependency Trait to change dependencies in jvm-test-suite DSL & buildscript #4376

Expand GradleDependency Trait to change dependencies in jvm-test-suite DSL & buildscript #4376

sihutch commented Aug 2, 2024

timtebeek commented Aug 2, 2024

sihutch commented Aug 2, 2024

timtebeek commented Aug 2, 2024

shanman190 commented Aug 8, 2024 •

edited by timtebeek

Loading

sihutch commented Aug 10, 2024

shanman190 Sep 6, 2024

shanman190 commented Sep 6, 2024 •

edited

Loading

shanman190 commented Sep 6, 2024

sihutch commented Sep 7, 2024

shanman190 commented Sep 7, 2024

shanman190 commented Sep 7, 2024

sihutch commented Sep 7, 2024 •

edited

Loading

shanman190 commented Sep 7, 2024

sihutch commented Sep 8, 2024

shanman190 commented Sep 8, 2024

github-actions bot left a comment

github-actions bot left a comment

timtebeek left a comment

sihutch commented Sep 9, 2024

shanman190 commented Sep 9, 2024

sihutch commented Sep 10, 2024


		GradleDependency.Matcher gradleDependencyMatcher = new GradleDependency.Matcher();

		if (!gradleDependencyMatcher.get(getCursor()).isPresent() \|\| !(StringUtils.isBlank(configuration) \|\| m.getSimpleName().equals(configuration))) {

Expand GradleDependency Trait to change dependencies in jvm-test-suite DSL & buildscript #4376

Expand GradleDependency Trait to change dependencies in jvm-test-suite DSL & buildscript #4376

Conversation

sihutch commented Aug 2, 2024

What's changed?

What's your motivation?

timtebeek commented Aug 2, 2024

sihutch commented Aug 2, 2024

timtebeek commented Aug 2, 2024

shanman190 commented Aug 8, 2024 • edited by timtebeek Loading

sihutch commented Aug 10, 2024

shanman190 Sep 6, 2024

Choose a reason for hiding this comment

shanman190 commented Sep 6, 2024 • edited Loading

shanman190 commented Sep 6, 2024

sihutch commented Sep 7, 2024

shanman190 commented Sep 7, 2024

shanman190 commented Sep 7, 2024

sihutch commented Sep 7, 2024 • edited Loading

shanman190 commented Sep 7, 2024

sihutch commented Sep 8, 2024

shanman190 commented Sep 8, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

timtebeek left a comment

Choose a reason for hiding this comment

sihutch commented Sep 9, 2024

shanman190 commented Sep 9, 2024

sihutch commented Sep 10, 2024

shanman190 commented Aug 8, 2024 •

edited by timtebeek

Loading

shanman190 commented Sep 6, 2024 •

edited

Loading

sihutch commented Sep 7, 2024 •

edited

Loading