codeconsole commented on code in PR #16399:
URL: https://github.com/apache/grails-core/pull/16399#discussion_r4102139151


##########
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:
   Fixed for the harness defaults in 34aaea15b6. After it registers 
`messageSource`, `conversionService` and the rest, `registerBeans` now puts 
back whatever the test's configuration declared under one of those names. So a 
`beans` block replaces them, as an application's configuration replaces the 
framework's auto-configured beans, which back off. 
`ReplacingHarnessDefaultsSpec` pins this, and it fails with the restore 
disabled.
   
   I've left included plugins' beans as they are, because that is what boot 
does. An application `@Bean` does not replace a plugin bean of the same name: 
the early phase registers the plugin's definition before the application's 
configuration is parsed, and `ConfigurationClassBeanDefinitionReader` then 
skips the `@Bean` ("a definition for bean ... already exists. This top-level 
bean definition is considered as an override"). I checked this with 
`EarlyPluginRegistrationOrderingSpec`'s setup and an application declaring 
`overrideProbe` as a `@Bean`: the plugin's `EarlyOrderingPluginResolver` wins. 
Only the application's `doWithSpring` or `beanRegistrar` replaces it, as the 
existing override spec there shows.
   
   So in a test, an included plugin's bean still wins over a `beans` block 
bean. `IncludedPluginBeanOverBeansBlockSpec` pins that, 
`BeanRegistrarOverIncludedPluginBeanSpec` pins that the test's 
`beanRegistrar()` replaces one, and the unit testing guide now says to use 
`beanRegistrar()` for that case.
   



##########
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:
   It's reported now, on both paths (34aaea15b6): "A unit test's 'beans' block 
cannot be @Shared - Spock moves a shared field where the beans DSL cannot 
follow it. Remove @Shared: the beans are created once for the test class 
whether or not it is there."
   
   I went with reporting it rather than compiling it. Compiling it would mean 
undoing more of Spock's rewrite than the moved initializer (the rename and the 
accessors it generates), and `@Shared` gains nothing here, since the context is 
built once per spec class. `GrailsBeansASTTransformationSpec` covers the 
explicit `@GrailsBeans` path, and it no longer reports the misleading "requires 
a 'beans' property". `GlobalGrailsClassInjectorTransformationSpec` covers the 
convention.
   



##########
grails-testing-support-core/src/test/resources/META-INF/grails-plugin.xml:
##########
@@ -0,0 +1,3 @@
+<plugin name='includedBeans' version='1.0'>

Review Comment:
   Removed in 34aaea15b6. The included-plugin specs pass on the descriptor the 
global transform writes into the test output.



-- 
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]

Reply via email to