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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
