borinquenkid commented on PR #15934:
URL: https://github.com/apache/grails-core/pull/15934#issuecomment-4927875118
**Note:** This comment was generated by an AI coding assistant (Claude),
directed by a human reviewer to verify that this PR's tests actually cover the
situations described in its own description and commit messages. Findings below
are grounded in specific file/line citations from the diff and the `8.0.x` base
branch, not inference.
Overall, the core architectural claims are well covered:
`EarlyPluginRegistrationOrderingSpec` and app3's
`PluginBeansBeforeAutoConfigurationSpec` genuinely prove plugin
`doWithSpring`/`beanRegistrar()` beans beat `@ConditionalOnMissingBean` Boot
defaults, at both unit and true end-to-end level (confirmed `app3` actually
depends on the `loadafter` plugin). The two headline regressions — "OSIV
silently disabled" and "sibling-package controllers losing beans" — are also
genuinely pinned: `OpenSessionInViewSpec` in both `hibernate5/app1` and
`hibernate7/app1`, and the pre-existing `UriMatchingInterceptorFunctionalSpec`,
which really does exercise a controller (`demo.InterceptorDemoController`)
living outside the `Application` class's own package (`functionaltests`).
A few gaps, in rough priority order:
1. **Mongo OSIV fix is untested.** `MongoDbDataStoreSpringInitializer` gets
the identical `isWebApplicationRegistry()` swap that fixed the Hibernate OSIV
regression (`MongoDbDataStoreSpringInitializer.groovy:129-130`), but no test
anywhere asserts `mongoOpenSessionInViewInterceptor` registers in a web app —
the same bug class just proven to silently break Hibernate.
2. **`/actuator/health` half of the actuator regression is untested.**
Commit `dd5ac787` states both `/actuator/env` and `/actuator/health` failed
with `HttpMessageNotWritableException`, but `ActuatorEnvClosureSpec` only
requests `/actuator/env` (lines 40, 48).
3. **`BeanDefinitionOverrideException` edge case (new upgrade-notes section)
has no test.** The closest existing spec, `GrailsAppContextOverridingSpec`,
only checks that `allow-bean-definition-overriding=false` propagates to the
bean factory flag — it never registers a colliding plugin+app bean pair or
asserts the exception.
4. **"Built/instantiated exactly once" claim is checked by existence, not
count.** `EarlyPluginRegistrationOrderingSpec` asserts the
`grailsApplication`/`pluginManager` singletons exist (or don't, in the control
case), but nothing spies on construction to prove single instantiation.
5. **Flag-reset-on-exception path is unexercised.**
`GrailsEarlyPluginRegistrationPostProcessor`'s `catch (RuntimeException | Error
e) { Environment.setInitializing(false); }` branch has no test that makes the
early phase throw and then checks the flag resets — only the successful-refresh
reset is asserted.
Flagging 1-2 as the highest priority since they mirror a regression class
this PR already proved happened once.
--
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]
