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

[sihutch]

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.

[timtebeek]

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

[sihutch]

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]

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. ;)

[shanman190]

@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:

• https://github.com/openrewrite/rewrite/pull/4354

[sihutch]

@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.

[sihutch]

@shanman190 Here's a first pass at a solution

[sihutch]

@shanman190 @timtebeek

I have removed DependencyMatchPredicate now so only the trait is being used. I have updated all of the tests to work with the toolingApi - I have not been able to get a handful of tests to pass so maybe you can either fix these or give me some pointers.

The failing tests are

1. DependencyUseStringNotationTest#basicMapLiteral
2. ChangeDependencyConfigurationTest#worksForProjectDependencies
3. UpgradeDependencyVersionTest#versionInParentSubprojectDefinitionWithPropertiesFiles
4. UpgradeDependencyVersionTest#noReposProperties
5. UpgradeDependencyVersionTest#noReposProperties#noRepos

For tests 1-3, the trait matcher doesn't appear to cover the cases.

I am not surprised by the failures for tests 4 and 5, as without the repos, resolution doesn't work. i.e this would have worked prior to using the toolingApi. Are these tests even useful? As the "before" build.gradle would not work in regular grade anyway due to resolution failures.

[shanman190]

@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:

1. 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.
2. ChangeDependencyConfigurationTest#worksForProjectDependencies: The GradleDependency.Matcher isn't handling project dependencies because those aren't ResolvedDependency instances.
3. 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]

@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.

[sihutch]

@shanman190

1. 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]

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]

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]

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]

@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.

[sihutch]

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

[shanman190]

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

[sihutch]

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.

[shanman190]

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

[sihutch]

@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.