addf5c98db
Some checks failed
The previous revision allowed vi.mock for virtual modules on the "consumer import is static" argument. #553 proved that argument wrong: a statically- imported module with an async factory body whose dynamic import landed after teardown still produced the race. The factory body — not the consumer — is the failure surface. - Drop the "residual exceptions" table. - Add the binding invariant: factory bodies under `**/*.svelte.{test,spec}.ts` must be synchronous (no `await`, no `import(...)`). - Document the canonical vi.hoisted + getter pattern, with file references. - Record the $app/stores → $app/state architectural call (Markus's recommendation), removing one of the last two deprecated-import outliers. - Record the preload-data=off hardening (Tobias's recommendation) as a pattern note. - Update the Enforcement section to list all four defence layers (ESLint, CI grep, in-suite meta-test, CI birpc assert) and the coverage-flake- probe verification workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
114 lines
8.5 KiB
Markdown
114 lines
8.5 KiB
Markdown
# ADR 012 — Browser-Mode Test Mocking Strategy
|
||
|
||
**Status:** Accepted
|
||
**Date:** 2026-05-11 (revised 2026-05-12)
|
||
**Issues:** [#535 — original incident](https://git.raddatz.cloud/marcel/familienarchiv/issues/535) · [#553 — revision](https://git.raddatz.cloud/marcel/familienarchiv/issues/553)
|
||
|
||
---
|
||
|
||
## 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.
|
||
|
||
---
|
||
|
||
## 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):**
|
||
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.
|
||
- **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.
|