Add dependencies in JVMTestSuite dependency block

[MBoegers]

What's changed?

If a dependency is added in a dependencies block inside a not standard sourceSet, the prefix mechanism in org.openrewrite.gradle.AddDependency Line 180 produces unexpected namings.

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

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

[shanman190]

Note that this is actually a combination of two distinct features. You've got a custom source set (which the configurations are created at the project level also), but the JVM Test Suites feature as well.

AddDependency hasn't yet been adapted to understand how to add dependencies to JVM Test Suite instances yet, so it's going to attempt to add the dependency like below which should still work, but doesn't match the test assertions.

dependencies {
    integrationTestImplementation "com.google.guava:guava:29.0-jre"
}

Both forms are still valid.

[shanman190]

I'd probably rename this test to something like addToJVMTestSuite which more clearly aligns to the assertion.

[MBoegers]

Thanks for clearing whats used here, that will help implementing. Its true AddDependency doesn't work as the expextation.
Is the nested dependency block a JVMTestSuite specific feature or possible in other combinations?

[shanman190]

There are four main forms of declaring dependencies which are nested at a deeper level than normal that I can recall.

• buildscript: to declare plugins; plugins blocks are preferred in modern versions
• allprojects: discouraged in modern versions and breaks configuration cache/project isolation
• subprojects: also discouraged in modern versions and breaks configuration cache/project isolation
• JVM Test Suites (as shown here)

Of course programmatically adding dependencies via the configuration is possible as well, but I'm leaving that off since we're really talking about DSL forms.

Since we handle plugins separately, so far we've ignored buildscript as well. Which leaves the last three above remaining, two of which are discouraged (and also complicated to deal with with respect to OpenRewrite's LST marker shape).

With JVM Test Suites, it's really wrapping up the historic logic that was necessary to create a source set for the tests to reside in -- which then inherently created the necessary configurations -- and tries to go a step further to encapsulate the test suite itself into a more use case specific and succinct form. But to answer your question, the declaration you have there in the test assertions is a JVM Test Suite specific feature.

[github-actions]

Some suggestions could not be made:

• rewrite-hcl/src/main/java/org/openrewrite/hcl/format/TabsAndIndentsVisitor.java
  • lines 255-256
• rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/HclPrinter.java
  • lines 363-364
• rewrite-java/src/main/java/org/openrewrite/java/internal/template/AnnotationTemplateGenerator.java
  • lines 158-159
• rewrite-java/src/main/java/org/openrewrite/java/internal/template/BlockStatementTemplateGenerator.java
  • lines 281-282
  • lines 669-669
• rewrite-java/src/main/java/org/openrewrite/java/search/FindEmptyClasses.java
  • lines 57-57
• rewrite-xml/src/main/java/org/openrewrite/xml/internal/XmlPrinter.java
  • lines 106-106
• rewrite-yaml/src/main/java/org/openrewrite/yaml/cleanup/RemoveUnusedVisitor.java
  • lines 76-76

[shanman190]

Lots of good stuff here, @MBoegers! I've left a few comments that we can discuss about possibly implementing, but some are certainly more complex than others and we can of course handle those via future PRs should we want to take them on.

[MBoegers]

@shanman190 thanks for educating me here 🙏

[github-actions]

Some suggestions could not be made:

• rewrite-hcl/src/main/java/org/openrewrite/hcl/format/TabsAndIndentsVisitor.java
  • lines 255-256
• rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/HclPrinter.java
  • lines 363-364
• rewrite-java/src/main/java/org/openrewrite/java/internal/template/AnnotationTemplateGenerator.java
  • lines 158-159
• rewrite-java/src/main/java/org/openrewrite/java/internal/template/BlockStatementTemplateGenerator.java
  • lines 281-282
  • lines 669-669
• rewrite-java/src/main/java/org/openrewrite/java/search/FindEmptyClasses.java
  • lines 57-57
• rewrite-xml/src/main/java/org/openrewrite/xml/internal/XmlPrinter.java
  • lines 106-106
• rewrite-yaml/src/main/java/org/openrewrite/yaml/cleanup/RemoveUnusedVisitor.java
  • lines 76-76

[github-actions]

Some suggestions could not be made:

• rewrite-hcl/src/main/java/org/openrewrite/hcl/format/TabsAndIndentsVisitor.java
  • lines 255-256
• rewrite-hcl/src/main/java/org/openrewrite/hcl/internal/HclPrinter.java
  • lines 363-364
• rewrite-java/src/main/java/org/openrewrite/java/internal/template/AnnotationTemplateGenerator.java
  • lines 158-159
• rewrite-java/src/main/java/org/openrewrite/java/internal/template/BlockStatementTemplateGenerator.java
  • lines 281-282
  • lines 669-669
• rewrite-java/src/main/java/org/openrewrite/java/search/FindEmptyClasses.java
  • lines 57-57
• rewrite-xml/src/main/java/org/openrewrite/xml/internal/XmlPrinter.java
  • lines 106-106
• rewrite-yaml/src/main/java/org/openrewrite/yaml/cleanup/RemoveUnusedVisitor.java
  • lines 76-76

[timtebeek]

Can't say I've read all the earlier review threads; do let me know if I missed anything!

I like what you did with the separate Find recipe here. Did you intentionally leave out adding the data table row earlier?

[shanman190]

Just one comment, but great work @MBoegers!

[MBoegers]

@timtebeek I can recall seeing an NPE, I think because using the findmethod (static context) does not initialize the data table properly, right?

[timtebeek]

@timtebeek I can recall seeing an NPE, I think because using the findmethod (static context) does not initialize the data table properly, right?

Ah yes that was solved in f4ecc97