From 2e6cc346abffd99a9dc5ddac30926d1b248120f0 Mon Sep 17 00:00:00 2001 From: Marcel Date: Wed, 13 May 2026 08:09:57 +0200 Subject: [PATCH] docs(adr-012): document duplicate-id hazard and patch-package backport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a second binding invariant section to ADR-012 covering the duplicate-id mechanism named in #553's follow-up investigation: same resolved module URL referenced via two distinct vi.mock id strings → @vitest/browser-playwright leaks an orphan Playwright route → birpc-closed crash in the next session. Records the rule (one canonical id per mocked module, prefer the spelling production uses, no-extension for .svelte rune modules), the in-suite detector (no-duplicate-mock-ids.test.ts), and the patch-package backport of vitest PR #10267 with its removal trigger. Extends the existing Consequences enforcement list from four layers to six, adding the duplicate-id detector and the patch-package layer. Refs: #553 · vitest-dev/vitest#9957 · vitest-dev/vitest#10267 Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/adr/012-browser-test-mocking-strategy.md | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/docs/adr/012-browser-test-mocking-strategy.md b/docs/adr/012-browser-test-mocking-strategy.md index e3039bbe..8b0ac788 100644 --- a/docs/adr/012-browser-test-mocking-strategy.md +++ b/docs/adr/012-browser-test-mocking-strategy.md @@ -99,15 +99,36 @@ The getter defers the read until consumption time; `vi.hoisted` guarantees the r --- +## Binding invariant: one canonical ID per mocked module (#553 — duplicate-id hazard) + +The sync-factory invariant above closes one named trigger of the `[birpc] rpc is closed` race. Investigation of a follow-up flake revealed a second, independent trigger: **the same resolved module URL mocked under two distinct ID strings** across or within spec files. + +`@vitest/browser-playwright` registers a Playwright `page.context().route(...)` handler per `vi.mock` call. The predicate matches on the module's resolved URL. When two `vi.mock` calls reference the same module under different IDs — for example `'$lib/foo.svelte'` and `'$lib/foo.svelte.js'` (both resolve to the same Svelte rune-module URL) — the registry stores both predicates but the cleanup map only tracks the latest. The orphan route survives session teardown. When the next session loads the same module, the orphan fires, calls `await module.resolve()` against a closed birpc channel, and crashes the run. + +This is fixed upstream in [vitest PR #10267](https://github.com/vitest-dev/vitest/pull/10267) (issue [#9957](https://github.com/vitest-dev/vitest/issues/9957)). Until that fix reaches a published `@vitest/browser-playwright` release, we close the gap from two sides: + +**The rule.** Every mocked module must be referenced under exactly one ID string across the entire client test suite. Pick the spelling production code uses. For Svelte 5 rune modules (`*.svelte.ts`), the canonical form is the no-extension import (`'$lib/foo.svelte'`) — matches the source file basename and matches Svelte 5 convention. Never mix `.svelte.js` and `.svelte` for the same module across specs. + +**Enforcement layers** (added in #553's second cycle, extending the four-layer chain above): + +5. **In-suite meta-test** at `frontend/src/__meta__/no-duplicate-mock-ids.test.ts` globs `src/**/*.svelte.{test,spec}.ts`, extracts every `vi.mock` first-arg string, canonicalises by stripping a trailing `.js`/`.ts` after `.svelte`, and fails if any canonical ID is referenced under two or more distinct spellings. Same shape as `no-async-mock-factories.test.ts`. +6. **`patch-package` backport** of PR #10267 at `frontend/patches/@vitest+browser-playwright+4.1.0.patch`. Applied automatically by the `postinstall` hook. Closes the race at the route-handler level — even if a contributor reintroduces a duplicate-ID, the patched `register` handler unroutes the existing predicate before installing the new one. + +**When to remove the patch.** Once `@vitest/browser-playwright` ships a release containing PR #10267, delete `patches/@vitest+browser-playwright+4.1.0.patch`. Bump the dependency to the version containing the fix. The in-suite meta-test stays — it's a cheap permanent guard against the contributor-facing pattern, independent of upstream library version. + +--- + ## Consequences - New browser-mode specs that need to stub an external library **must not** use `vi.mock(externalLib, factory)`. Add a loader/factory parameter to the underlying hook or service instead. - The CI `unit-tests` job includes a permanent grep guard that fails the build if `rpc is closed` appears in any coverage run log. This catches regressions before they reach the acceptance criterion. - Acceptance criterion for #535: 60 consecutive green `workflow_dispatch` CI runs against `main` after the fix is merged, with zero `rpc is closed` lines in any log. -- **Enforcement (four layers, defence in depth):** +- **Enforcement (six layers, defence in depth):** 1. **ESLint `no-restricted-syntax`** in `eslint.config.js` (scoped to `**/*.{spec,test}.ts`) flags two patterns: (a) the literal `vi.mock('pdfjs-dist', ...)` — enforces the libLoader pattern — and (b) any `vi.mock(..., async () => { ... await import(...) ... })` — enforces the synchronous-factory invariant. Both messages point at this ADR. Failure surfaces at save time. 2. **CI grep guard** in `.gitea/workflows/ci.yml` runs before the test suite launches. Mirrors the ESLint patterns with `grep -Pzn`. ~10s round-trip. 3. **In-suite meta-test** at `frontend/src/__meta__/no-async-mock-factories.test.ts` globs `src/**/*.svelte.{test,spec}.ts` and asserts none match the banned pattern. Catches at every vitest invocation — the layer hardest to disable. 4. **CI birpc assert** runs after the coverage step and fails the build if `[birpc] rpc is closed` appears in any log line. Catches the symptom even if all the upstream layers were bypassed. + 5. **In-suite duplicate-ID meta-test** at `frontend/src/__meta__/no-duplicate-mock-ids.test.ts` enforces the one-canonical-ID-per-module rule from the duplicate-id-hazard section above. + 6. **`patch-package` backport** at `frontend/patches/@vitest+browser-playwright+4.1.0.patch` closes the upstream race itself, applied via `postinstall`. To be removed when `@vitest/browser-playwright` releases [vitest PR #10267](https://github.com/vitest-dev/vitest/pull/10267). - **Acceptance verification:** `coverage-flake-probe.yml` is a `workflow_dispatch`-triggered matrix workflow that runs the coverage suite 20× in parallel against a single SHA and asserts zero birpc lines. One fire, parallel cost, deterministic signal — replaces accumulating 20 sequential push events. - **When to revisit the LibLoader home:** If three or more components adopt this pattern, consider extracting a shared `$lib/types/lib-loader.ts` or a generic `DynamicImportLoader` type to avoid parallel type definitions across modules.