grails / grails-gradle-plugin

Apache License 2.0
6 stars 9 forks source link

Add source set dependencies back to `integrationTestImplementation` #244

Closed guillermocalvo closed 10 months ago

guillermocalvo commented 10 months ago

After some investigation, I found out that the problem is not that "plugins can overwrite message bundles" in integration tests, but that version 6 of the plugin mistakenly removed main/test source set dependencies from configuration integrationTestImplementation.

I added back the file dependencies so that it behaves like version 5. This solves #235

Please note that configuration dependencies are the only ones that are deprecated and should be replaced by Configuration#extendsFrom.

puneetbehl commented 10 months ago

This is removed intentionally as there was some deprecation message I saw which is something similar to following:

Adding a Configuration as a dependency is a confusing behavior which isn't recommended. This behaviour has been deprecated and is scheduled to be removed in Gradle 8.0. If you're interested in inheriting the dependencies from the Configuration you are adding, you should use Configuration#extendsFrom instead. 

I think you would be able to reproudce this by generating Gradle Build Scan against the local SNAPSHOT of this.

guillermocalvo commented 10 months ago

@puneetbehl

This is removed intentionally as there was some deprecation message I saw which is something similar to following:

Adding a Configuration as a dependency is a confusing behavior which isn't recommended. This behaviour has been deprecated and is scheduled to be removed in Gradle 8.0. If you're interested in inheriting the dependencies from the Configuration you are adding, you should use Configuration#extendsFrom instead. 

I think you would be able to reproudce this by generating Gradle Build Scan against the local SNAPSHOT of this.

That's what I mean, configuration dependencies are the only ones that are deprecated; file dependencies are safe to keep.

See, you replaced all dependencies in version 5:

DependencyHandler dependencies = project.dependencies
dependencies.add("integrationTestImplementation", SourceSets.findMainSourceSet(project).output)
dependencies.add("integrationTestImplementation", SourceSets.findSourceSet(project, SourceSet.TEST_SOURCE_SET_NAME).output)
dependencies.add("integrationTestImplementation", project.configurations.findByName("testCompileClasspath"))
dependencies.add("integrationTestRuntimeOnly", project.configurations.findByName("testRuntimeClasspath"))

With:

project.configurations.named("integrationTestImplementation") {
    it.extendsFrom(project.configurations.named("testImplementation").get())
}
project.configurations.named("integrationTestRuntimeOnly") {
    it.extendsFrom(project.configurations.named("testRuntimeOnly").get())
}

The two missing SourceSet dependencies are the root cause of #235:

dependencies.add("integrationTestImplementation", SourceSets.findMainSourceSet(project).output)
dependencies.add("integrationTestImplementation", SourceSets.findSourceSet(project, SourceSet.TEST_SOURCE_SET_NAME).output)

These two don't produce any deprecation warnings.

puneetbehl commented 10 months ago

Isn't the following doing the same thing:

https://github.com/grails/grails-gradle-plugin/blob/6.1.x/src/main/groovy/org/grails/gradle/plugin/core/IntegrationTestGradlePlugin.groovy#L53-L54

guillermocalvo commented 10 months ago

Isn't the following doing the same thing:

https://github.com/grails/grails-gradle-plugin/blob/6.1.x/src/main/groovy/org/grails/gradle/plugin/core/IntegrationTestGradlePlugin.groovy#L53-L54

Nope, that doesn't seem to work the way you would expect.