jdaugherty commented on code in PR #16399:
URL: https://github.com/apache/grails-core/pull/16399#discussion_r4101957057
##########
grails-core/src/main/groovy/org/grails/compiler/injection/GlobalGrailsClassInjectorTransformation.groovy:
##########
@@ -387,6 +391,8 @@ class GlobalGrailsClassInjectorTransformation implements
ASTTransformation, Comp
if (beansProperty == null ||
!classNode.getAnnotations(GRAILS_BEANS_ANNOTATION).isEmpty()) {
Review Comment:
A `@Shared` block is dropped here without a word. Spock renames a shared
field to `$spock_sharedField_beans` and removes the `beans` property, so
`getProperty('beans')` is null by the time this runs:
```groovy
class SharedBeansSpec extends Specification implements GrailsUnitTest {
@Shared
def beans = {
bean('greeting', String) { 'hello' }
}
void "the bean is registered"() {
expect:
applicationContext.containsBean('greeting') // false, and no
BeansConfiguration is generated
}
}
```
With `@GrailsBeans` written out it fails instead, with `@GrailsBeans
requires a 'beans' property initialised to a closure`, which points away from
the cause. `@Shared` is a natural thing to reach for, since the context is
built once per spec class. Could a unit test's shared `beans` field either be
compiled like an instance one, or be reported with a message that names
`@Shared`? A spec for whichever it is would cover it.
##########
grails-testing-support-core/src/test/resources/META-INF/grails-plugin.xml:
##########
@@ -0,0 +1,3 @@
+<plugin name='includedBeans' version='1.0'>
Review Comment:
This file looks unnecessary: the global transform already writes
`META-INF/grails-plugin.xml` for `IncludedBeansGrailsPlugin` into
`build/classes/groovy/test`, since the descriptor is compiled from test
sources. With this resource deleted, all of the module's tests still pass, the
three included-plugin specs among them. Keeping it puts two descriptors for
`includedBeans` on the test classpath (versions `1.0` and `8.0.0-SNAPSHOT`).
Can it be removed?
##########
grails-testing-support-core/src/main/groovy/org/grails/testing/GrailsApplicationBuilder.groovy:
##########
@@ -170,9 +178,18 @@ class GrailsApplicationBuilder {
beanFactory.allowCircularReferences =
environment.getProperty(Settings.SPRING_MAIN_ALLOW_CIRCULAR_REFERENCES,
Boolean, Boolean.TRUE)
def classLoader = this.class.classLoader
+ // The test's own configuration first, as an application's comes
before auto-configuration:
+ // parsed ahead of them, its beans are what an auto-configuration's
@ConditionalOnMissingBean sees.
+ configurationClasses?.each { Class<?> configurationClass ->
Review Comment:
Registering the test's configuration here means it is parsed before anything
`TestRuntimeGrailsApplicationPostProcessor` writes. The harness defaults from
`registerBeans` (`messageSource`, `proxyHandler`, `conversionService`, ...) and
the included plugins' `doWithSpring` and `beanRegistrar` beans all land
afterwards and win a shared name, so a `beans` block cannot replace any of them:
```groovy
class MessageSourceSpec extends Specification implements GrailsUnitTest {
def beans = {
bean('messageSource', MyMessageSource)
}
void "the test's messageSource is used"() {
expect:
applicationContext.getBean('messageSource') instanceof
MyMessageSource // fails: StaticMessageSource
}
}
```
The same override through the deprecated `doWithSpring()` works, because it
runs after `registerBeans` in the same closure. A nested `@Configuration` class
behaves like the `beans` block, and a bean from an included plugin's
`doWithSpring` or `beanRegistrar` (core's `customEditors`, say) replaces the
test's in the same way. Only the test's `beanRegistrar()` gets the last word.
That is the reverse of an application:
`GrailsEarlyPluginRegistrationPostProcessor` drains plugin beans before the
application's configuration is parsed, so an application's `beans` block
replaces a plugin bean, as the upgrade guide says. The unit testing guide leads
with the `beans` block for exactly this case ("To provide or replace beans in
the context..."), so a test migrating off `doWithSpring` silently ends up with
the default instead.
Could the harness register the default and plugin beans ahead of the test's
configuration, the way the early registration phase does for an application? If
that is out of scope here, the guide should say that replacing a framework or
plugin bean needs `beanRegistrar()`. Either way, a spec that replaces
`messageSource` and an included plugin's bean from a `beans` block would pin
whichever behavior is chosen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
