diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 0e70c3e0..0b27866c 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -37,17 +37,34 @@ jobs: working-directory: frontend - name: Run unit and component tests with coverage - run: npm run test:coverage + shell: bash + run: | + set -eo pipefail + npm run test:coverage 2>&1 | tee /tmp/coverage-test-${{ github.run_id }}.log working-directory: frontend env: TZ: Europe/Berlin + # Diagnostic guard: covers the coverage run only. If `npm test` (above) + # exits 1 with a birpc error, the named pattern appears here — not there. + - name: Assert no birpc teardown race in coverage run + shell: bash + if: always() + run: | + if grep -qF "[birpc] rpc is closed" /tmp/coverage-test-${{ github.run_id }}.log 2>/dev/null; then + echo "FAIL: [birpc] rpc is closed teardown race detected in coverage run" + grep -F "[birpc] rpc is closed" /tmp/coverage-test-${{ github.run_id }}.log + exit 1 + fi + - name: Upload coverage reports if: always() uses: actions/upload-artifact@v4 with: name: coverage-reports - path: frontend/coverage/ + path: | + frontend/coverage/ + /tmp/coverage-test-${{ github.run_id }}.log - name: Build frontend run: npm run build diff --git a/docs/adr/012-browser-test-mocking-strategy.md b/docs/adr/012-browser-test-mocking-strategy.md new file mode 100644 index 00000000..763a7649 --- /dev/null +++ b/docs/adr/012-browser-test-mocking-strategy.md @@ -0,0 +1,90 @@ +# 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; + +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 + +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` type to avoid parallel type definitions across modules. diff --git a/frontend/src/lib/document/viewer/PdfViewer.svelte b/frontend/src/lib/document/viewer/PdfViewer.svelte index 5b23911f..311033c8 100644 --- a/frontend/src/lib/document/viewer/PdfViewer.svelte +++ b/frontend/src/lib/document/viewer/PdfViewer.svelte @@ -1,6 +1,6 @@