jdaugherty commented on PR #16223:
URL: https://github.com/apache/grails-core/pull/16223#issuecomment-5412845105
Here are my notes on this change:
### Why the async changes are a -1
`AsyncWebRequestPromiseDecoratorLookupStrategy` caught the
`IllegalStateException` the container
throws when asked to start a second async cycle, returned no decorators, and
logged at debug.
`WebUtils.clearGrailsWebRequest` swallowed the same exception on the
teardown side. Both are the
same symptom: an async task operating on a request the container has already
recycled — i.e. data
that was not cleaned up correctly.
The result is a changed contract. A promise may now run undecorated with
nothing in the response and
nothing at any visible log level to say so. The tests pass because the
failure is quiet, not because
the cause is gone. Reverted: the two classes and the spec added with them,
restored byte-for-byte to
their pre-merge state.
### Two CI problems in the same PR
**The `~/.embedmongo` cache never worked.** It was added to all 17
dependency-jar cache steps across
both workflows, but nothing that can *write* that cache ever downloads a
mongod. `gradle.yml`'s
`build` job is the only `actions/cache/save` writer, and it runs
`-PonlyCoreTests`, which is in the
`onlyIf` of `mongodb-test-config.gradle` — no MongoDB task runs there.
`mongodbFunctional` and the
joint workflow do download a mongod, but they use plain `actions/cache@v4`,
which only saves on a
primary-key miss; the key already exists by then, so the binary was
discarded every run. Meanwhile
13 unrelated jobs restored up to 250MB they have no use for, and the
`hashFiles(dependencies.gradle)`
key threw the download away on every dependency bump.
Now it has its own step in only the two jobs that run a server, keyed
`embedmongo-<os>-<version>`,
no branch and no hash. The version in the key also settles the matrix —
under one key the entry that
missed first won and the other restored a binary it couldn't use.
**The joint workflow was testing three MongoDB releases at once.** It served
the example apps from a
`mongo:8` service container but named no version to the build, so
testcontainers used the `7.0.19`
default and the embedded server used `V7_0`. The service container is now
gone rather than aligned:
`StartMongoGrailsIntegrationExtension` probes `localhost:27017` and starts
its own container when
nothing answers, so it was only ever an optimisation — and
`mongodbFunctional` already runs the whole
MongoDB suite with no service container at all, as a required upstream of
`publish`.
`-PmongodbContainerVersion=8.0` now pins testcontainers and the embedded
server to one release.
### 90 tests were running in no task at all
Reviving this turned up something bigger. Classes that pollute static state
get a `Test` task of
their own and are excluded from `test`. A `Test` task that is *registered*
has neither
`testClassesDirs` nor `classpath` — the `java` plugin wires those onto its
own `test` task only — so
it is `NO-SOURCE`: **it reports success having run nothing.** Combined with
the `test` exclusion, the
classes ran nowhere.
Affected: the five `isolatedTestPatterns` tasks in `grails-test-suite-uber`
(40 tests) and
`execIsolatedTests` in `grails-test-suite-web` (50 tests).
This is older than it looks. `b156214b99` (Nov 2023) rewrote `task x(type:
Test)` as
`tasks.register(x, Test)`, but **both forms are `NO-SOURCE`** — I verified
them side by side in a
throwaway project — and Gradle was 7.6.3 on both sides of that commit. It
inherited the problem
rather than causing it. `TestTaskShardingPlugin` collects these through
`tasks.withType(Test)`, so
CI has been distributing empty tasks across shards.
Three things kept it quiet, all fixed:
1. the tasks ran nothing;
2. `grails-test-report` matches phases by exact task name, so results were
invisible to the aggregate
reports even when they did run;
3. in the web suite, nothing declared `execIsolatedTests` as work — `build`
→ `check` → `test` only,
so it was reachable just by naming it. `check` now depends on it.
The fix is additive in both modules: two properties per task, one `check`
dependency, and the task
names registered in the report. No task is removed.
### What the revived tests found
34 of 40 in the uber suite passed. Four failed, in `RestfulControllerSpec`
and
`ResourceAnnotationRestfulControllerSpec`: both assert that
`save`/`update`/`patch` re-render the
`create` or `edit` view for an instance with errors, and both build that
instance with an empty
title.
This is fallout from our intentional Grails 8 change making persistent
properties nullable by
default (`DefaultConstraintEvaluator.applyDefaultNullableConstraint`).
Binding an empty string
stores `null`, `blank` never fires on a null, so the instance was valid and
the controller
redirected. The domains said `title blank: false`, which meant required
under the old default; they
now say `nullable: false` as well, which is what they always meant. **These
tests were never updated
when we changed the default because they were dark — nothing flagged them.**
### Dead entries: only the ones that were deleted, not the ones that moved
Several isolated-test patterns name classes this repo no longer has. They
fall into two groups, and
only the first is touched.
**Deleted outright.** `JSONBindingTests` and `AutoParams*MarshallingTests`
are dropped from the web
suite's list. `1e38f1b5ea` (June 2013) removed all three of those classes —
335 deletions, zero
additions — because the params auto-marshalling feature went away. There is
no successor, so naming
them selects nothing and excludes nothing.
**Moved, and rewritten under new names.** Everything else stays exactly as
it is, patterns and tasks
alike:
- `GSPResponseWriterSpec` and `**/pages/ext/jsp/*` moved out in `6b0cf3817a`
(Apr 2017). The JSP
tests are back in the monorepo as `org.grails.gsp.jsp.*` (8 classes) in
`grails-gsp/plugin`.
- `GroovyPageAttributesTests` now lives in `grails-gsp/grails-taglib`.
- `GrailsDomainBinderTests`, `ComponentValidationTests` and
`HibernateMappingUniqueConstraintTests`
moved out in `ac96f5d5a0` (Mar 2013) and were rewritten as the
`org.grails.orm.hibernate.cfg.domainbinding.**` `*BinderSpec` suite plus
the `UniqueConstraint*` /
`EmbeddedWithValidationException` specs in `grails-data-hibernate5/7`.
- `DefaultGrailsControllerClassSpec` lives in `grails-core`, where it runs
and passes today.
`grails-test-suite-persistence` is therefore untouched, including
`testGrailsDomainBinder` and
`testIsolatedPersistentOne`.
### Deliberately left alone: GSP and Hibernate
Those moved tests came back into the monorepo when their repos were merged
in, but **without the
isolation they had here** — `grails-gsp/plugin` and
`grails-data-hibernate5/7` have no isolated test
tasks at all.
**I have not touched either.** Whether the original pollution concern still
applies is a separate
question — the Hibernate binder has been decomposed since, and those specs
pass today in a shared
fork — and it shouldn't ride along on a revert. Raising it here so it isn't
lost.
--
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]
