Repository navigation
test: fix five load-sensitive flaky tests by moving module loads out of timed windows - #3010
Merged
Merged
Conversation
… chunk
DatasetPreview and ReportPreview render their renderers behind
`React.lazy(() => import('@object-ui/plugin-charts' | '@object-ui/plugin-report'))`,
so everything the assertions wait on lives *behind* a Suspense boundary and
does not exist until that chunk resolves. Loading it is unbounded work: under
full-suite parallelism Vite's transform pipeline is saturated (the `dom-heavy`
project alone spends ~60s in transform), and the first import of the package
graph can outlast RTL's 1000ms `waitFor`/`findBy` window.
Instrumenting the import under a real full `dom-heavy` run measured
`import('@object-ui/plugin-report')` at 312ms / 424ms / 976ms across three
runs — that last one consumes 97.6% of the entire assertion budget before a
single render or `queryDataset` call can happen. A captured failure confirms
it: the DOM at timeout still showed the "Loading report renderer…" Suspense
fallback, and `queryDataset` had 0 calls because `ReportRenderer` — the thing
that calls it — had not loaded yet.
That explains exactly which tests flaked:
- ReportPreview.dataset: the *first two* tests, the ones running while the
very first import is still in flight; tests 3-5 find the module warm.
- DatasetPreview: only the ratio-measure test, because its "use the right
axis" caption is the one assertion in the file that sits *inside* the
Suspense boundary. Earlier tests assert on the table, which renders outside
it — so they start the import but never wait for it, leaving the last test
to race a load already in flight.
Fix: resolve each chunk once in `beforeAll`, taking the module load out of
every assertion window rather than widening the windows. ESM caches by
resolved specifier, so warming the same specifier makes the component's own
`React.lazy` factory resolve immediately. No timeout was raised and no file
was moved off the `dom-heavy` list.
Verified: `--project dom-heavy` failed 1 run in 4 before the change (the
captured failure above) and is green 14 consecutive runs after it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…avy flakes)
Follow-up to the previous commit, which fixed the two `dom-heavy` preview files
with a `beforeAll` warm-up. Two changes here:
1. Switch that warm-up to a module-scope import. `beforeAll` is bounded by
`hookTimeout` (10s) — *narrower* than the 15s `testTimeout` it replaced. The
import phase has no such bound (a full run logs `import 1523s` cumulative
across 716 files without an import-phase failure), and it is where module
loading naturally belongs. Same idiom now used for every fix below.
2. Extend the same treatment to three more files that share the root cause —
unbounded work billed against a bounded window — found while verifying:
- `anchors.page-seed.test.ts`: `createSeed` reaches the page synthesizer via
`await import('@object-ui/plugin-detail')` (anchors.ts:207), so the cold
transform of that graph is billed to the test body. With `isolate: false`
the `unit` project's shared worker graph makes "who pays the cold
transform" scheduling-dependent, so it blew the 15s budget only sometimes.
Verified a timeout, not a wrong result: `createSeed` swallows failures and
returns `{}`, so a genuine synth failure would read as
`expected false to be true`, which was never observed.
- `plugin-kanban/src/index.test.ts`: was
`beforeAll(async () => { await import('./index') }, 15000)` — already
"fixed" once by raising its timeout, and still timed out under full load
(observed 15021ms, surfacing as 8 skipped tests rather than a failed
assertion). The raised timeout is deleted, not re-raised.
Tests: 15021ms hook -> 2ms.
- `all-locales-key-parity.test.ts`: a genuine quadratic, not a load effect.
Line 64 called `keysOf(builtInLocales[lang])` *inside* the `.filter()`
predicate, re-walking the whole locale tree once per `en` key (~2.5k keys x
a full recursive walk). The sibling assertion below it always hoisted the
call; this one didn't. Hoisting it: tests 7.51s -> 25ms (~300x), same 20
assertions passing. Parity itself is clean — all 9 non-`en` packs have 0
missing and 0 extra keys — so this was never a missing-translation failure.
No timeout was raised and no file was moved to an allowlist.
Verified:
- `--project dom-heavy`: failed 1 run in 4 before; green 20/20 runs after
(14 on the `beforeAll` form, 6 on the final static-import form).
- `--project unit`: the four files above pass; the 7 files that still fail are
a pre-existing broken baseline (real AssertionErrors in spec-subschema-parity,
onMutation, filter-operator-ast-parity x2, view-config-utils,
view-operator-builder-parity, clientValidation.fieldRules) — unchanged by
this commit, and unrelated to timing.
- Full suite before vs after: identical 19 genuine test failures, no new
failures, and no cross-file interference from the module-scope imports
despite `isolate: false`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
#3010 fixed five load-sensitive flaky tests that all shared one root cause — an unbounded module load billed against a bounded window — and found one file that had already been "fixed" once by raising its timeout to 15s (and still blew it at 15021ms under full load). Without writing the rule down, the next agent reaches for the same band-aid. Adds a `测试纪律` subsection to §9 covering: assert-behind-a-lazy-boundary -> import at module scope (the import phase has no test/hook timeout); why `beforeAll` is the wrong warm-up spot (`hookTimeout` 10s is *narrower* than `testTimeout` 15s); that raising a timeout or adding a file to `heavyDomTests` is forbidden as a flake fix; how to read the two tells (a Suspense fallback in the dump, and a failed *file* with 0 failed *tests*); and the stale-install trap that makes a wall of local parity failures look like a regression. Docs only — no code, no changeset. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…3021) Follow-up to #3010 and the §9 rule added in #3015. A sweep for the same anti-pattern — `beforeAll(async () => { await import(…) })`, which bills a cold module transform to `hookTimeout` — found 36 more test files, every single one already carrying a raised timeout, on an escalating ladder: 15s -> 30s -> 60s. That ladder is the evidence that raising the timeout never fixed anything. This takes the four highest-risk ones. Each drops the raised timeout instead of raising it again; the import phase has no test or hook timeout. - `plugin-markdown/src/index.test.ts` (was 15s) — the twin of the file fixed in #3010: same shape, same budget, same comment verbatim ("Increase timeout to 15 seconds for async import"). plugin-kanban proved that budget insufficient by blowing it at 15021ms, and this file's timed portion was already 5.83s of the same 15s. This was a live flake, not a theoretical one. - `plugin-charts/src/index.test.ts` (was 60s) — recharts-backed graph. - `plugin-editor/src/index.test.ts` (was 30s) — Monaco. - `apps/console/.../public-contract.test.ts` (was 60s) — also folds the `let contract` + hook assignment into a module-scope `const`, now that the two side-effect imports are evaluated before the module body. The console file was the delicate one: its assertions pin the eager/lazy registration split (objectui#2953), so the concern was whether moving the imports perturbs what is loaded when. It does not — static imports are evaluated in order before the module body, and the shared setup still runs before this module is imported at all. Its order-sensitive "reaches the lazily-registered blocks before their chunks load" test passes unchanged. Timed portion of each file, isolated (before -> after): plugin-markdown 5.83s -> 2ms plugin-charts 2.22s -> 2ms plugin-editor 1.78s -> 4ms public-contract 345ms -> 3ms Verified: full suite 100% green — 731 files passed / 1 skipped, 8521 tests passed / 24 skipped, zero failures. The `unit` project alone is 297/297 files and 4084/4084 tests, which also confirms no cross-file interference from the new module-scope registrations despite that project running `isolate: false`. Note for anyone baselining locally: the ~19 failures this repo can show on a long-lived worktree are a STALE INSTALL (`node_modules` @objectstack/spec 14.6.0 vs ^17.0.0-rc.0 declared), not a regression. `pnpm install --prefer-offline` clears all of them — hence the all-green run above. Already documented in §9. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
… 33 sites (#3026) The prose rule added in #3015 did not hold on its own, and there is hard evidence for that: sweeping for `beforeAll(async () => { await import(…) })` found 37 files, and EVERY one already carried a raised timeout, on an escalating ladder — 15s -> 30s -> 60s. One even left the escalation as advice (`// Increase timeout to 30 seconds for heavy renderer imports`), and plugin-kanban blew its raised 15s anyway at 15021ms (#3010). A convention that gets re-broken 37 times is a lint rule. Adds `object-ui/no-dynamic-import-in-test-hook` (error, scoped to test files): a module loaded inside `beforeAll`/`beforeEach` bills its cold Vite transform to `hookTimeout`, so the test passes or fails on machine load rather than on the code it covers. Imported at module scope the same cost lands in the import phase, which no test or hook timeout applies to. Kept deliberately narrow, with both exemptions pinned in the RuleTester because both are legitimate and both are real code here: - a lazy FACTORY — `registerLazy('x', () => import('./x'))` — the hook installs a loader, it never runs the import. The hook frame resets inside a nested function, so this never reports. (Without this the rule would flag the console's own plugin registration and be unusable.) - a deliberate re-import against per-run module state (`vi.resetModules`, `doMock`, `doUnmock`, `stubEnv`, `stubGlobal`) — there the re-import is the point, and hoisting it would break the test rather than speed it up. No autofixer on purpose: the mechanical case is a one-line move, but the general case captures the module into a variable the tests read, and hoisting that means introducing top-level await and reordering side effects. Because an `error` ratchet has to lint clean today (the convention the other three rules follow), this also converts the 33 remaining sites — 30 in packages/components, 3 in plugin-dashboard. 27 were the canonical pure warm-up. The other 6 also register overrides that must run AFTER the barrel, so only the import moved out; those keep a (now sync, now untimed) hook, and the ordering still holds because static imports are evaluated before any hook. The side-effect import goes after the last existing import — the faithful equivalent of a hook that ran once every static import had been evaluated. Verified: - rule: 19 RuleTester cases (12 valid / 7 invalid); a fresh violation fails lint as an error with the exact fix in the message. - repo sweep: 33 violations -> 0. The only remaining error-level lint findings are 3 pre-existing ones under `e2e/`, which CI's per-package `turbo run lint` does not cover; untouched here. - tests: the 33 converted files pass (209 tests), and the full suite is 100% green — 733 files passed / 1 skipped, 8552 tests passed / 24 skipped. - the suite's cumulative timed portion dropped 243s -> 127s. Wall clock is unchanged (~229s): the work did not disappear, it moved out of timed windows into the import phase, which is the point. AGENTS.md §9 now points at the rule instead of only describing the convention, including both exemptions so the next agent does not fight the linter. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The race
Three
dom-heavytests failed intermittently under full-suite parallel load but passed in isolation. All of them turned out to be one root cause, and it is not in the code under test:An unbounded module load sitting inside a bounded assertion window.
DatasetPreviewandReportPreviewrender their renderers behindReact.lazy(() => import('@object-ui/plugin-charts' | '@object-ui/plugin-report')). Everything the assertions wait on lives behind that Suspense boundary and does not exist until the chunk resolves. Under full parallelism Vite's transform pipeline is saturated — thedom-heavyproject alone spends ~60s in transform — so the first import of the package graph can outlast RTL's 1000ms defaultwaitFor/findBywindow.Evidence
A captured failure shows the DOM at timeout still rendering the Suspense fallback:
queryDatasethad 0 calls becauseReportRenderer— the thing that calls it — had not loaded yet. The test duration was 1070ms: the 1000ms window plus overhead.Instrumenting the import under a real full
dom-heavyrun measuredimport('@object-ui/plugin-report')at 312ms / 424ms / 976ms across three runs. That last one consumes 97.6% of the entire assertion budget before a single render or query can happen.This explains exactly which tests flaked:
ReportPreview.dataset.test.tsxDatasetPreview.test.tsxThe fix
Load each module at module scope in the test file, so the cost lands in the import phase — which no test or hook timeout applies to — instead of inside an assertion window. ESM caches by resolved specifier, so warming the same specifier makes the component's own
React.lazyfactory resolve immediately.No timeout was raised and no file was moved to an allowlist.
The two
unit-project filesBoth were reported as inconsistent. Verified: timeouts, not genuine failures — and one shares the exact root cause.
anchors.page-seed.test.ts— same root cause.createSeedreaches the page synthesizer viaawait import('@object-ui/plugin-detail')(anchors.ts:207), so the cold transform is billed to the test body.unitrunsisolate: false, so whether the module is already warm is scheduling-dependent — which is precisely the inconsistency. Confirmed it can only be a timeout:createSeedswallows failures and returns{}, so a genuine synth failure would read asexpected false to be true, never observed.all-locales-key-parity.test.ts— not a load effect: a genuine quadratic. Line 64 calledkeysOf(builtInLocales[lang])inside the.filter()predicate, re-walking the whole locale tree once perenkey (~2.5k keys × a full recursive walk). The sibling assertion immediately below it always hoisted the call; this one didn't. Parity itself is clean — all 9 non-enpacks have 0 missing and 0 extra keys — so this was never a missing-translation failure.One more, found while verifying
plugin-kanban/src/index.test.tssurfaced during verification with the same root cause, and it is the anti-pattern's own cautionary tale:Already "fixed" once by raising the timeout — and it still timed out under full load (observed 15021ms, surfacing as 8 skipped tests rather than a failed assertion). The raised timeout is deleted, not re-raised.
Results
Timed portion of each file, isolated:
all-locales-key-parityplugin-kanban/indexanchors.page-seed+ i18n combinedSuite verification:
--project dom-heavy: failed 1 run in 4 before; green 20/20 runs after (14 on thebeforeAllform, 6 on the final static-import form).--project unit: all four files above pass. The 7 files that still fail are a pre-existing broken baseline with realAssertionErrors (spec-subschema-parity,onMutation,filter-operator-ast-parity×2,view-config-utils,view-operator-builder-parity,clientValidation.fieldRules) — unchanged by this PR and unrelated to timing.isolate: false.no-explicit-anywarnings).Test-only change, so no changeset per AGENTS.md.
The durable takeaway
If a test asserts on content behind a
React.lazy/ dynamic-import boundary, load that module at module scope. Otherwise the module loader competes with the assertion timeout, and the test's reliability depends on machine load rather than on the code it covers.🤖 Generated with Claude Code