Skip to content

Commit 54735f9

Browse files
authored
test(sdk): stop re-transforming built sibling packages, instead of warming the cost (#2946)
* test(sdk): warm every lazy namespace import once, instead of per file The SDK loads five namespace implementations with a dynamic import() on first use, so whichever test runs first pays the whole cold-import cost inside its own 5000ms budget. That has now been fixed three times, once per namespace, each time by warming that one file: ids.test.ts (3a00b5e), list.test.ts (#2935, after five unrelated PRs failed at once between 5005ms and 5056ms and main went red on it), and parquet-geometry.test.ts (#2248) for the same shape elsewhere. Replace the per-file patches with one setup file covering all six lazily-imported packages, so drawing, bcf and sandbox cannot become the fourth instance. Measured, warm cache, running list.test.ts and ids.test.ts together: without setup cold tests 469ms and 491ms, setup 0ms, tests 968ms with setup cold tests 2ms and 2ms, setup 1.20s, tests 8ms The cost moves out of every test's budget rather than being absorbed by a larger one. Deliberately NOT raising testTimeout, which is how packages/data handles its own cold-transform cost: a blanket raise buys the same green at the price of every genuine hang in this package taking the new timeout to report. allSettled rather than all, because @ifc-lite/sandbox is deliberately not a dependency of the SDK and its import rejects here exactly as it does for a consumer who has not installed it. The warm-up is an optimisation; every test still passes without it, just closer to the timeout. * test(sdk): make the warm-up list check itself against the source Review of the previous commit found the arrangement had no way to notice its own drift. The list of lazily-imported packages is maintained by hand against src/namespaces, so a seventh dynamic import added later gets no warm-up and no signal -- the failure surfaces months on, as a flake under CI load, in whichever unrelated test happened to run first. An un-warmed package is indistinguishable from a warmed one until that happens. Add a test that reads both sides as text and compares them, in both directions: every specifier src imports lazily must be warmed, and every entry in the list must still be imported lazily. Deliberately not by importing vitest.setup.ts, which would execute the warm-up and would let a rename keep the test green while the list no longer matched src. It carries its own control: if the loader idiom is ever refactored away, the scan finds nothing and both comparisons pass vacuously against two empty sets, so a third assertion fails loudly instead. Mutation-verified, each applied from a snapshot and asserted to have landed before the run: new lazy import in src, not added to the list -> FAILS package removed from the list -> FAILS stale entry left in the list -> FAILS Also from review: repoint ids.test.ts's comment, which still described a warm-up that this change had deleted from that file; state in the setup docblock that the ten files touching none of these packages now pay for it too (types.test.ts 76ms -> 680ms, full suite at --maxWorkers=2 1.61s -> 2.26s), since the tradeoff against testTimeout is not honest without it; and record that @ifc-lite/sandbox can never resolve here because it depends on @ifc-lite/sdk, so its rejection is inert by construction rather than a missing dependency somebody should fix. * test(sdk): make the warm-up drift check a lint, not a test The drift check landed as packages/sdk/src/namespaces/lazy-imports.test.ts, which reads two source files and compares their text. The repo bans that shape and gates on it: AGENTS.md "Never assert on a source file's text", enforced by scripts/check-source-text-assertions.mjs at test.yml:415. Run against the previous commit it exits 1, so the branch was red as it stood. The gate is right. A claim that one file's list matches another file's declarations can only be made by reading both sources, and that is exactly the shape that certifies a string exists rather than that the code works. check-clash-degenerate-reason-parity.mjs was moved out of a test file for the same reason and its header says so; this follows it, including the --root flag and the vacuity guard. Two things the move buys beyond satisfying the gate: - comments are now stripped from both sides symmetrically, so a package named only in prose cannot stand in for a real one. Both files discuss these package names at length, so that was a live hole. - the checker gets a regression harness, which is how the sibling checks prove they can fire. Seven cases against mutated copies of the real sources in a temp tree, each mutation anchor asserted present in the real input first so a drifted anchor fails loudly instead of quietly testing nothing: unwarmed new import, package dropped, stale entry, loader idiom refactored away (the vacuity mode -- two empty sets "agree"), and a package named only in a comment. Wired as two steps of the CI node-test job, matching the sibling pair. * test(sdk): anchor the warm-up scan on the array, trim the restated prose From cleanup review, each item verified rather than taken on the claim: - warmedPackages anchored on the array literal via one regex instead of indexOf plus a fragile indexOf('];'). The reviewer tested the naive whole-file alternative and it is wrong: a straight-quoted specifier in prose satisfies it, and a renamed constant leaves it returning all six while the list it names no longer exists. Anchoring makes a rename yield the empty set, so every package reads as unwarmed and it fails loudly. Pinned as an eighth harness case. - the cold-import justification was written five times across four files. It now lives once, in vitest.setup.ts. vitest.config.ts and ids.test.ts point at it, and the checker header keeps only what is specific to checking rather than to warming. - the three-fix history compressed to its evidence (3a00b5e, #2935, #2248). "This replaces the per-file patches" was already said by the diff deleting them. Kept, against the finding: the `import(` guard in the source scan is inert on today's tree. It states the contract that only DYNAMIC imports cost anything to warm, so a future static-import binding is not demanded here. The comment now says it is inert and why it stays. * test(sdk): stop re-transforming built sibling packages The cold-import cost this branch was warming is not module loading. pnpm links @ifc-lite/* as symlinks, so the specifier resolves to packages/<name>/dist/index.js -- a real path inside the project root -- and vite treats that built file as SOURCE and runs it through its SSR transform on every run. Node imports the same files in 12-40ms. It surfaced as a timeout rather than as slowness because the namespaces load lazily, so the transform landed inside the 5000ms budget of whichever test touched one first. One config line removes it. Full sdk suite, --maxWorkers=2, 3 reps, 173/173 green in every column: before 1.60s (transform 1.38s) warming the imports 2.28s (transform 3.41s, setup 2.50s) externalising built dist 0.61s (transform 129ms) The two tests that flaked land at 29ms and 38ms with nothing warmed at all. So this deletes the mechanism the earlier commits on this branch built: vitest.setup.ts, the hand-maintained package list, the lint that checked the list against the source, and that lint's regression harness. The warm-up was a correct fix one level above the cost, and it made the suite slower than doing nothing. Verified rather than assumed, because this changes how every sdk test resolves its dependencies: same dist file either way, so nothing about what is under test changes; vi.mock('@ifc-lite/sandbox') still applies (src/sandbox.test.ts, 4/4); and the siblings must be built, which turbo test already guarantees via dependsOn ["^build"]. The same transform tax is paid by every package in this monorepo that imports a sibling. Filed separately rather than widened into here. * test(sdk): anchor the external pattern, cover pkg/, assert it took effect Review findings on the previous commit, each checked against the tree rather than taken on the claim: - the pattern was an unanchored substring, so it would also match a checkout living beneath some other packages/<x>/dist/ -- and then it matches this package's own src/*.ts, which Node cannot execute. Now anchored at the repo root. - it covered dist/ only, missing @ifc-lite/wasm, which builds to packages/wasm/pkg/ifc-lite.js (turbo build outputs are dist/** and pkg/**) and is reachable here via @ifc-lite/clash/wasm. A large generated bundle is exactly what must not be re-transformed inside a lazy import's budget. - the comment cited turbo's test task as dependsOn ["^build"]. It is ["build"], with build itself ["^build"], so the guarantee holds transitively but the citation was wrong. AGENTS.md line 13 carries the same inaccuracy; not touched here. - "nothing about what is under test changes" was false in one respect: externalised modules are native ESM, so vi.spyOn on a sibling's namespace now throws "Module namespace is not configurable". vi.mock is unaffected. Nothing spies that way today; the comment now names it rather than overstating. And the finding that mattered most: nothing made a regression visible. The warm-ups were self-evidencing, sitting in the file they protected; a config is action-at-a-distance, and if it stops applying the tests do not fail -- they quietly go back to a ~2s transform inside a 5s budget and the flake returns on somebody else's PR. So it is asserted, on the shape of the module namespace rather than on a timing that would be flaky by construction: a natively imported ES module namespace is sealed, a vite-built one is an ordinary object. Measured both ways -- configurable/extensible false with the rule, true without. Mutation-verified: external rule deleted -> FAILS deps.inline overrides it (path-shaped) -> FAILS deps.inline: [/@ifc-lite/] -> passes, correctly: inline matches resolved paths too, so that pattern overrides nothing. The mutation never applied; it is not a surviving guard. * test(sdk): cover every heavy sibling, not the lightest one The guard added in the previous commit sampled @ifc-lite/lists alone. That is the SMALLEST of the eight packages externalised here at 168K; the cost is @ifc-lite/data at 9.8M and @ifc-lite/parser at 5.5M. Adding `inline: [/packages\/(data|parser)\/dist\//]` -- precisely the "later inline rule added for an unrelated reason" the guard's own comment names as the threat -- left it green while suite transform went 120ms to 944ms. The safety net had a hole in the exact shape of the threat it documents, which is the failure mode it exists to prevent. Now it.each over the five that matter. Re-run against the same mutation: 2 tests fail, transform 173ms -> 1.23s, so the condition is observable where before it was not. Two more from the same review: - the discriminator is stated as "today's vite" rather than as a definition. Measured, the ONLY differing bit between a native and a transformed namespace is sealed-ness; proto and Symbol.toStringTag are identical in both. So if vite ever seals its namespaces this goes green while inlining, and per-package coverage is the real belt. - repoRoot is normalised to forward slashes. On win32 fileURLToPath yields C:\repo\ while the pattern tail and the ids vitest matches against use /, so the regex could never match and externalisation would silently vanish. The `pkg` half of (dist|pkg) is now labelled UNTESTED insurance rather than left to read as covered: instrumenting the pattern over the whole suite shows eight ids submitted, every one a dist/index.js, none under pkg. It stays for @ifc-lite/wasm, which is reachable via @ifc-lite/clash/wasm and is exactly the kind of large generated bundle that must not be re-transformed inside a lazy import's budget.
1 parent 989ee2c commit 54735f9

4 files changed

Lines changed: 148 additions & 50 deletions

File tree

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
4+
5+
import { expect, it } from 'vitest';
6+
7+
/**
8+
* `vitest.config.ts` stops vite re-transforming sibling packages' built output,
9+
* which is what kept this package's lazily-imported namespaces inside a 5000ms
10+
* test budget (#2935).
11+
*
12+
* That protection is action-at-a-distance: it lives in the config, not in the
13+
* files it protects, and if it ever stops taking effect the tests do not fail
14+
* -- they silently go back to paying a ~2s transform inside a 5s budget, and
15+
* the flake returns on somebody else's PR. Absence of the protection looks
16+
* exactly like success, which is the whole reason it is asserted here.
17+
*
18+
* One concrete way it could silently stop: vitest checks `deps.inline` BEFORE
19+
* `deps.external`, so a later inline rule added for an unrelated reason
20+
* overrides the pattern with no error at all. Verified: adding
21+
* `inline: [/packages\/[^/]+\/dist\//]` turns this test red. Note it has to be
22+
* path-shaped to do so -- `inline: [/@ifc-lite/]` changes nothing, because
23+
* inline matches resolved paths too, which is the same trap as `external`.
24+
*
25+
* Asserted on the shape of the module namespace rather than on a timing, which
26+
* would be flaky by construction. Under today's vite, a natively imported ES
27+
* module namespace is sealed and one built by the SSR transform is not; every
28+
* other distinguishing bit is identical (`proto === null`,
29+
* `Symbol.toStringTag === 'Module'` in both). That makes sealed-ness the best
30+
* cheap discriminator available rather than a definition -- if vite ever seals
31+
* its namespaces this goes green while inlining, so the per-package coverage
32+
* below is the real belt.
33+
*
34+
* Every heavy sibling is checked, not a sample. An earlier version asserted
35+
* only `@ifc-lite/lists`, which is the SMALLEST of the eight packages
36+
* externalised here at 168K; the cost is `@ifc-lite/data` at 9.8M and
37+
* `@ifc-lite/parser` at 5.5M. Inlining just those two -- precisely the "later
38+
* inline rule added for an unrelated reason" named above -- left the guard
39+
* green while suite transform went 120ms to 944ms. The safety net had a hole
40+
* in the exact shape of the threat it documents.
41+
*/
42+
it.each([
43+
'@ifc-lite/data',
44+
'@ifc-lite/parser',
45+
'@ifc-lite/lists',
46+
'@ifc-lite/clash',
47+
'@ifc-lite/ids',
48+
])('imports %s natively, not through vite (#2935)', async (specifier) => {
49+
const namespace: Record<string, unknown> = await import(/* @vite-ignore */ specifier);
50+
const someExport = Object.keys(namespace)[0];
51+
expect(someExport, `${specifier} must export something to inspect`).toBeTruthy();
52+
53+
const descriptor = Object.getOwnPropertyDescriptor(namespace, someExport);
54+
expect(
55+
{ configurable: descriptor?.configurable, extensible: Object.isExtensible(namespace) },
56+
`${specifier} came through vite's transform: its namespace is configurable and ` +
57+
'extensible, where a native ESM one is neither. server.deps.external in ' +
58+
'vitest.config.ts stopped applying to it, and the cold-transform flake is back',
59+
).toEqual({ configurable: false, extensible: false });
60+
});

packages/sdk/src/namespaces/ids.test.ts

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,10 @@ import type {
1313

1414
const ids = new IDSNamespace();
1515

16-
// `IDSNamespace` dynamically imports `@ifc-lite/ids` on first use (see
17-
// `loadIDS` in ids.ts) so that SDK consumers who never touch `bim.ids`
18-
// don't pay for it. That's the right tradeoff for real callers, who load
19-
// the module once and then reuse it — but it means whichever test in this
20-
// file happens to be the first to call `validate`/`parse`/etc. pays the
21-
// one-time cold-import cost inside its own timer. Under CI's parallel test
22-
// load that cost alone can approach the default 5s test timeout. Warm the
23-
// import here, during module collection (unbounded by any per-test or
24-
// per-hook timeout), so the locale test below only times the logic it
25-
// exists to check.
26-
await import('@ifc-lite/ids');
16+
// `@ifc-lite/ids` is imported lazily by `loadIDS`. It used to be warmed here
17+
// so the locale test below did not pay the cold import inside its own budget;
18+
// that cost was vite re-transforming built sibling output, and it is gone --
19+
// see `vitest.config.ts`.
2720

2821
const sv = (value: string): IDSSimpleValue => ({ type: 'simpleValue', value });
2922

packages/sdk/src/namespaces/list.test.ts

Lines changed: 1 addition & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -19,47 +19,9 @@
1919
* of undefined (reading 'length')` instead of running unfiltered.
2020
*/
2121

22-
import { beforeAll, describe, expect, it } from 'vitest';
22+
import { describe, expect, it } from 'vitest';
2323
import { ListNamespace, type ListDefinition } from './list.js';
2424

25-
/**
26-
* `ListNamespace.execute()` dynamically imports `@ifc-lite/lists` and
27-
* `@ifc-lite/data` on first use (`loadLists` in `list.ts`) so SDK consumers
28-
* who never touch `bim.list` don't pay for them. Right tradeoff for real
29-
* callers -- but it means whichever test here runs first pays the one-time
30-
* cold-import cost inside its own timer: measured at 2002ms cold and 481ms on
31-
* a warm repo, against 2-3ms once the modules are resolved.
32-
*
33-
* Under CI's parallel load that alone crosses the 5000ms default. This file
34-
* failed on FIVE unrelated PRs at once -- #2822, #2905, #2907, #2923, #2930 --
35-
* every one between 5005ms and 5056ms, while passing locally on the same
36-
* commit. Warming here moves the cost outside every `it()`'s budget, so each
37-
* test times only its own logic and keeps the tight default: a genuine hang
38-
* still fails in 5s rather than 30s.
39-
*
40-
* Same root cause and directory as `ids.test.ts` (3a00b5e64, which reported
41-
* 5021-5038ms), and the same shape as `packages/export/src/parquet-geometry.test.ts`
42-
* (#2248). `bcf.ts`, `drawing.ts` and `sandbox.ts` share the lazy-import idiom
43-
* and have not fired yet.
44-
*
45-
* Warmed rather than mocked: these tests exist to prove the real bridge to the
46-
* real library produces real values, and a mocked `executeList` would pass
47-
* with the very column-mapping bug they were written to catch (#2841).
48-
*
49-
* Known limitation, and the alternative was tried rather than assumed: this
50-
* names `execute()`'s imports instead of driving `execute()` itself, so a
51-
* third dependency added there would silently stop being covered. Driving the
52-
* real method would stay correct by construction, but every module-scope
53-
* `await` form breaks function hoisting in this file under vite's transform
54-
* (`makeProviderWithPsets` hits `ReferenceError: makeProvider is not
55-
* defined`), which is why the warm-up sits in `beforeAll`. If `execute()`
56-
* gains an import, add it here.
57-
*/
58-
beforeAll(async () => {
59-
await import('@ifc-lite/lists');
60-
await import('@ifc-lite/data');
61-
}, 30_000);
62-
6325
interface Provider {
6426
getEntitiesByType: (type: number) => number[];
6527
getAllEntityIds?: () => number[];

packages/sdk/vitest.config.ts

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
4+
5+
import { fileURLToPath } from 'node:url';
6+
import { defineConfig } from 'vitest/config';
7+
8+
// Normalised to forward slashes: on win32 `fileURLToPath` yields `C:\repo\`,
9+
// while the pattern tail and the ids vitest tests against both use `/`, so the
10+
// regex would never match and externalisation would silently vanish.
11+
const repoRoot = fileURLToPath(new URL('../../', import.meta.url)).replaceAll('\\', '/');
12+
13+
/**
14+
* Sibling workspace packages, matched by their BUILT output.
15+
*
16+
* Anchored at the repo root rather than left as a floating substring: vitest
17+
* tests this against a resolved absolute path, so an unanchored
18+
* `packages/<x>/dist/` would also match a checkout that happens to live
19+
* beneath some other `packages/<x>/dist/` directory -- and then it matches
20+
* this package's own `src/*.ts` too, which Node cannot execute.
21+
*
22+
* `pkg` as well as `dist` is UNTESTED insurance, stated plainly: instrumenting
23+
* the pattern over the full suite shows eight ids submitted, every one of them
24+
* a `packages/<name>/dist/index.js`, and none under `pkg`. It is here because
25+
* `@ifc-lite/wasm` builds to
26+
* `packages/wasm/pkg/ifc-lite.js` (turbo's build outputs are
27+
* `["dist/**", "pkg/**"]`). It is reachable from here through
28+
* `@ifc-lite/clash/wasm`, and it is a large generated bundle -- exactly the
29+
* thing that must not be re-transformed inside a lazy import's budget.
30+
*/
31+
const BUILT_SIBLING = new RegExp(
32+
`^${repoRoot.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}packages/[^/]+/(dist|pkg)/`,
33+
);
34+
35+
export default defineConfig({
36+
test: {
37+
environment: 'node',
38+
// Load sibling workspace packages as the built JS they already are,
39+
// instead of re-transforming them.
40+
//
41+
// pnpm links `@ifc-lite/*` as symlinks, so the specifier resolves to
42+
// `packages/<name>/dist/index.js` -- a real path inside the project root.
43+
// Vite therefore treats that built file as SOURCE and runs it through its
44+
// SSR transform on every run. That transform, not module loading, is the
45+
// cost: Node imports those same files in 12-40ms.
46+
//
47+
// It surfaced as a timeout rather than as slowness because the namespaces
48+
// load their implementations with a dynamic `import()` on first use, so the
49+
// whole transform landed inside the 5000ms budget of whichever test touched
50+
// one first. Measured at 2002ms, it crossed the limit under CI load and took
51+
// `main` red on 19 of 20 runs (#2935), after being patched one test file at
52+
// a time three times before that (3a00b5e64, #2248).
53+
//
54+
// Full sdk suite, `--maxWorkers=2`, 3 reps, 173/173 green in every column:
55+
//
56+
// before 1.60s (transform 1.38s)
57+
// warming the imports instead 2.28s (transform 3.41s, setup 2.50s)
58+
// this 0.61s (transform 129ms)
59+
//
60+
// The two tests that flaked go to 29ms and 38ms with nothing warmed.
61+
//
62+
// Resolution is unchanged -- the import already resolved to `dist`; only the
63+
// transform is removed, and every sibling `exports` map in this closure is a
64+
// plain `{types, import, default}` with no condition that would pick a
65+
// different file. It does mean the siblings must be BUILT: `turbo.json` gives
66+
// `test` a `dependsOn: ["build"]`, and `build` in turn `["^build"]`, so turbo
67+
// covers it transitively. Running `vitest` package-locally does not, but that
68+
// was already true -- the import resolved to `dist` before this too.
69+
//
70+
// ONE REAL BEHAVIOUR CHANGE: externalised modules are native ESM, so their
71+
// namespace objects are non-configurable and `vi.spyOn(await
72+
// import('@ifc-lite/...'), 'someExport')` now throws "Module namespace is not
73+
// configurable". `vi.mock` is unaffected and still wins over externalisation,
74+
// for both static and dynamic imports. Nothing in the suite spies that way
75+
// today; if you need to, use `vi.mock` with a factory.
76+
//
77+
// Written as a path pattern on purpose: `external: [/@ifc-lite\//]` does NOT
78+
// work, because `external` matches resolved paths rather than specifiers, and
79+
// pnpm realpaths the symlink so the id contains neither the specifier nor
80+
// `/node_modules/`.
81+
server: { deps: { external: [BUILT_SIBLING] } },
82+
},
83+
});

0 commit comments

Comments
 (0)