4db2e97490
CI proved cross-file sharing of a virtual-module mock body cannot work in
@vitest/browser-playwright 4.1.6: the static-import spread fails the hoist
("no top level variables"), and the await-vi.hoisted-import form fails to
parse ("Unexpected identifier 'vi'"). vi.hoisted has the same hoist
constraint as vi.mock, so there is no way to thread an external module's
body into the factory here.
Reverts Phase 1: restores the 4 $app/forms/$app/navigation specs to their
inline factories, inlines NotificationBell.spec's forms stub, deletes the
src/__mocks__/$app/* modules and the $mocks alias (vite, vitest-coverage,
kit). The no-factory-ban meta-test stays (no-factory vi.mock is still
banned). ADR-012 amended to record the infeasibility. Everything else
($app/state migration, confirm context-inject, notification refactor, the
pin, the meta-test) is unaffected. Part of #560.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
177 lines
16 KiB
Markdown
177 lines
16 KiB
Markdown
# ADR 012 — Browser-Mode Test Mocking Strategy
|
||
|
||
**Status:** Accepted
|
||
**Date:** 2026-05-11 (revised 2026-05-12, 2026-06-02)
|
||
**Issues:** [#535 — original incident](https://git.raddatz.cloud/marcel/familienarchiv/issues/535) · [#553 — revision](https://git.raddatz.cloud/marcel/familienarchiv/issues/553) · [#560 — shared-mock-body dedup](https://git.raddatz.cloud/marcel/familienarchiv/issues/560)
|
||
|
||
---
|
||
|
||
## Context
|
||
|
||
Vitest browser-mode tests (the `client` project, run with `@vitest/browser-playwright` / Chromium) use a different module resolution path than Node-environment tests. When a spec calls `vi.mock('some-module', factory)`, vitest registers a `ManualMockedModule`. At runtime, every time Chromium requests that module, a playwright route handler intercepts the request and calls the Node worker over **birpc** (`resolveManualMock`) to evaluate the factory and return the module body.
|
||
|
||
This is safe for modules that are imported **statically** at spec module-eval time (e.g. `$app/navigation`, `$env/static/public`): those requests resolve before the first test runs and well before any teardown occurs.
|
||
|
||
It is **unsafe** for modules that are imported **dynamically** (e.g. inside an `async onMount`, inside a lazy-loaded chunk): Chromium may fetch the module after the worker's birpc channel has already closed, producing:
|
||
|
||
```
|
||
Error: [birpc] rpc is closed, cannot call "resolveManualMock"
|
||
❯ ManualMockedModule.factory node_modules/@vitest/browser/dist/index.js:3221:34
|
||
```
|
||
|
||
This raises an unhandled rejection that exits the vitest process with code 1, even though every test in the run reported green.
|
||
|
||
`pdfjs-dist` and `pdfjs-dist/build/pdf.worker.min.mjs?url` are loaded via `await Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')])` inside `usePdfRenderer.svelte.ts::init()`, which is called from `onMount`. These dynamic imports triggered the race.
|
||
|
||
---
|
||
|
||
## Decision
|
||
|
||
**Prefer prop injection over `vi.mock(module, factory)` for any module that is loaded dynamically in browser-mode specs.**
|
||
|
||
### The libLoader pattern (for external rendering libraries)
|
||
|
||
When a component depends on a large external library loaded via dynamic import, extract the import into an injectable loader function with a production default:
|
||
|
||
```typescript
|
||
// usePdfRenderer.svelte.ts
|
||
type LibLoader = () => Promise<readonly [typeof import('pdfjs-dist'), { default: string }]>;
|
||
|
||
const defaultLibLoader: LibLoader = () =>
|
||
Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')]);
|
||
|
||
export function createPdfRenderer(libLoader: LibLoader = defaultLibLoader) { ... }
|
||
```
|
||
|
||
The component threads the loader as an optional prop:
|
||
|
||
```svelte
|
||
<!-- PdfViewer.svelte -->
|
||
let { url, ..., libLoader = undefined } = $props();
|
||
const renderer = untrack(() => createPdfRenderer(libLoader));
|
||
```
|
||
|
||
Tests supply a synchronous fake — no `vi.mock` needed:
|
||
|
||
```typescript
|
||
const fakePdfjs = { GlobalWorkerOptions: ..., getDocument: vi.fn(), TextLayer: class {} };
|
||
const fakeLoader = vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
|
||
render(PdfViewer, { url: '...', libLoader: fakeLoader });
|
||
```
|
||
|
||
### The test-host pattern (for component behaviour)
|
||
|
||
For components that fetch data or call services, the `*.test-host.svelte` pattern threads the dependency as a prop rather than mocking the module. See `PersonMentionEditor.test-host.svelte` for the canonical example.
|
||
|
||
---
|
||
|
||
## Binding invariant: factory bodies must be synchronous (#553)
|
||
|
||
The original revision of this ADR allowed `vi.mock(virtualModule, factory)` for SvelteKit/Vite virtual modules on the argument that their consumer imports were resolved at static-import time. **That reasoning is wrong.** What matters is what the **factory body** does, not where the mocked module is consumed.
|
||
|
||
`EnrichmentBlock.svelte.spec.ts` (issue #553) was statically imported and still produced the race: its `vi.mock('$app/stores', async () => { const mod = await import(...); return mod; })` factory performed a dynamic import in its body, and that body was invoked asynchronously when Chromium fetched the manually-mocked module — sometimes after the worker's birpc channel had already closed.
|
||
|
||
**Therefore: under `**/\*.svelte.{test,spec}.ts`, every `vi.mock`factory body must be synchronous. No`await`, no `import(...)`.\*\*
|
||
|
||
If a factory needs to share state with the spec (a mutable ref, a `vi.fn`, a writable store), use `vi.hoisted()` to lift the reference above `vi.mock`'s implicit hoist:
|
||
|
||
```ts
|
||
const { mockNavigating } = vi.hoisted(() => ({
|
||
mockNavigating: { type: null as string | null },
|
||
}));
|
||
|
||
vi.mock("$app/state", () => ({
|
||
get navigating() {
|
||
return mockNavigating;
|
||
},
|
||
}));
|
||
```
|
||
|
||
The getter defers the read until consumption time; `vi.hoisted` guarantees the reference is initialised before the (also hoisted) `vi.mock` factory runs. See `DropZone.svelte.spec.ts:9`, `NotificationBell.svelte.spec.ts:6-10`, and `EnrichmentBlock.svelte.spec.ts` for canonical examples.
|
||
|
||
### Architectural follow-on: prefer `$app/state` over `$app/stores`
|
||
|
||
`$app/stores` is the deprecated subscription-based store API; `$app/state` is the modern reactive proxy. New components should import from `$app/state`. As part of #553 we migrated `EnrichmentBlock.svelte` from `$app/stores.navigating` to `$app/state.navigating` with `!!navigating.type` — matching the pattern already established in `routes/aktivitaeten/+page.svelte:117` and `routes/documents/+page.svelte:261`. Migration eliminated the _need_ to mock a store at all in that spec.
|
||
|
||
**Pattern note:** When an overlay or dropdown triggers a navigation action, use `<button type="button">` with an `onclick` handler that calls `goto(path)` — do **not** use `<a href="…">` with `e.preventDefault()`. SvelteKit registers its link interceptor as a capture-phase `document` listener, so it fires before the component's bubble-phase `onclick`. By the time `e.preventDefault()` runs the router has already initiated navigation, which tears down the vitest-browser Playwright orchestrator iframe. A `<button>` carries no `href`, so the capture-phase interceptor never fires. See `NotificationDropdown.svelte` for the canonical example.
|
||
|
||
**Pattern note (#553):** Browser-mode tests run with `data-sveltekit-preload-data="off"` (set in `src/test-setup.ts` via the client project's `setupFiles`). Hover-prefetch otherwise fires real fetch requests for route loader chunks; those requests go through the same Playwright route handler that serves mocked modules. An in-flight prefetch landing after iframe teardown can hit the handler with a closed birpc channel, raising an unhandled rejection.
|
||
|
||
---
|
||
|
||
## 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.6.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.6.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 (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.6.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<T>` type to avoid parallel type definitions across modules.
|
||
|
||
---
|
||
|
||
## Revision 2026-06-02 (#560 — shared mock bodies, no-factory ban)
|
||
|
||
### No-factory `vi.mock` of a virtual module is forbidden
|
||
|
||
PR #657 attempted to delete `vi.mock` factories entirely and rely on Vitest auto-resolving a bare `vi.mock('$app/navigation')` to an adjacent `src/__mocks__/$app/navigation.ts`, the way Jest's `__mocks__/` directory works. **This is empirically false for SvelteKit virtual modules in browser-mode Vitest.** A no-factory `vi.mock(virtualModule)` substitutes _some_ exports (plain function references like `goto`) but leaves others bound to the live implementation — notably `replaceState`, which SvelteKit re-exports through a getter delegating to the live router. CI #1857 failed on `admin/tags/[id]` with `Cannot call replaceState(...) before router is initialized`, raised from a `$effect`. A partial auto-mock is therefore unsafe.
|
||
|
||
**Rule:** under `**/*.svelte.{spec,test}.ts`, a `vi.mock` of a virtual module must always pass a factory. The factory body must still be synchronous (the original binding invariant above). Enforced by a seventh layer:
|
||
|
||
7. **In-suite no-factory-ban meta-test** at `frontend/src/__meta__/no-factory-ban.test.ts` — same source-scan mechanism as the other meta-tests; fails if any browser spec contains a `vi.mock('mod')` with no second argument.
|
||
|
||
### Cross-file sharing of a virtual-module mock body is infeasible (the third false premise)
|
||
|
||
The original #560 plan ("Option A") proposed deduplicating the non-trivial interceptor factories by importing a shared body from `src/__mocks__/` into a sync factory:
|
||
|
||
```ts
|
||
import * as formsMock from "$mocks/$app/forms";
|
||
vi.mock("$app/forms", () => ({ ...formsMock }));
|
||
```
|
||
|
||
**CI proved this does not work in `@vitest/browser-playwright` 4.1.6**, across two runs:
|
||
|
||
1. The static-import form above fails at runtime — vitest hoists `vi.mock` _above_ the import, so the factory references an uninitialised binding: `vi.mock factory: make sure there are no top level variables inside, since this call is hoisted`.
|
||
2. The documented escape, loading the body through an async hoisted import, fails to even parse in browser mode — vitest's hoist transform mangles it: `SyntaxError: Unexpected identifier 'vi'`.
|
||
|
||
```ts
|
||
const formsMock = await vi.hoisted(() => import("$mocks/$app/forms")); // parse error in browser mode
|
||
```
|
||
|
||
`vi.hoisted` has the _same_ constraint as `vi.mock` (its factory can't reference top-level imports either, since it too is hoisted above them), so there is no way to get an external module's body into the hoisted context here. **Therefore: do not share virtual-module mock bodies across spec files. Define each `vi.mock` factory inline, with a synchronous body.** Duplicating the handful of interceptor factories is the accepted cost — it is the only pattern that works. The `src/__mocks__/$app/*` modules and the `$mocks` alias added for Option A were removed. (Revisit on a newer `@vitest/browser-playwright` whose hoist transform handles async `vi.hoisted` imports.)
|
||
|
||
The no-factory-ban above still stands: every `vi.mock` of a virtual module must pass an _inline_ sync factory — never no factory, never a spread of an imported binding.
|
||
|
||
### Rejected: Option C (config-level auto-resolve)
|
||
|
||
Re-enabling implicit `__mocks__/` auto-resolution through a Vitest config flag or a `setupFiles` shim was rejected. It trades auditability for cosmetics: the mock binding becomes a hidden default invisible at the call site, and its failure mode (a partial mock) is the hardest to debug — exactly the PR #657 class. The no-factory-ban meta-test deliberately keeps the door closed.
|
||
|
||
### Patch pin
|
||
|
||
`@vitest/browser-playwright` is exact-pinned (no caret) to `4.1.6` in `package.json` so `patches/@vitest+browser-playwright+4.1.6.patch` keeps applying; a caret range could float onto a version the patch rejects. Pin and patch are both removed once the library ships a release containing [PR #10267](https://github.com/vitest-dev/vitest/pull/10267).
|