Skip to content

Fix flaky grails-views-gson tests caused by shared static test state #16033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
borinquenkid wants to merge 1 commit into
base: 8.0.x
Choose a base branch
from
+96 −52
Open

Fix flaky grails-views-gson tests caused by shared static test state #16033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions grails-views-gson/src/main/groovy/grails/plugin/json/view/test/JsonViewTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package grails.plugin.json.view.test

import groovy.json.JsonSlurper
import groovy.text.Template
import groovy.transform.CompileDynamic
import groovy.transform.CompileStatic

import org.springframework.beans.factory.annotation.Autowired
Expand All @@ -41,6 +42,7 @@ import grails.web.mapping.LinkGenerator
import grails.web.mime.MimeUtility
import org.grails.datastore.mapping.keyvalue.mapping.config.KeyValueMappingContext
import org.grails.datastore.mapping.model.MappingContext
import org.grails.validation.ConstraintEvalUtils

/**
* A trait that test classes can implement to add support for easily testing JSON views
Expand Down Expand Up @@ -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() {

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ConstraintEvalUtils.clearDefaultConstraints()

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* 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() {

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (metaClass.respondsTo(this, 'cleanupGrailsApplication')) {
cleanupGrailsApplication()
}
}

/**
* Render a template for the given source
*
Expand Down
83 changes: 52 additions & 31 deletions grails-views-gson/src/test/groovy/grails/plugin/json/view/ExpandSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package grails.plugin.json.view

import tools.jackson.databind.json.JsonMapper
import grails.persistence.Entity
import grails.plugin.json.view.test.JsonRenderResult
import grails.plugin.json.view.test.JsonViewTest
import org.grails.datastore.mapping.core.Session
Expand All @@ -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)

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

void 'Test expand parameter allows expansion of child associations'() {
given: 'An entity with a proxy association'
def mockSession = Mock(Session)
mockSession.getMappingContext() >> mappingContext
mockSession.retrieve(Team, 1L) >> new Team(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 1L)
mockSession.retrieve(ExpandTeam, 1L) >> new ExpandTeam(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 1L)

def player = new Player(name: 'Cantona', team: teamProxy)
def player = new ExpandPlayer(name: 'Cantona', team: teamProxy)

def templateText = '''
import grails.plugin.json.view.*
import grails.plugin.json.view.ExpandPlayer as Player

@Field Player player

json g.render(player)
'''

Expand Down Expand Up @@ -84,15 +85,13 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
given: 'An entity with a proxy association'
def mockSession = Mock(Session)
mockSession.getMappingContext() >> mappingContext
mockSession.retrieve(Team, 1L) >> new Team(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 1L)
mockSession.retrieve(ExpandTeam, 1L) >> new ExpandTeam(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 1L)

def player = new Player(name: 'Cantona', team: teamProxy)
def player = new ExpandPlayer(name: 'Cantona', team: teamProxy)
def templateText = '''
import grails.plugin.json.view.*

@Field Map map

json g.render(map)
'''

Expand All @@ -119,13 +118,13 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
given: 'An entity with a proxy association'
def mockSession = Mock(Session)
mockSession.getMappingContext() >> mappingContext
mockSession.retrieve(Team, 1L) >> new Team(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 1L)
mockSession.retrieve(ExpandTeam, 1L) >> new ExpandTeam(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 1L)

def player = new Player(name: 'Cantona', team: teamProxy)
def player = new ExpandPlayer(name: 'Cantona', team: teamProxy)

def templateText = '''
import grails.plugin.json.view.*
import grails.plugin.json.view.ExpandPlayer as Player
model {
Player player
}
Expand All @@ -140,7 +139,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
{
"_links": {
"self": {
"href": "http://localhost:8080/player",
"href": "http://localhost:8080/expandPlayer",
"hreflang": "en",
"type": "application/hal+json"
}
Expand All @@ -161,7 +160,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
"team": {
"_links": {
"self": {
"href": "http://localhost:8080/team/1",
"href": "http://localhost:8080/expandTeam/1",
"hreflang": "en",
"type": "application/hal+json"
}
Expand All @@ -171,7 +170,7 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
},
"_links": {
"self": {
"href": "http://localhost:8080/player",
"href": "http://localhost:8080/expandPlayer",
"hreflang": "en",
"type": "application/hal+json"
}
Expand All @@ -185,48 +184,48 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
given: 'An entity with a proxy association'
def mockSession = Mock(Session)
mockSession.getMappingContext() >> mappingContext
mockSession.retrieve(Team, 9L) >> new Team(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, Team, 9L)
def player = new Player(name: 'Cantona', team: teamProxy)
mockSession.retrieve(ExpandTeam, 9L) >> new ExpandTeam(name: 'Manchester United')
def teamProxy = mappingContext.proxyFactory.createProxy(mockSession, ExpandTeam, 9L)
def player = new ExpandPlayer(name: 'Cantona', team: teamProxy)
player.id = 3

when: 'The domain is rendered with expand parameters'
JsonRenderResult result = render('''
import grails.plugin.json.view.*
import grails.plugin.json.view.ExpandPlayer as Player
model {
Player player
}

json jsonapi.render(player, [expand: 'team'])
''', [player: player])

then: 'The JSON relationships are in place'
objectMapper.readTree(result.jsonText) == objectMapper.readTree('''
{
"data": {
"type": "player",
"type": "expandPlayer",
"id": "3",
"attributes": {
"name": "Cantona"
},
"relationships": {
"team": {
"links": {
"self": "/team/9"
"self": "/expandTeam/9"
},
"data": {
"type": "team",
"type": "expandTeam",
"id": "9"
}
}
}
},
"links": {
"self": "/player/3"
"self": "/expandPlayer/3"
},
"included": [
{
"type":"team",
"type":"expandTeam",
"id": "9",
"attributes": {
"titles": null,
Expand All @@ -241,11 +240,33 @@ class ExpandSpec extends Specification implements JsonViewTest, GrailsUnitTest {
}
},
"links": {
"self": "/team/9"
"self": "/expandTeam/9"
}
}
]
}
''')
}
}

@Entity
class ExpandTeam {
String name
ExpandPlayer captain
List players
List<String> titles
@SuppressWarnings('unused')
static hasMany = [players: ExpandPlayer]
}

@Entity
class ExpandPlayer {
Long version
String name
@SuppressWarnings('unused')
static belongsTo = [team: ExpandTeam]

static constraints = {
name nullable: false
}
}
26 changes: 5 additions & 21 deletions grails-views-gson/src/test/groovy/grails/plugin/json/view/api/JsonApiSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mappingContext.addPersistentEntities(Widget, Author, Book, ResearchPaper)
}

void cleanup() {
ConstraintEvalUtils.clearDefaultConstraints()
clearConstraintsMapCache(SuperHero)
}

/**
* Clears the private static constraintsMapInternal field in the Validateable trait.
*/
private static void clearConstraintsMapCache(Class<?> clazz) {
if (STATIC_FIELD_HELPER.isAssignableFrom(clazz)) {
def setterMethod = clazz.getMethod('grails_validation_Validateable__constraintsMapInternal$set', Map)
setterMethod.invoke(null, (Map) null)
}
}

void 'test simple case'() {
given:
def theWidget = new Widget(name: 'One', width: 4, height: 7)
Expand Down
Loading