Skip to content
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

Added a new unit test to check the use of variables in the plugins section in a Gradle file #3900

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lnnery
Copy link

@lnnery lnnery commented Jan 10, 2024

What's changed?

Added a new unit test to check the use of variables in the plugins section in a Gradle file. There was an error when using variables in this section, and that was what led to the creation of the new test.

Have you considered any alternatives or workarounds?

Yes, if the variables are removed from the plugins section everything works, however, this is not a good approach as it requires changing the Gradle file.

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

Added a new unit test using variables in plugins section, because there was an error caused by that.
@timtebeek
Copy link
Contributor

Thanks for adding this test @lnnery ! Helpful indeed to track support for this use case too. In parallel there's also some efforts underway on the use of gradle.properties for dependency in

You might be interested to follow those as well, and the plans there to create a ScanningRecipe to handle gradle.properties files, as well as the associated Slack thread.

@timtebeek timtebeek marked this pull request as draft January 10, 2024 10:17
Added missing import.
@timtebeek
Copy link
Contributor

Sharing the failure here for easier visibility

FindPluginsTest > findPluginWithVariable() STANDARD_OUT
    doesntmatter: 3: [Static type checking] - The variable [jfrogBintrayVersion] is undeclared.

     @ line 3, column 39.

           id 'com.jfrog.bintray' version "${jfrogBintrayVersion}"

                                             ^


    1 error



FindPluginsTest > findPluginWithVariable() FAILED
    java.lang.ClassCastException: class org.openrewrite.groovy.tree.G$GString cannot be cast to class org.openrewrite.java.tree.J$Literal (org.openrewrite.groovy.tree.G$GString and org.openrewrite.java.tree.J$Literal are in unnamed module of loader 'app')
        at org.openrewrite.gradle.search.FindPlugins.lambda$find$0(FindPlugins.java:95)
        at org.openrewrite.gradle.search.FindPlugins$$Lambda$1101/0x00007f65d892abc0.apply(Unknown Source)
        at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:273)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
        at org.openrewrite.gradle.search.FindPlugins.find(FindPlugins.java:99)
        at org.openrewrite.gradle.search.FindPluginsTest.lambda$findPluginWithVariable$4(FindPluginsTest.java:76)
        at org.openrewrite.gradle.search.FindPluginsTest$$Lambda$1090/0x00007f65d891ca10.accept0(Unknown Source)
        at org.openrewrite.internal.ThrowingConsumer.accept(ThrowingConsumer.java:26)
        at org.openrewrite.test.SourceSpec.lambda$beforeRecipe$4(SourceSpec.java:153)
        at org.openrewrite.test.SourceSpec$$Lambda$1091/0x00007f65d891cc68.apply0(Unknown Source)
        at org.openrewrite.test.internal.ThrowingUnaryOperator.apply(ThrowingUnaryOperator.java:28)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:323)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at org.openrewrite.gradle.search.FindPluginsTest.findPluginWithVariable(FindPluginsTest.java:62)

@timtebeek timtebeek added the bug Something isn't working label Jan 11, 2024
@timtebeek
Copy link
Contributor

You might want to keep an eye out on this PR as well, which adds similar support to UpgradePluginVersion:

@timtebeek
Copy link
Contributor

The above PR #3933 has now been merged, and the same approach can likely be replicated here to allow finding plugins which use properties in their version number.

@timtebeek
Copy link
Contributor

Still the same failure here for FindPlugins; there's been a lot of changes recently to recipes affecting Gradle to support properties files, but I suspect FindPlugins was missed so far. We can likely copy the same approach here though to get this going.

@lnnery
Copy link
Author

lnnery commented Feb 13, 2024

Hi @ajohnsonz, adding you here as requested by @timtebeek :)

@timtebeek
Copy link
Contributor

Ah yes, thanks @lnnery ! Figured @ajohnsonz might have some insights to share as to what's needed to make FindPlugins also work with plugin versions defined in a properties file.

@timtebeek
Copy link
Contributor

We might need to be mindful that if FindPlugins is currently used as a precondition, then we can not simply convert it into a scanning recipe, as scanning recipes are not supported as preconditions.

@ajohnsonz
Copy link
Contributor

ajohnsonz commented Feb 14, 2024

Hi @lnnery @timtebeek - you can certainly take a look at UpgradePluginVersion which has now been upgraded to work with plugins whose version is in gradle.properties - I'm not very familiar with "FindPlugin" but I assume that perhaps UpgradePluginVersion is doing both a find and a replace, so could copy some of the code out of there and remove the "replace" part?

It really depends if you care about finding the plugin and also knowing the version, or if you just care about finding the groupId and artifactId - if the latter, a simple if instanceof GString block before the downcast would at least prevent that exception. If the former I'm not really sure how that would be achieved if it can't support ScanningRecipe 🤔

@timtebeek
Copy link
Contributor

Thanks for chiming in! Looking at how FindPlugins is used in AddPluginVisitor it might make most sense indeed to add that instanceof for now.
https://github.com/search?q=org%3Aopenrewrite+FindPlugins&type=code

@timtebeek
Copy link
Contributor

I've pushed up a minimal change to prevent that cast exception; figured easier to iterate if there's something to critique. This now returns the variable name, which I don't think we have to resolve to be able to satisfy FindPlugins responsibility. It seems we only use this from AddPluginVisitor, where we check for the existence of the plugin, not whatever version is used there already. Good enough?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

None yet

3 participants