You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "borinquenkid (via GitHub)" <gi...@apache.org> on 2026/07/21 21:57:21 UTC
[PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
borinquenkid opened a new pull request, #16033:
URL: https://github.com/apache/grails-core/pull/16033
## Summary
~20 test methods across `JsonViewHelperSpec`, `JsonViewTestSpec`, `JsonApiSpec`, and
`ExpandSpec` (module `:grails-views-gson:test`) show the same failure+flakiness
pattern (~2% failures, ~1% flakiness per apache/grails-core#16030) — one shared root
cause, not 20 independent bugs.
## Root cause
Two confirmed leak vectors (an initial hypothesis about shared Groovy trait statics
was empirically disproven via isolated Groovy 5.0.7/Spock 2.4 probes — traits do *not*
share static storage across implementing classes in this version):
1. `ExpandSpec` silently resolved `Team`/`Player` to `JsonViewHelperSpec`'s compiled
classes via an unqualified same-package reference (no import), so both specs
registered the identical `Class` objects into independently-built
`KeyValueMappingContext` instances — letting class-keyed GORM caches leak between
unrelated specs.
2. `org.grails.validation.ConstraintEvalUtils` is a genuine JVM-wide static cache keyed
by `System.identityHashCode(config)`. `JsonApiSpec` alone worked around this for its
own fixtures via a reflection hack into `Validateable`'s internal state; the other
three specs had no equivalent reset, so whichever spec ran last in a shared test
fork left stale cached constraint/association state for the next one — explaining
the toMany/toOne/hasOne/expand assertion flakiness.
## Fix
- Hoisted cleanup into the shared `JsonViewTest` trait (implemented by ~19 specs) so
new specs get correct isolation by default: `cleanup()` clears the
`ConstraintEvalUtils` cache after every feature; a separate `cleanupSpec()` tears
down any cached `GrailsApplication` once per spec class (split this way because
per-feature teardown broke `MapRenderSpec`'s once-per-spec datastore bean
registration — found via isolated probes before touching real test files).
- Gave `ExpandSpec` its own `ExpandTeam`/`ExpandPlayer` domain classes instead of
implicitly borrowing another spec's.
- Moved `JsonApiSpec` off the reflection hack onto the public
`SuperHero.clearConstraintsMapCache()` API (available since 7.1); removed its now-
redundant local `cleanup()`.
No production code changed.
## Testing
- Full `:grails-views-gson:test` (178 tests): 0 failures, run to completion multiple
times including with varied `--tests` orderings across the originally-flagged specs
specifically to catch order-dependent regressions.
- CodeNarc/Checkstyle: clean.
Related: apache/grails-core#16030
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#discussion_r3652225989
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy:
##########
@@ -24,39 +24,23 @@ import grails.plugin.json.view.test.JsonRenderResult
import grails.plugin.json.view.test.JsonViewTest
import grails.validation.Validateable
import org.grails.testing.GrailsUnitTest
-import org.grails.validation.ConstraintEvalUtils
import spock.lang.Shared
import spock.lang.Specification
class JsonApiSpec extends Specification implements JsonViewTest, GrailsUnitTest {
- // Cache the static field helper interface for performance
- private static final Class<?> STATIC_FIELD_HELPER = Class.forName('grails.validation.Validateable$Trait$StaticFieldHelper')
-
@Shared
JsonMapper objectMapper = JsonMapper.builder().build()
void setup() {
- ConstraintEvalUtils.clearDefaultConstraints()
- clearConstraintsMapCache(SuperHero)
+ // Resets SuperHero's own cached constraints map (see Validateable#clearConstraintsMapCache).
+ // JsonViewTest#cleanup() already resets the shared ConstraintEvalUtils cache after every
+ // feature, but SuperHero's cache is specific to this spec's own Validateable command
+ // object, so it is reset here too.
+ SuperHero.clearConstraintsMapCache()
Review Comment:
Good change — dropping the `Validateable$Trait$StaticFieldHelper` reflection in favour of the public `clearConstraintsMapCache()` is exactly right, and it matches how `grails-validation`'s own specs do it (`ValidateableTraitSpec:46`, `ValidateableTraitAdHocSpec:37`). It also brings this spec in line with the project rule about testing through public APIs.
One consequence worth being explicit about: with the local `cleanup()` gone, `SuperHero`'s cache is now only reset on the way *in*, so the last feature of this spec leaves it populated for the rest of the fork. That's harmless today — `SuperHero` is declared and used only in this file — but it's the opposite of the isolation-by-default goal, and note that you can't fix it by re-adding a `cleanup()` here as long as the trait declares one. Another argument for keeping the fixture methods out of the published trait.
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy:
##########
@@ -104,6 +106,43 @@ trait JsonViewTest {
return templateEngine
}()
+ /**
+ * Resets the {@link ConstraintEvalUtils} default-constraints cache after every feature
+ * method so state from one test cannot leak into the next test that happens to run in the
+ * same JVM/fork.
+ *
+ * <p>{@link ConstraintEvalUtils} memoizes the default GORM constraints map in a single
+ * JVM-wide static field keyed by the identity of the last {@code Config} seen. Since each
+ * spec implementing this trait typically builds its own {@code GrailsApplication}/{@code
+ * Config}, a stale entry left behind by one spec can otherwise be picked up by another.
+ * This is cheap to recompute, so it is safe to clear after every feature.</p>
+ */
+ void cleanup() {
+ ConstraintEvalUtils.clearDefaultConstraints()
+ }
+
+ /**
+ * Tears down any {@code GrailsApplication} cached by {@code org.grails.testing.GrailsUnitTest}
+ * once the whole spec has finished, so a later spec running in the same JVM/fork always starts
+ * from a clean application context.
+ *
+ * <p>This is deliberately a {@code cleanupSpec()} (once per spec class), not a per-feature
+ * {@code cleanup()}: {@code GrailsUnitTest} intentionally builds its {@code GrailsApplication}
+ * lazily and reuses it across every feature of a spec (it is expensive to build and some
+ * traits, e.g. {@code DataTest}, register beans into it once per spec). Tearing it down after
+ * every feature would rebuild it before the next feature could see those spec-scoped beans.</p>
+ *
+ * <p>{@code GrailsUnitTest} lives in {@code grails-testing-support-core}, a test-only
+ * dependency this trait cannot reference directly since it ships as part of the main
+ * {@code grails-views-gson} artifact, so the call is made dynamically only when present.</p>
+ */
+ @CompileDynamic
+ void cleanupSpec() {
Review Comment:
This method is a no-op and can be dropped, along with the `@CompileDynamic` annotation and the `groovy.transform.CompileDynamic` import.
`cleanupGrailsApplication()` is already invoked after every spec class that implements `GrailsUnitTest`, via the global Spock extension in `grails-testing-support-core`:
```groovy
// org/grails/testing/spock/TestingSupportExtension.groovy:51 (registered in
// META-INF/services/org.spockframework.runtime.extension.IGlobalExtension)
if (GrailsUnitTest.isAssignableFrom(spec.reflection)) {
spec.addCleanupSpecInterceptor(cleanupContextInterceptor)
}
```
`CleanupContextInterceptor` calls `cleanupGrailsApplication()` in a `finally` block, and Spock attaches cleanup-spec interceptors to a synthetic `CLEANUP_SPEC` `MethodInfo` that runs for every spec whether or not one is declared (`PlatformSpecRunner.createMethodForDoRunCleanupSpec`, spock-core 2.4). So for a `GrailsUnitTest` spec this trait method runs inside `invocation.proceed()`, nulls `_grailsApplication`, and the interceptor's own call then finds null and no-ops — net behaviour identical to 8.0.x. For a spec that doesn't implement `GrailsUnitTest`, `respondsTo` is false and nothing happens at all.
That matters beyond tidiness: this is the half of the change that adds a second breaking fixture method to the published trait, for no behavioural gain.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy:
##########
@@ -32,23 +33,23 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
JsonMapper objectMapper = JsonMapper.builder().build()
void setup() {
- mappingContext.addPersistentEntities(Team, Player)
+ mappingContext.addPersistentEntities(ExpandTeam, ExpandPlayer)
Review Comment:
This is the part of the PR I'd keep — an unqualified same-package reference silently binding to another spec's `@Entity` classes is a real trap, and giving `ExpandSpec` its own fixtures is the right shape of fix.
But it's applied to one of four specs doing exactly this, and not to the two with the worst numbers in #16030. `Team` and `Player` are declared once, in `JsonViewHelperSpec.groovy:663` and `:672`, and after this PR they are still registered into three other independently-built `KeyValueMappingContext` instances:
- `IncludeAssociationsSpec` — `import grails.plugin.json.view.*` plus `addPersistentEntities(Player, Team)`
- `HalEmbeddedSpec` — same-package unqualified reference, `addPersistentEntities(Team, Player)`
- `IterableRenderSpec` — same-package unqualified reference, `addPersistentEntities(Player, Team)`, in five separate features
The same pattern holds for two other classes:
- `grails.plugin.json.view.api.Author` is declared in `api/JsonApiSpec.groovy:449` and also registered by `api/JsonApiHandleAssociationsSpec` (`addPersistentEntities(Author, PublishedBook, Publisher)`)
- `Person` is declared in `EmbeddedAssociationsSpec.groovy:180` and also registered by `HalEmbeddedSpec` (`addPersistentEntities(Person, Parent)`)
Per the dashboard the two worst specs are `JsonViewHelperSpec` (8 methods, 20 failures) and `JsonApiSpec` (7 methods, 20 failures) — and both still share entity classes with another spec after this change. `ExpandSpec` (19 failures) is the only one decoupled. So if class-keyed GORM state is the root cause, the flakiness should survive this PR.
Could you extend the same treatment to those specs? Given `Team`/`Player` are wanted by four specs, a shared read-only fixture file plus one mapping context, or per-spec copies as done here, would both work — the important thing is that no two independently-built mapping contexts see the same `Class`. A dedicated fixture source file would also make the ownership obvious and stop the next spec from picking them up by accident.
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy:
##########
@@ -104,6 +106,43 @@ trait JsonViewTest {
return templateEngine
}()
+ /**
+ * Resets the {@link ConstraintEvalUtils} default-constraints cache after every feature
+ * method so state from one test cannot leak into the next test that happens to run in the
+ * same JVM/fork.
+ *
+ * <p>{@link ConstraintEvalUtils} memoizes the default GORM constraints map in a single
+ * JVM-wide static field keyed by the identity of the last {@code Config} seen. Since each
+ * spec implementing this trait typically builds its own {@code GrailsApplication}/{@code
+ * Config}, a stale entry left behind by one spec can otherwise be picked up by another.
+ * This is cheap to recompute, so it is safe to clear after every feature.</p>
+ */
+ void cleanup() {
+ ConstraintEvalUtils.clearDefaultConstraints()
Review Comment:
Separately from the API concern: I don't think this clearing is load-bearing, because the cache is already reset once per spec class today.
`ConstraintEvalUtils`' static initialiser registers its own reset as a *preserved* shutdown operation:
```groovy
// grails-core/.../org/grails/validation/ConstraintEvalUtils.groovy:37
static {
ShutdownOperations.addOperation({ clearDefaultConstraints() } as Runnable, true)
}
```
`true` is `preserveForNextShutdown`, so `ShutdownOperations.runOperations()` re-adds it after each run and it fires every time. `GrailsUnitTest.cleanupGrailsApplication()` calls `runOperations()` (`GrailsUnitTest.groovy:184`), and nothing in the repo calls `ShutdownOperations.resetOperations()` — the only thing that would drop a preserved operation. Since the cache can only be non-empty if `ConstraintEvalUtils` has been loaded, and loading is what registers the reset, any populated cache is guaranteed to be cleared at the end of the spec that populated it. So cross-spec leakage of this cache is already impossible for `GrailsUnitTest` specs; this change only moves it from per-spec to per-feature, and a single spec has a single `Config`.
The stated mechanism also doesn't fit the failure rate. `getDefaultConstraints(config)` recomputes unless `configId == System.identityHashCode(config)`, so returning a stale map requires an identity-hash collision between two *consecutive* `Config` instances — roughly 1 in 2^31, not ~1%.
If you have a reproduction that shows otherwise I'd like to see it, since that would point at a real bug in `ConstraintEvalUtils`' identity-hash keying — which would be worth fixing there (e.g. a `WeakHashMap` keyed on the `Config` itself) rather than papered over from a test trait.
##########
grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy:
##########
@@ -104,6 +106,43 @@ trait JsonViewTest {
return templateEngine
}()
+ /**
+ * Resets the {@link ConstraintEvalUtils} default-constraints cache after every feature
+ * method so state from one test cannot leak into the next test that happens to run in the
+ * same JVM/fork.
+ *
+ * <p>{@link ConstraintEvalUtils} memoizes the default GORM constraints map in a single
+ * JVM-wide static field keyed by the identity of the last {@code Config} seen. Since each
+ * spec implementing this trait typically builds its own {@code GrailsApplication}/{@code
+ * Config}, a stale entry left behind by one spec can otherwise be picked up by another.
+ * This is cheap to recompute, so it is safe to clear after every feature.</p>
+ */
+ void cleanup() {
Review Comment:
Blocking: this breaks downstream specs, and there is no workaround available to the user.
`JsonViewTest` is a published trait. Because a Groovy trait method becomes an interface method, an implementing class must declare it `public` — but Spock's AST transform lowers the visibility of fixture methods. The two requirements are irreconcilable, so any application spec that implements `JsonViewTest` and declares its own `cleanup()` no longer compiles. Reproduced on this branch by adding a spec with a `cleanup()` body to `grails-views-gson/src/test`:
```
> Task :grails-views-gson:compileTestGroovy FAILED
startup failed:
.../TmpUserCleanupProbeSpec.groovy: 29: The method cleanup should be public as it implements
the corresponding method from interface grails.plugin.json.view.test.JsonViewTest
. At [29:5] @ line 29, column 5.
void cleanup() {
^
```
`cleanup()` is one of the most commonly used Spock fixtures, and the user's only remedy is to delete theirs. The same applies to `cleanupSpec()` below. It's already biting inside this PR: `JsonApiSpec` had to give up its own `cleanup()`, not because the teardown was unnecessary but because it can no longer declare one.
If per-feature isolation is wanted for this module's specs, it belongs in a test-scoped fixture — a base spec or trait under `grails-views-gson/src/test`, or in `grails-testing-support-views-gson`, which is already a `testImplementation` dependency and is the natural home for test-lifecycle behaviour. That keeps the shipped API unchanged.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5351110804
## ✅ All tests passed ✅
🏷️ Commit: d7ee26c4b2205329a663567b087f523b7ec3f697
▶️ Tests: 65618 executed
⚪️ Checks: 77/77 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5040045364
## ✅ All tests passed ✅
🏷️ Commit: 8a4a6eba392c80f45dcac9ed39a644be25f0e5f8
▶️ Tests: 16354 executed
⚪️ Checks: 60/60 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5350325563
Thanks for pushing on this — the mapping-context finding is a good catch, and I agree it changes what this PR should claim.
**On the premise.** You're right that `mappingContext` being a plain (non-`@Shared`) trait property means every feature already gets its own `KeyValueMappingContext`, with or without this PR. I've updated the description to stop framing this as a flakiness fix and call it what it actually is: closing a real same-package-borrowing footgun, independent of whether it explains #16030. The whole-spec-fixture-failure theory is still the leading explanation for that issue and remains open.
**On the design question — done, in `d7ee26c4b2`.** Switched from prefixed class names to per-spec sub-packages, each keeping the original simple names:
- `grails.plugin.json.view.expand`, `.include`, `.halembedded`, `.iterable`, `.maprender`, `.nullrendering` for `Team`/`Player`/`PlayerWithAge`/`Person`/`Address`
- `grails.plugin.json.view.api.handleassociations` for `Author`
Since `type`/`href` values come from the entity's simple class name, none of the expected-JSON assertions had to change — the diff on the 8 touched spec files dropped from +363/−219 to +100/−99, plus 7 small new fixture files. And it's self-enforcing the way you described: a same-package unqualified reference can't resolve into another spec's fixture anymore, because they're in different packages.
**Both remaining gaps from your last pass are closed:**
- `HalEmbeddedSpec`'s `Address` now has its own copy alongside `Person`.
- `NullRenderingSpec` gets its own `Child` POGO instead of unqualified-borrowing `PogoDeepRenderingSpec`'s `Child2`.
**`TemplateInheritanceSpec` and `PaginationSpec` are intentionally left alone**, per your reasoning: the published `child2`/`child4`/`circular` `.gson` templates import `grails.plugin.json.view.Player`/`.Circular` directly, so `TemplateInheritanceSpec`'s model has to be that exact class — I added a comment on the class explaining that so it doesn't read as an oversight next time. `PaginationSpec`'s explicit `import grails.plugin.json.view.Book` is disambiguating, not silently binding, so per your comment I left it as-is.
Also fixed the `captain.id == 1L` no-op you flagged — deleted rather than turned into an assignment, since the expected HAL link asserts no id.
Verified: full `:grails-views-gson:test` (178 tests) 0 failures, `codeStyle` clean.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5148353914
## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16033?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 51.8676%. Comparing base ([`6d1acad`](https://app.codecov.io/gh/apache/grails-core/commit/6d1acaddb6bf52db63370ade4e4ee8ec89d54bd3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a42b487`](https://app.codecov.io/gh/apache/grails-core/commit/a42b48780f6f8e123962fcba8792b0e4794263e3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 139 commits behind head on 8.0.x.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/grails-core/pull/16033?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## 8.0.x #16033 +/- ##
================================================
+ Coverage 0 51.8676% +51.8676%
- Complexity 0 18117 +18117
================================================
Files 0 2046 +2046
Lines 0 96274 +96274
Branches 0 16727 +16727
================================================
+ Hits 0 49935 +49935
- Misses 0 38970 +38970
- Partials 0 7369 +7369
```
[see 2046 files with indirect coverage changes](https://app.codecov.io/gh/apache/grails-core/pull/16033/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5148596964
## ✅ All tests passed ✅
🏷️ Commit: a42b48780f6f8e123962fcba8792b0e4794263e3
▶️ Tests: 44995 executed
⚪️ Checks: 60/60 completed
---
_Learn more about TestLens at [testlens.app](https://testlens.app)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5309077240
@jdaugherty nudge
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#discussion_r3816555407
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:
##########
@@ -33,25 +33,25 @@ class HalEmbeddedSpec extends Specification implements JsonViewTest {
JsonMapper objectMapper = JsonMapper.builder().build()
void setup() {
- mappingContext.addPersistentEntities(Team, Player)
+ mappingContext.addPersistentEntities(HalTeam, HalPlayer)
Review Comment:
This is the clearest place to raise it: I don't think the mapping-context rationale holds, and I checked rather than reasoned about it.
`JsonViewTest.mappingContext` is a plain trait property, not `@Shared`:
```groovy
// grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy:67
@Autowired(required = false)
MappingContext mappingContext = {
def ctx = new KeyValueMappingContext('test')
ctx.setCanInitializeEntities(true)
return ctx
}()
```
Groovy initialises trait properties from `$init$` in the implementing class's constructor, and Spock instantiates the spec once per feature method. So every feature gets its own `KeyValueMappingContext`, and this `setup()` re-registers the same two `Class` objects into a fresh one each time.
I confirmed it with a throwaway spec implementing `JsonViewTest` — three features, `setup()` calling `addPersistentEntities`, recording `System.identityHashCode` of both the context and the resulting `PersistentEntity`:
```
PROBE distinct mappingContexts=3 distinct PersistentEntities=3
```
Three features, three independently-built contexts, three distinct `PersistentEntity` instances for one `Class`.
So "no class is registered into two independently-built mapping contexts" isn't an invariant this codebase has, and it isn't one the renames can establish. Post-rename, `HalEmbeddedSpec` registers `HalTeam`/`HalPlayer` into nine independently-built contexts on its own (nine features, all through this `setup()`); `IterableRenderSpec`, `NullRenderingSpec` and `MapRenderSpec` do the same explicitly, per feature, in this very diff. Whether a *second spec* also registers the class isn't a categorical change — it's N versus 2N of something that already happens by design.
That moves the stated root cause from "unconfirmed", where the description currently leaves it, to "contradicted by the PR's own code". I'd rather the description drop the flakiness framing than leave a hypothesis in the permanent history that the diff itself disproves.
What still stands on its own merits is narrower: the template strings previously did `import grails.plugin.json.view.*` into a package holding 20+ specs' fixtures, and the specs referenced `Player`/`Team` unqualified. Both are genuinely fragile and worth closing. But that's a readability and robustness argument — which is exactly what the per-spec sub-package suggestion on `IterableRenderSpec` buys, at a fraction of the churn.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/IncludeAssociationsSpec.groovy:
##########
@@ -44,7 +45,29 @@ json jsonapi.render(players, [associations: false])
}
then: "The result is an array"
- renderResult.jsonText == '{"data":[{"type":"player","id":"1","attributes":{"name":"Cantona"}}],"links":{"self":"/foo"}}'
+ renderResult.jsonText == '{"data":[{"type":"includeAssociationsPlayer","id":"1","attributes":{"name":"Cantona"}}],"links":{"self":"/foo"}}'
}
}
+
+@Entity
+class IncludeAssociationsTeam {
Review Comment:
Two more borrows the sweep hasn't reached, beyond the `Address` and `Child2` cases I flagged last time:
- `TemplateInheritanceSpec` constructs `new Player(...)` in six places and `new Circular(...)` in `"test circular rendering is handled"`, both binding unqualified to `JsonViewHelperSpec`'s `@Entity` classes (`JsonViewHelperSpec:672` and `:690`). Those are the same `Team`/`Player` fixtures the rest of this PR is decoupling, so it's the last remaining `@Entity` case in the package.
- `PaginationSpec` (`grails.plugin.json.view.api`) imports `grails.plugin.json.view.Book`, which is `JsonViewTemplateEngineSpec`'s `@Linkable` POGO — while `JsonApiSpec` declares its own `Book` in `PaginationSpec`'s own package. Only the explicit import disambiguates the two; remove it and the reference silently flips to a different class. Lower stakes than the entity cases, but the same shape.
I'm not asking for six more renames on top of these — the opposite. Enumerating them is the argument for the sub-package approach, which would cover every case in the package mechanically and without touching a single expected-JSON string.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16033:
URL: https://github.com/apache/grails-core/pull/16033
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "borinquenkid (via GitHub)" <gi...@apache.org>.
borinquenkid commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5148374227
Thanks for the detailed review — all five points below are addressed in `a42b48780f`, but I want to be upfront that the central question you raised isn't resolved, only investigated further.
**The 5 inline points:**
1. Blocking (trait `cleanup()`/`cleanupSpec()` breaks downstream specs): reverted. `JsonViewTest` is back to its pre-PR shape.
2. `ConstraintEvalUtils` reset not load-bearing: agreed, moot now that point 1 is reverted — your `ShutdownOperations`/`preserveForNextShutdown` analysis was correct.
3. `cleanupSpec()` no-op: removed along with `@CompileDynamic`.
4. `ExpandSpec`-only decoupling incomplete: extended to `IncludeAssociationsSpec`, `HalEmbeddedSpec` (`Team`/`Player` and `Person`), `IterableRenderSpec`, `MapRenderSpec`, `NullRenderingSpec`, and `JsonApiHandleAssociationsSpec` (`Author`) — each gets its own spec-prefixed entity classes now, so no class is registered into two independently-built mapping contexts.
5. `JsonApiSpec`'s `SuperHero` cache left populated after the last feature: `cleanup()` restored (it only had to go in the first version because the trait declared one).
**On your stack-trace ask — still open.** I couldn't produce one either. What I checked:
- All ~50 failed CI runs on `8.0.x` in the last 30 days (the window #16030's dashboard covers): zero show a `grails-views-gson` failure.
- Ran the four originally-flagged specs together in a single JVM/fork (`-PmaxTestParallel=1 -PforkEveryUnitTest=0`) against the pre-this-PR base commit, 60 iterations (~44 min): 0 failures. Same result you got with your 4 runs.
So three independent attempts now (yours, and this one twice) have failed to reproduce it locally, and I couldn't find a corroborating CI job failure either. `testlens-app`'s dashboard is the only source for the failure counts, and its underlying per-test data isn't reachable via the GitHub API or CI artifacts — I couldn't get past the aggregate numbers to see what's actually throwing.
Given that, I've updated the PR description to stop asserting the class-cache-leak explanation as settled and instead flag it as the working hypothesis it is. The entity-decoupling change is worth keeping on its own merits (shared `Class` objects across independently-built mapping contexts is a real risk regardless), but I think your whole-spec-fixture-failure theory is still the more likely explanation for the *specific* pattern in #16030, and this PR doesn't confirm or rule it out either way. Open to suggestions on how to get real evidence here — happy to try something more targeted than brute-force repetition if you have an idea for what would actually trigger it.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on code in PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#discussion_r3724913097
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:
##########
@@ -421,16 +421,17 @@ class HalEmbeddedSpec extends Specification implements JsonViewTest {
void 'test hal embedded with associations that have GORM embedded properties'() {
given: 'A domain class with embedded associations'
- mappingContext.addPersistentEntities(Person, Parent)
- def p = new Person(name: 'Robert')
+ mappingContext.addPersistentEntities(HalPerson, Parent)
+ def p = new HalPerson(name: 'Robert')
p.homeAddress = new Address(postCode: '12345')
Review Comment:
The `Person` → `HalPerson` rename is right, but `Address` is still reaching into another spec by exactly the mechanism this PR is closing: it's declared in `EmbeddedAssociationsSpec` (line 190) and picked up here unqualified via same-package resolution.
It matters for the same reason `Person` did. `Address` is the embedded type of both `Person` and `HalPerson`, so `GormMappingConfigurationStrategy` calls `context.createEmbeddedEntity(Address)` — see `AbstractMappingContext#createEmbeddedEntity`, which builds a fresh `EmbeddedPersistentEntity(type, this)` bound to the calling context — once for this spec and once for `EmbeddedAssociationsSpec`. That's the identical `Class` object wrapped by two independently-built mapping contexts, which is the condition the rest of the PR eliminates.
Please give this spec its own `HalAddress` alongside `HalPerson`. It's a two-line change in a file you're already editing.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/IterableRenderSpec.groovy:
##########
@@ -339,3 +340,25 @@ class IterableRenderSpec extends Specification implements JsonViewUnitTest {
ex.message == 'Error rendering view: JSON API pagination arguments must contain resource and total'
}
}
+
+@Entity
+class IterableRenderTeam {
Review Comment:
Design question on the approach as a whole, anchored here because this file shows the cost most clearly.
Prefixing the class names forces every expected-JSON string in the spec to change, and that churn is most of the +363/−219. Per-spec sub-packages would buy the same isolation for almost none of it: the JSON API `type` comes from `PersistentEntity.decapitalizedName` (`DefaultJsonApiViewHelper:183`) and HAL hrefs from `GrailsNameUtils.getPropertyName(clazz)` (`TestLinkGenerator:72`) — both the *simple* name. So `grails.plugin.json.view.iterable.Player` renders byte-identically to today's `Player`, distinct `Class` object and all, and every assertion in the file stays untouched.
Two reasons I lean that way:
- Rewritten assertions lose their regression value. If name derivation itself regressed, the old strings would catch it; the new ones were written to match current output.
- It's self-enforcing. Nothing in this PR stops the next spec added to `grails.plugin.json.view` from typing `new Player(...)` and silently binding to `JsonViewHelperSpec` all over again. With per-spec packages that doesn't compile.
There's no template fallout to worry about: the module's only `.gson` fixtures live under `grails-app/views` and none of them are named for `player` or `team`.
This is a rework of a rename you've already done twice, so I'll leave the call to you. If you keep the prefixes, please add a line of comment above each duplicated fixture block saying why it's duplicated — otherwise someone will helpfully consolidate the seven copies back into one shared pair and reintroduce the problem.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:
##########
@@ -330,14 +330,14 @@ class HalEmbeddedSpec extends Specification implements JsonViewTest {
void 'test hal embedded method for one-to-many associations'() {
when: 'A GSON view that renders hal.embedded(..) is rendered'
- def player = new Player(id: 1L, name: 'Cantona')
+ def player = new HalPlayer(id: 1L, name: 'Cantona')
player.id = 1L
- def captain = new Player(name: 'Keane')
+ def captain = new HalPlayer(name: 'Keane')
captain.id == 1L
Review Comment:
`==` rather than `=`, so this line does nothing — Spock only treats bare conditions as assertions in `then:`/`expect:`, and this is a `when:` block. Pre-existing, but you're changing the lines directly above and below it, so it's free to fix here.
Worth noting the expected JSON further down asserts `"href": "http://localhost:8080/halPlayer"` with no id, i.e. the captain genuinely has no id and the feature is passing for the right reason. So the fix is to delete this line rather than turn it into an assignment — unless you'd rather set the id and update the expected href to `/halPlayer/1`.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/NullRenderingSpec.groovy:
##########
@@ -110,15 +111,37 @@ class NullRenderingSpec extends Specification implements JsonViewTest {
model {
Object obj
}
-
+
json g.render(obj, [renderNulls: true])
'''
when:
- mappingContext.addPersistentEntity(Player)
+ mappingContext.addPersistentEntity(NullRenderingPlayer)
def renderResult = render(templateText, [obj: new Child2()])
then:
objectMapper.readTree(renderResult.jsonText) == objectMapper.readTree('{"name": null, "parent": null}')
}
+}
+
+@Entity
+class NullRenderingTeam {
+ String name
+ NullRenderingPlayer captain
+ List players
+ List<String> titles
+ @SuppressWarnings('unused')
+ static hasMany = [players: NullRenderingPlayer]
+}
+
+@Entity
+class NullRenderingPlayer {
+ Long version
+ String name
+ @SuppressWarnings('unused')
+ static belongsTo = [team: NullRenderingTeam]
+
+ static constraints = {
+ name nullable: false
+ }
}
Review Comment:
Nit: still missing the trailing newline at EOF, and this commit rewrites the tail of the file anyway.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiHandleAssociationsSpec.groovy:
##########
@@ -72,3 +72,8 @@ class Publisher {
String name
}
+@Entity
+class HandleAssociationsAuthor {
+ String name
+}
+
Review Comment:
Nit: trailing blank line at EOF.
##########
grails-views-gson/src/test/groovy/grails/plugin/json/view/NullRenderingSpec.groovy:
##########
@@ -92,12 +93,12 @@ class NullRenderingSpec extends Specification implements JsonViewTest {
model {
Object obj
}
-
+
json g.render(obj)
'''
when:
- mappingContext.addPersistentEntity(Player)
+ mappingContext.addPersistentEntity(NullRenderingPlayer)
def renderResult = render(templateText, [obj: new Child2()])
Review Comment:
Same pattern as the `Team`/`Player` borrowing the rest of this PR fixes: `Child2` is declared in `PogoDeepRenderingSpec` and reached here unqualified.
Lower stakes than the entity cases — `Child2` is a plain POGO, so nothing registers it into a mapping context — but it's the same silent binding, in a file you're already changing. A `NullRenderingChild` local to this spec closes it.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky grails-views-gson tests caused by shared static test state [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty commented on PR #16033:
URL: https://github.com/apache/grails-core/pull/16033#issuecomment-5347781956
@borinquenkid
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org