diff --git a/docs/adr/012-browser-test-mocking-strategy.md b/docs/adr/012-browser-test-mocking-strategy.md index d916fc10..e3039bbe 100644 --- a/docs/adr/012-browser-test-mocking-strategy.md +++ b/docs/adr/012-browser-test-mocking-strategy.md @@ -1,8 +1,8 @@ # 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) +**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) --- @@ -65,22 +65,38 @@ For components that fetch data or call services, the `*.test-host.svelte` patter --- -## Residual exceptions +## Binding invariant: factory bodies must be synchronous (#553) -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: +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. -| 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 | +`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. -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. +**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 `