7f07180c71
Some checks failed
- Explicitly states no lint rule is planned; CI guard is the backstop (addresses Elicit OQ-001 from PR #536 round 4) - Adds a "when to revisit" note: extract shared DynamicImportLoader<T> if 3+ components adopt the libLoader pattern (addresses Markus Keller round-4 observation on PR #536) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
91 lines
5.0 KiB
Markdown
91 lines
5.0 KiB
Markdown
# ADR 012 — Browser-Mode Test Mocking Strategy
|
||
|
||
**Status:** Accepted
|
||
**Date:** 2026-05-11
|
||
**Issue:** [#535 — Unit & Component Tests job exits 1 from vitest-browser teardown race](https://git.raddatz.cloud/marcel/familienarchiv/issues/535)
|
||
|
||
---
|
||
|
||
## 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.
|
||
|
||
---
|
||
|
||
## Residual exceptions
|
||
|
||
The following `vi.mock(module, factory)` calls in browser specs are **acceptable** because the mocked modules are loaded statically at spec module-eval time and cannot produce a teardown race:
|
||
|
||
| Module | Why it stays as vi.mock |
|
||
|--------|------------------------|
|
||
| `$app/navigation` | SvelteKit virtual module — no DI seam |
|
||
| `$app/forms` | SvelteKit virtual module — no DI seam |
|
||
| `$app/state` | SvelteKit virtual module — no DI seam |
|
||
| `$app/stores` | SvelteKit virtual module — no DI seam |
|
||
| `$env/static/public` | Vite env virtual module — no DI seam |
|
||
|
||
These modules are resolved at static import time (before any test runs). Their `vi.mock` factories are served by birpc synchronously during module graph resolution, not after worker teardown.
|
||
|
||
---
|
||
|
||
## 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:** No automated lint rule is planned; the CI coverage guard is the regression backstop. If a lint rule is added later (e.g. an ESLint rule flagging `vi.mock` of non-virtual modules in browser-mode spec files), update this ADR.
|
||
- **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.
|