Fix flaky grails-views-gson tests caused by shared static test state - #16033
Fix flaky grails-views-gson tests caused by shared static test state#16033borinquenkid wants to merge 2 commits into
Conversation
CI's flaky-test dashboard (#16030) showed ~20 flaky test methods across JsonViewHelperSpec, ExpandSpec, JsonApiSpec and JsonViewTestSpec, all sharing a common root cause: static state that leaks between specs when several of them execute in the same test JVM/fork. Two concrete leaks were found: 1. ExpandSpec declared top-level `Team`/`Player` classes with no import, which silently resolved (same package, same simple names) to the *compiled `Team`/`Player` classes already defined by JsonViewHelperSpec*. Both specs then registered those identical Class objects into their own, independently-built KeyValueMappingContext instances, so any class-keyed GORM cache populated by one spec could be observed by the other. Fixed by giving ExpandSpec its own distinct `ExpandTeam`/`ExpandPlayer` domain classes (and updating the JSON/HAL assertions, whose type names and URLs are derived from the class name). 2. `org.grails.validation.ConstraintEvalUtils` memoizes the default GORM constraints map in a single JVM-wide static field keyed by `System.identityHashCode(config)`. JsonApiSpec already worked around this for its own SuperHero fixture with hand-rolled setup()/cleanup() logic (plus reflection into Validateable's internal static field), but JsonViewHelperSpec, ExpandSpec and JsonViewTestSpec had no equivalent reset, so a stale cache entry left by whichever spec ran first in a fork could be picked up by the next. Generalized the reset by adding a `cleanup()` fixture method directly to the `JsonViewTest` trait (grails-views-gson/src/main/.../test/JsonViewTest.groovy) that clears the ConstraintEvalUtils cache after every feature, so every spec implementing the trait gets it for free. A companion `cleanupSpec()` tears down any GrailsApplication cached by org.grails.testing.GrailsUnitTest, but only once per spec class (not per-feature): GrailsUnitTest intentionally builds and reuses its GrailsApplication across an entire spec's features, and some traits (e.g. DataTest) register beans into it once per spec, so tearing it down after every feature broke DataTest-based specs (MapRenderSpec) in testing. GrailsUnitTest itself is a test-only dependency that this main-sourceSet trait cannot reference directly, so the call is made dynamically only when the implementing spec actually has it. JsonApiSpec's own setup()/cleanup() was simplified accordingly: the reflection-based Validateable static field hack is replaced with the public `SuperHero.clearConstraintsMapCache()` API (available since 7.1), and the now-redundant ConstraintEvalUtils call is dropped since the trait handles it. Verified empirically (via isolated Groovy/Spock trait-composition probes) that a class overriding a trait-provided cleanup() fails to compile under Groovy 5/Spock 2.4, which is why JsonApiSpec no longer defines cleanup() itself. Also empirically confirmed that Groovy trait static fields are *not* shared across implementing classes (contrary to the initial triage hypothesis) - the actual leak vectors are the two described above. Full :grails-views-gson:test suite (178 tests) passes repeatedly, including reruns with --rerun-tasks and varied --tests subsets/orderings covering all previously-flagged specs plus the other GrailsUnitTest+JsonViewTest specs. codeStyle and aggregateStyleViolations report zero Checkstyle/CodeNarc violations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses flakiness in :grails-views-gson:test by eliminating shared JVM/static test state leaks between specs, improving test isolation without changing production/runtime behavior.
Changes:
- Added centralized per-feature cleanup to
JsonViewTestto clear the JVM-wideConstraintEvalUtilsdefault-constraints cache. - Updated
ExpandSpecto use its own dedicated@Entitydomain classes (ExpandTeam/ExpandPlayer) to avoid accidental cross-spec class reuse and class-keyed cache leakage. - Simplified
JsonApiSpecby removing the reflection-based cache reset and usingValidateable’s publicclearConstraintsMapCache()API viaSuperHero.clearConstraintsMapCache().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy | Introduces dedicated domain classes for this spec and updates expected JSON/link values accordingly to prevent cross-spec cache leakage. |
| grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy | Removes reflection-based cache manipulation and uses the public constraints-cache clear API for the Validateable fixture. |
| grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy | Adds standardized teardown hooks to clear shared validation constraint state after each feature and optionally tear down cached GrailsApplication after the spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Before taking a look at this, I had AI take a look. Here's it's comments:
Thanks for digging into #16030 — the triage write-up is genuinely useful, and disproving the trait-static hypothesis with isolated probes rather than assuming was the right instinct. Two things I'd like to resolve before this lands.
1. The JsonViewTest changes are in src/main. The PR body says "No production code changed", but grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy ships in the grails-views-gson artifact and is the documented way applications test JSON views. Adding cleanup()/cleanupSpec() to it is a breaking API change for downstream specs (details inline — I reproduced the compile failure locally). I also believe both fixture methods are redundant with machinery that already exists in grails-testing-support-core; sources cited inline.
2. The ExpandSpec decoupling looks right, but it's applied to one of four specs with the same problem — and not to the two with the highest failure counts. Details inline on ExpandSpec.
On evidence: the counts in #16030 are worth a second look. Every flagged method within a spec has an identical count (all 8 JsonViewHelperSpec methods 20/2037, all 7 JsonApiSpec methods 20/2023, all 4 ExpandSpec methods 19/2034, JsonViewTestSpec 19/2029). Identical per-method counts across an entire spec is the signature of ~20 CI runs in which those specs failed wholesale — a fixture/setup() throw or a fork-level failure — rather than independent per-assertion flakiness. That's a different shape of bug than a cache returning stale data, and it's the strongest clue available. Could you pull the actual stack trace from one of those failing runs?
The reason I'm pushing on that: a green suite doesn't discriminate between hypotheses here. I ran :grails-views-gson:test on this branch (178/178) and then again three times with the JsonViewTest change reverted and only the ExpandSpec change kept, single JVM, -PforkEveryUnitTest=0 -PtestBisect to maximise shared state — 178/178 every time. 8.0.x passes ~99% of the time on its own, so neither result tells us whether the leak is closed.
What I'd suggest: land the ExpandSpec and JsonApiSpec changes (both are improvements on their own merits), extend the class-decoupling to the remaining specs, and drop the JsonViewTest trait change.
| * 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() { |
There was a problem hiding this 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 cheap to recompute, so it is safe to clear after every feature.</p> | ||
| */ | ||
| void cleanup() { | ||
| ConstraintEvalUtils.clearDefaultConstraints() |
There was a problem hiding this 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:
// 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.
| * {@code grails-views-gson} artifact, so the call is made dynamically only when present.</p> | ||
| */ | ||
| @CompileDynamic | ||
| void cleanupSpec() { |
There was a problem hiding this 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:
// 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.
|
|
||
| void setup() { | ||
| mappingContext.addPersistentEntities(Team, Player) | ||
| mappingContext.addPersistentEntities(ExpandTeam, ExpandPlayer) |
There was a problem hiding this 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.*plusaddPersistentEntities(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.Authoris declared inapi/JsonApiSpec.groovy:449and also registered byapi/JsonApiHandleAssociationsSpec(addPersistentEntities(Author, PublishedBook, Publisher))Personis declared inEmbeddedAssociationsSpec.groovy:180and also registered byHalEmbeddedSpec(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.
| // 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() |
There was a problem hiding this 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.
…d test entities jdaugherty's review on #16033 raised two issues: 1. Blocking: adding cleanup()/cleanupSpec() to the published JsonViewTest trait (grails-views-gson/src/main) breaks any downstream spec that declares its own cleanup()/cleanupSpec(), because a Groovy trait method becomes a public interface method while Spock's AST transform lowers the visibility of fixture methods it generates - the two are irreconcilable. Separately, the ConstraintEvalUtils reset this added isn't load-bearing: ConstraintEvalUtils registers its own reset as a preserved ShutdownOperations entry, so the cache is already cleared once per spec for every GrailsUnitTest spec today. Both fixture methods are removed; JsonViewTest reverts to its pre-#16033 shape. 2. The ExpandSpec fix (dedicated ExpandTeam/ExpandPlayer entities instead of an unqualified same-package reference to JsonViewHelperSpec's Team/Player) was applied to only one of several specs with the same problem, and not to the worst offenders. JsonViewHelperSpec declares Team, Player and PlayerWithAge; IncludeAssociationsSpec, HalEmbeddedSpec, IterableRenderSpec, MapRenderSpec and NullRenderingSpec all implicitly borrowed Team/Player via unqualified same-package resolution and registered the identical Class objects into their own independently-built KeyValueMappingContext/GORM mocks. HalEmbeddedSpec additionally borrowed Person from EmbeddedAssociationsSpec, and JsonApiHandleAssociationsSpec borrowed Author from JsonApiSpec. Each of these specs now gets its own spec-prefixed entity classes (IncludeAssociationsPlayer/Team, HalPlayer/Team/Person, IterableRenderPlayer/Team, MapRenderPlayer/Team, NullRenderingPlayer/Team, HandleAssociationsAuthor), so no class is ever registered into two independently-built mapping contexts. JsonViewHelperSpec, EmbeddedAssociationsSpec and JsonApiSpec keep their original classes unchanged since those are no longer borrowed by anyone else. JsonApiSpec's cleanup() (removed in the original PR when the trait started declaring one) is restored now that the trait no longer declares its own, closing the gap the reviewer noted where SuperHero's constraints cache was only reset on the way into the spec, not on the way out. Full :grails-views-gson:test (178 tests): 0 failures. codeStyle (checkstyle + CodeNarc on src/main) clean; checkstyle/codenarc on test sources are skipped project-wide, unchanged by this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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 🚀 New features to boost your workflow:
|
|
Thanks for the detailed review — all five points below are addressed in The 5 inline points:
On your stack-trace ask — still open. I couldn't produce one either. What I checked:
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. 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 |
✅ All tests passed ✅🏷️ Commit: a42b487 Learn more about TestLens at testlens.app. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:337
captain.id == 1Lis a no-op comparison in awhen:block (Spock only treats conditions as assertions inthen/expect). If the intent is to leave the id unset (so the expected HAL link has no id), this line should be removed or replaced with a clarifying comment; if the intent is to set the id, use assignment (=) and update the expected JSON accordingly.
def player = new HalPlayer(id: 1L, name: 'Cantona')
player.id = 1L
def captain = new HalPlayer(name: 'Keane')
captain.id == 1L
def team = new HalTeam(captain: captain, name: 'Manchester United', players: [player])
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Thanks for turning this round quickly — both asks from my last review are addressed. git diff <merge-base>..a42b487 -- grails-views-gson/src/main comes back empty, so the trait is byte-identical to base and "No production code changed" now holds, and the entity decoupling is extended to six more specs. I also appreciate how explicit the PR body is about what the evidence does and doesn't show.
Verified locally on a42b487: DO_NOT_CACHE_TESTS=1 ./gradlew :grails-views-gson:test → 178 tests, 0 failures. The branch is 314 commits behind 8.0.x, but 8.0.x has no changes under grails-views-gson since your merge base, so there are no conflicts to expect.
Two things I'd like resolved before this merges, plus one design question.
1. The decoupling pass is still incomplete, including in files this PR edits. Details inline on HalEmbeddedSpec (Address) and NullRenderingSpec (Child2). Beyond those two, TemplateInheritanceSpec still resolves Player and Circular out of JsonViewHelperSpec by the same unqualified same-package mechanism — so the commit message's claim that "JsonViewHelperSpec, EmbeddedAssociationsSpec and JsonApiSpec keep their original classes unchanged since those are no longer borrowed by anyone else" isn't accurate. There's a good reason those two are awkward to move: grails-app/views/_child{2,3,4}*.gson and circular/_circular.gson import grails.plugin.json.view.Player and grails.plugin.json.view.Circular directly, so renaming them means touching published-module templates. Please state that as the reason rather than claiming nothing borrows them. (api/PaginationSpec also imports grails.plugin.json.view.Book from JsonViewTemplateEngineSpec, but that one is an explicit import rather than a silent binding, so I'd leave it.)
2. captain.id == 1L in HalEmbeddedSpec — inline.
3. Design question: per-spec sub-packages instead of name prefixes — inline on IterableRenderSpec, where the churn is easiest to see.
Nits, none blocking:
HalEmbeddedSpecimportsgrails.gorm.annotation.Entitywhile the other new fixture blocks usegrails.persistence.Entity. Both work, and each file is internally consistent, so only worth aligning if it's cheap.- Several of the copied fixtures carry fields the borrowing spec never touches (
IncludeAssociationsTeam.captain/titles,NullRenderingTeamin its entirety). They're faithful copies of the originals, which is defensible; trimming is optional. - Two EOF nits flagged inline.
On #16030: I'd land this on its own merits, but please don't close #16030 with it, and consider retitling the PR and branch to what's actually verifiable — something like "Isolate shared test entities in grails-views-gson specs". My own runs don't discriminate between hypotheses any better than yours do; 178/178 green tells us nothing about whether the leak is closed. The identical per-method failure counts within each spec are still the strongest lead, and I'd like the issue left open pointing at that rather than treated as resolved by association.
| def p = new Person(name: 'Robert') | ||
| mappingContext.addPersistentEntities(HalPerson, Parent) | ||
| def p = new HalPerson(name: 'Robert') | ||
| p.homeAddress = new Address(postCode: '12345') |
There was a problem hiding this 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.
| player.id = 1L | ||
| def captain = new Player(name: 'Keane') | ||
| def captain = new HalPlayer(name: 'Keane') | ||
| captain.id == 1L |
There was a problem hiding this 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.
| when: | ||
| mappingContext.addPersistentEntity(Player) | ||
| mappingContext.addPersistentEntity(NullRenderingPlayer) | ||
| def renderResult = render(templateText, [obj: new Child2()]) |
There was a problem hiding this 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.
| } | ||
|
|
||
| @Entity | ||
| class IterableRenderTeam { |
There was a problem hiding this 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.viewfrom typingnew Player(...)and silently binding toJsonViewHelperSpecall 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.
| static constraints = { | ||
| name nullable: false | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Nit: still missing the trailing newline at EOF, and this commit rewrites the tail of the file anyway.
| class HandleAssociationsAuthor { | ||
| String name | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: trailing blank line at EOF.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:372
- The expected captain self link is currently the collection URL (
/halPlayer) which matches a null id. If the captain id is meant to be set (see setup above), the expected URL should include the id to avoid asserting the wrong behavior.
"_links": {
"self": {
"href": "http://localhost:8080/halPlayer",
"hreflang": "en",
"type": "application/hal+json"
}
grails-views-gson/src/test/groovy/grails/plugin/json/view/HalEmbeddedSpec.groovy:336
captain.id == 1Luses the equality operator, so it never assigns an id to the captain. This makes the test setup inconsistent with the other HAL link assertions and can produce different link output than intended.
This issue also appears on line 367 of the same file.
def player = new HalPlayer(id: 1L, name: 'Cantona')
player.id = 1L
def captain = new HalPlayer(name: 'Keane')
captain.id == 1L
def team = new HalTeam(captain: captain, name: 'Manchester United', players: [player])
Summary
~20 test methods across
JsonViewHelperSpec,JsonViewTestSpec,JsonApiSpec, andExpandSpec(module:grails-views-gson:test) show the same failure+flakinesspattern (~2% failures, ~1% flakiness per #16030) — one shared root
cause, not 20 independent bugs.
Root cause (hypothesis, not confirmed — see Testing)
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. The current working hypothesis is a class-keyed
GORM cache leak:
ExpandSpec(and, as of this update, several other specs) silently resolved shareddomain classes like
Team/Playervia an unqualified same-package reference (noimport), so multiple specs registered the identical
Classobjects intoindependently-built
KeyValueMappingContextinstances — letting class-keyed GORMcaches leak between unrelated specs.
This has not been confirmed against an actual failure. jdaugherty's review raised
a specific, unresolved counter-observation: every flagged method within a given spec
in #16030's dashboard shares an identical failure count (e.g. all 8
JsonViewHelperSpecmethods at 20/2165), which looks like whole-spec/fixture-level failures rather than
independent per-assertion staleness — a different bug shape than this fix addresses.
See Testing below for what was and wasn't found while investigating this.
Fix
JsonViewTest(the publishedgrails-views-gsontrait) to its pre-PRshape. An earlier version of this PR added
cleanup()/cleanupSpec()directly toit; that breaks any downstream spec implementing
JsonViewTestthat declares itsown
cleanup(), because a Groovy trait method becomes a public interface methodwhile Spock's AST transform lowers the visibility of generated fixture methods —
the two are irreconcilable. Confirmed via a local reproduction of the compile
failure. Separately, the
ConstraintEvalUtilsreset this added isn't load-bearing:ConstraintEvalUtilsregisters its own reset as a preservedShutdownOperationsentry, so the cache is already cleared once per spec for every
GrailsUnitTestspectoday.
ExpandSpec(its ownExpandTeam/ExpandPlayer) to the other specs implicitly borrowing the sameclasses:
IncludeAssociationsSpec,HalEmbeddedSpec(Team/PlayerandPerson, borrowed fromEmbeddedAssociationsSpec),IterableRenderSpec,MapRenderSpec,NullRenderingSpec, andJsonApiHandleAssociationsSpec(
Author, borrowed fromJsonApiSpec) each now get their own spec-prefixeddomain classes, so no class is registered into two independently-built mapping
contexts.
JsonApiSpeckeeps the publicSuperHero.clearConstraintsMapCache()API (in placeof the earlier reflection hack into
Validateable's internal state) and restoresits own
cleanup()— dropped in an earlier revision when the trait declared one —now that the trait no longer owns that fixture method.
No production code changed.
Testing
:grails-views-gson:test(178 tests): 0 failures, run to completion multipletimes including with varied
--testsorderings across the originally-flagged specs.proof of correctness:
-PforkEveryUnitTest=0 -PtestBisect, maximizing shared state) — both with thisPR's changes and with them reverted.
8.0.xin the last 30 days (the exact windowTest Dashboard #16030's dashboard covers): none show a
grails-views-gsontest failure.(
-PmaxTestParallel=1 -PforkEveryUnitTest=0) against the pre-this-PR base commit,60 iterations, ~44 minutes: 0 failures.
this PR. The Test Dashboard #16030 dashboard (generated by the
testlens-appGitHub App) is theonly source for the failure counts cited above; its underlying per-test data
isn't reachable via the GitHub API or CI artifacts.
Given the above, this PR fixes a real, independently-verifiable defect (classes
silently shared across independently-built GORM mapping contexts is a genuine risk
regardless of whether it explains #16030) but does not have confirmed evidence that
it fixes the specific flakiness in #16030. Continuing to investigate the
whole-spec-fixture-failure hypothesis jdaugherty raised is worthwhile follow-up,
separate from whether this PR should land.
Related: #16030