refactor: use the gradle runner to set the plugin classpath #14680
Conversation
This allows simplifying the test build scripts by avoiding to set the classpath in each of them. It also allows (and requires, as a matter of fact) to use the plugins block to apply the boot plugin under test. Unfortunately, this doesn't work for the tests for the reaction to the Kotlin plugin. See the comments in the GradleBuild class and in each KotlingPluginActionIntegrationTests build script.
spring-projects-issues
added
the
status: waiting-for-triage
|
On first impressions, this looks great. Thank you, @jnizet. You've obviously done some digging into the oddity with the tests that react to the Kotlin plugin being applied. Did you get a feel for why that plugin in particular gets loaded using a different class loader? There are other tests that cover reacting to other plugins being applied that don't appear to suffer from the same problem. I wonder what's special about the Kotlin plugin. |
|
Thanks @wilkinsona! I've asked a question on the Gradle Slack plugin (in the plugin-development channel) this morning, but I'm still waiting for an answer. As far as I understand, reacting to the built-in pugins isn't a problem because they're in the "built-in" classpath (not sure about the terminology, but hopefully you get the idea). Reacting to the dependency management plugin is fine because it's bundled with the boot plugin and is thus part of the plugin classpath set on the gradle runner. The Kotlin plugin is the only one that is part of the compile classpath of the boot plugin, but is not bundled with it, and is loaded dynamically by gradle at runtime. If you look at the implementation of the various PluginApplicationAction subclasses, the Kotlin one is the only one where Class.forName() is used to dynamically load the plugin class (and return null if loading the class fails). For all the other ones, a class literal (e.g. I guess it would be possible to make the tests pass by using reflection to mutate the plugin and its tasks, but I don't think this is a good idea. If the Gradle guys provide another workaround or a solution, I'll be happy to amend this PR or open a new one. |
philwebb
added
type: task
Previously, each test build script used a property to configure its classpath. This commit simplifies the tests by setting the classpath once on the GradleRunner, thereby removing the need for it to be set in each test script. It also allows, and, in fact, requires, the use of the plugins block to apply the Boot plugin under test. Unfortunately, this doesn't work for the tests for the reaction to the Kotlin plugin. See the comments in the GradleBuild class and in each KotlingPluginActionIntegrationTests build script. See gh-14680
* gh-14680: Polish "Refactor Gradle plugin tests to use runner's plugin classpath" Refactor Gradle plugin tests to use runner's plugin classpath
|
Thanks for the explanation, @jnizet, and once again for the PR. I've merged the changes into master. I've also tweaked things a bit in 8c6910c to allow the runner's plugin classpath to be used for the Kotlin plugin tests too. It's using the same approach as was already used for the Dependency Management plugin. |
|
Thanks for letting me know. I was reluctant to do that because it makes the tests run in conditions that are different from the actual runtime conditions, where the Kotlin plugin is not bundled with the Boot plugin. |
|
The runtime conditions seem to be different to the test conditions with either approach. Given that, I thought it was better to take the approach that keep things consistent in the tests. |
This allows simplifying the test build scripts by avoiding to set the classpath in each of them.
It also allows (and requires, as a matter of fact) to use the plugins block to apply the boot plugin
under test.
Unfortunately, this doesn't work for the tests for the reaction to the Kotlin plugin.
See the comments in the GradleBuild class and in each KotlingPluginActionIntegrationTests build script.
See #14585 (comment)