diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index aa65e565..5aa5d748 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -39,10 +39,22 @@ jobs: - name: Assert no banned vi.mock patterns shell: bash run: | + # Literal pdfjs-dist (libLoader pattern — ADR 012) if grep -rF "vi.mock('pdfjs-dist'" frontend/src/; then echo "FAIL: banned vi.mock('pdfjs-dist') pattern found — see ADR 012. Use the libLoader prop injection pattern instead." exit 1 fi + # Async factory with dynamic import in body (named mechanism — ADR 012 / #553). + # Multiline PCRE matches `vi.mock(, async ... { ... await import(...) ... })` + # across line breaks. __meta__ is excluded because it contains fixture strings + # demonstrating the very pattern this check is meant to forbid. + if grep -rPzln 'vi\.mock\([^)]+,\s*async[^{]*\{[\s\S]*?await\s+import\s*\(' \ + --include='*.spec.ts' --include='*.test.ts' \ + --exclude-dir='__meta__' \ + frontend/src/; then + echo "FAIL: banned async vi.mock factory with dynamic import in body — see ADR 012 / #553. Use a synchronous factory + vi.hoisted instead." + exit 1 + fi - name: Run unit and component tests with coverage shell: bash diff --git a/.gitea/workflows/coverage-flake-probe.yml b/.gitea/workflows/coverage-flake-probe.yml new file mode 100644 index 00000000..0258ba7a --- /dev/null +++ b/.gitea/workflows/coverage-flake-probe.yml @@ -0,0 +1,64 @@ +name: Coverage Flake Probe + +# Manually-triggered probe for the birpc teardown race documented in ADR 012 +# / #553. Runs the full coverage suite 20× in parallel against a single SHA +# and asserts zero `[birpc] rpc is closed` lines across every cell. Verifies +# the acceptance criterion that the race no longer surfaces under coverage. + +on: + workflow_dispatch: + +jobs: + coverage-flake-probe: + name: Coverage flake probe (run ${{ matrix.run }}) + runs-on: ubuntu-latest + container: + image: mcr.microsoft.com/playwright:v1.58.2-noble + strategy: + fail-fast: false + matrix: + run: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20] + steps: + - uses: actions/checkout@v4 + + - name: Cache node_modules + id: node-modules-cache + uses: actions/cache@v4 + with: + path: frontend/node_modules + key: node-modules-${{ hashFiles('frontend/package-lock.json') }} + + - name: Install dependencies + if: steps.node-modules-cache.outputs.cache-hit != 'true' + run: npm ci + working-directory: frontend + + - name: Compile Paraglide i18n + run: npx @inlang/paraglide-js compile --project ./project.inlang --outdir ./src/lib/paraglide + working-directory: frontend + + - name: Run unit and component tests with coverage + shell: bash + run: | + set -eo pipefail + npm run test:coverage 2>&1 | tee /tmp/coverage-test-${{ github.run_id }}-${{ matrix.run }}.log + working-directory: frontend + env: + TZ: Europe/Berlin + + - name: Assert no birpc teardown race + shell: bash + if: always() + run: | + if grep -qF "[birpc] rpc is closed" /tmp/coverage-test-${{ github.run_id }}-${{ matrix.run }}.log 2>/dev/null; then + echo "FAIL: [birpc] rpc is closed teardown race detected in run ${{ matrix.run }}" + grep -F "[birpc] rpc is closed" /tmp/coverage-test-${{ github.run_id }}-${{ matrix.run }}.log + exit 1 + fi + + - name: Upload coverage log on failure + if: failure() + uses: actions/upload-artifact@v4 + with: + name: coverage-log-run-${{ matrix.run }} + path: /tmp/coverage-test-${{ github.run_id }}-${{ matrix.run }}.log diff --git a/docs/adr/012-browser-test-mocking-strategy.md b/docs/adr/012-browser-test-mocking-strategy.md index d916fc10..8b0ac788 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,57 @@ 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 `