test: retire four raised hook timeouts by importing at module scope - #3021
Merged
Conversation
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: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
github-actions
Bot
added
plugin
plugin: charts
plugin: editor
plugin: markdown
apps
tests
labels
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
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.
Follow-up to #3010 (the fix) and #3015 (the §9 rule). Sweeping for the same anti-pattern —
beforeAll(async () => { await import(…) }), which bills a cold module transform tohookTimeout— turned up 36 more test files, every single one already carrying a raised timeout, on an escalating ladder:That ladder is the finding. Nobody raised a timeout once and moved on — the budgets went 15s → 30s → 60s, which is what "raising the timeout" looks like when it isn't actually fixing anything. One of the 30s files even leaves the escalation as advice:
// Increase timeout to 30 seconds for heavy renderer imports.This PR takes the four highest-risk ones. Each drops its raised timeout rather than raising it again — the import phase is subject to no test or hook timeout.
The four
plugin-markdown/src/index.test.ts(was 15s) — the twin of the file fixed in test: fix five load-sensitive flaky tests by moving module loads out of timed windows #3010: same shape, same budget, and the same comment verbatim (// Increase timeout to 15 seconds for async import).plugin-kanbanproved that budget insufficient by blowing it at 15021ms, and this file's timed portion was already 5.83s of the same 15s. 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/src/__tests__/public-contract.test.ts(was 60s) — also folds thelet contract+ hook assignment into a module-scopeconst, now that the two side-effect imports are evaluated before the module body.On the console file
This was the delicate one and worth being explicit about. Its assertions pin the eager/lazy registration split (#2953), so the real question was whether moving the imports changes what is loaded when. It does not:
getPublicConfigs()at module scope sees exactly the registrations the hook used to produce;Its order-sensitive test — "reaches the lazily-registered blocks before their chunks load" — passes unchanged.
Results
Timed portion of each file, isolated:
plugin-markdownplugin-chartsplugin-editorpublic-contractFull suite is 100% green: 731 files passed / 1 skipped, 8521 tests passed / 24 skipped, zero failures. The
unitproject 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 runningisolate: false.Lint 0 errors (only pre-existing
no-explicit-anywarnings in untouched test bodies);type-check38/38 successful. Test-only change, so no changeset.One note for anyone baselining locally
The ~19 failures this repo can show on a long-lived worktree are a stale install, not a regression:
node_modules/@objectstack/specwas 14.6.0 against^17.0.0-rc.0declared inpackage.json.pnpm install --prefer-offline(9s) clears every one of them — which is why the run above is all-green where earlier local runs in this worktree showed 19 failures. Already documented in AGENTS.md §9; flagging it here because it is the single easiest way to misread this suite.Remaining
The 33-file
packages/componentscluster (all the sameawait import('../../../renderers')on 30s) is untouched here — mechanical, but a large diff that deserves its own PR. The durable fix is mechanical enforcement: the repo already has theeslint-rules/local-plugin infrastructure, and a rule flaggingawait import(...)insidebeforeAll/beforeEachin test files would stop this regrowing. 36 files and a 15s→30s→60s ladder are the evidence that a prose convention alone does not hold.🤖 Generated with Claude Code