test: PdfViewer.svelte.test.ts re-introduces banned vi.mock('pdfjs-dist') factory — restores birpc teardown race #546

New Issue

marcel · 2026-05-12T10:31:13+02:00

marcel commented

2026-05-12 10:31:13 +02:00

Problem

frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts (added in the recent merge to main) uses vi.mock('pdfjs-dist', factory) on lines 5 and 26:

vi.mock('pdfjs-dist', () => { ... });
vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', () => ({ default: '' }));

This is exactly the pattern that PR #535 / commit 11547645 eliminated from the old PdfViewer.svelte.spec.ts. The old spec file was fixed to use prop injection (libLoader); the new test file was written with the banned pattern.

Effect: running npm run test locally exits with:

Error: [birpc] rpc is closed, cannot call "resolveManualMock"
  ❯ ManualMockedModule.factory   @vitest/browser/dist/index.js:3221:34

This also fails the CI coverage guard (Assert no birpc teardown race in coverage run).

Root cause

Per ADR 012: pdfjs-dist is loaded via dynamic import inside usePdfRenderer.svelte.ts::init(), which is called from onMount. The birpc factory route handler can receive this request after the worker channel has already closed, producing the unhandled rejection that exits vitest with code 1.

$app/navigation, $app/state, etc. are safe because they are resolved statically at module-eval time. pdfjs-dist is not.

Fix

Port PdfViewer.svelte.test.ts to use the libLoader prop-injection pattern already established in PdfViewer.svelte.spec.ts and documented in ADR 012:

const fakePdfjs = { GlobalWorkerOptions: { workerSrc: '' }, getDocument: vi.fn(), TextLayer: class {} };
const fakeLoader = vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
render(PdfViewer, { url: '...', libLoader: fakeLoader });

Remove the two vi.mock(...) calls at the top of the file. No production code change is needed — PdfViewer.svelte already accepts libLoader as an optional prop.

Acceptance criteria

PdfViewer.svelte.test.ts contains no vi.mock('pdfjs-dist', ...) call
npm run test exits 0 with no birpc unhandled rejection
CI coverage guard remains green

## Problem `frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts` (added in the recent merge to main) uses `vi.mock('pdfjs-dist', factory)` on lines 5 and 26: ```typescript vi.mock('pdfjs-dist', () => { ... }); vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', () => ({ default: '' })); ``` This is exactly the pattern that PR #535 / commit `11547645` eliminated from the old `PdfViewer.svelte.spec.ts`. The old spec file was fixed to use prop injection (`libLoader`); the new test file was written with the banned pattern. **Effect:** running `npm run test` locally exits with: ``` Error: [birpc] rpc is closed, cannot call "resolveManualMock" ❯ ManualMockedModule.factory @vitest/browser/dist/index.js:3221:34 ``` This also fails the CI coverage guard (`Assert no birpc teardown race in coverage run`). ## Root cause Per [ADR 012](../docs/adr/012-browser-test-mocking-strategy.md): `pdfjs-dist` is loaded via dynamic import inside `usePdfRenderer.svelte.ts::init()`, which is called from `onMount`. The birpc factory route handler can receive this request after the worker channel has already closed, producing the unhandled rejection that exits vitest with code 1. `$app/navigation`, `$app/state`, etc. are safe because they are resolved statically at module-eval time. `pdfjs-dist` is not. ## Fix Port `PdfViewer.svelte.test.ts` to use the `libLoader` prop-injection pattern already established in `PdfViewer.svelte.spec.ts` and documented in ADR 012: ```typescript const fakePdfjs = { GlobalWorkerOptions: { workerSrc: '' }, getDocument: vi.fn(), TextLayer: class {} }; const fakeLoader = vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const); render(PdfViewer, { url: '...', libLoader: fakeLoader }); ``` Remove the two `vi.mock(...)` calls at the top of the file. No production code change is needed — `PdfViewer.svelte` already accepts `libLoader` as an optional prop. ## Acceptance criteria - `PdfViewer.svelte.test.ts` contains no `vi.mock('pdfjs-dist', ...)` call - `npm run test` exits 0 with no birpc unhandled rejection - CI coverage guard remains green

marcel added the P2-medium bug test labels 2026-05-12 10:31:19 +02:00

marcel referenced this issue

2026-05-12 10:31:49 +02:00

test: fix flaky browser-mode tests in AnnotationShape and OcrTrainingCard specs #547

marcel commented

2026-05-12 10:34:28 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

PdfViewer.svelte.test.ts lines 5 and 26 carry two vi.mock('pdfjs-dist', ...) calls — the exact banned pattern from ADR 012.
PdfViewer.svelte.spec.ts already demonstrates the approved replacement: makeFakeLibLoader() returns vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const) — no vi.mock involved at all.
The LibLoader type is exported from usePdfRenderer.svelte.ts and can be imported directly in the migrated file — no redefinition needed.
PdfViewer.svelte.test.ts covers 32 test cases; PdfViewer.svelte.spec.ts covers only 3. The migration must carry all 32 behaviors across, not just remove the offending lines.
PdfViewer.svelte already accepts libLoader?: LibLoader (line ~43 via $props()), so zero production code changes are needed.

Recommendations

Copy makeFakePdfjsLib() and makeFakeLibLoader() from spec.ts into test.ts (or extract to a shared __fixtures__/pdfjs.ts), then delete both vi.mock calls at the top of test.ts.
Pass libLoader: makeFakeLibLoader() to every render(PdfViewer, { ... }) call — or hoist it into a beforeEach variable if all tests in a describe block share the same fake.
After the migration, consolidate spec.ts and test.ts into one canonical file. Two files for one component create discovery confusion and split ownership over which behaviors are covered where. The natural choice is to keep test.ts (larger coverage) and delete spec.ts.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - `PdfViewer.svelte.test.ts` lines 5 and 26 carry two `vi.mock('pdfjs-dist', ...)` calls — the exact banned pattern from ADR 012. - `PdfViewer.svelte.spec.ts` already demonstrates the approved replacement: `makeFakeLibLoader()` returns `vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const)` — no `vi.mock` involved at all. - The `LibLoader` type is exported from `usePdfRenderer.svelte.ts` and can be imported directly in the migrated file — no redefinition needed. - `PdfViewer.svelte.test.ts` covers **32 test cases**; `PdfViewer.svelte.spec.ts` covers only **3**. The migration must carry all 32 behaviors across, not just remove the offending lines. - `PdfViewer.svelte` already accepts `libLoader?: LibLoader` (line ~43 via `$props()`), so zero production code changes are needed. ### Recommendations - Copy `makeFakePdfjsLib()` and `makeFakeLibLoader()` from `spec.ts` into `test.ts` (or extract to a shared `__fixtures__/pdfjs.ts`), then delete both `vi.mock` calls at the top of `test.ts`. - Pass `libLoader: makeFakeLibLoader()` to every `render(PdfViewer, { ... })` call — or hoist it into a `beforeEach` variable if all tests in a `describe` block share the same fake. - After the migration, **consolidate** `spec.ts` and `test.ts` into one canonical file. Two files for one component create discovery confusion and split ownership over which behaviors are covered where. The natural choice is to keep `test.ts` (larger coverage) and delete `spec.ts`.

marcel commented

2026-05-12 10:34:37 +02:00

🏛️ Markus Keller — Application Architect

Observations

The libLoader prop pattern is correct architectural thinking: inversion of control at the component boundary. The production default is the real dynamic import; tests supply a synchronous fake via prop. No global module state is manipulated.
ADR 012 documents exactly this decision — the fact that the bug slipped through despite the ADR demonstrates that documentation + late CI guard alone is insufficient enforcement for a "never do this" rule.
Two parallel test files for the same component (PdfViewer.svelte.spec.ts with 3 tests and PdfViewer.svelte.test.ts with 32 tests) is an architectural smell. They will diverge further over time, and neither file owns the full behavioral specification.
The CI guard in ci.yml (lines 48–58) runs if: always() and grep-checks the coverage log — that is structurally correct. The problem is that it is the only line of defense, which is why this regression landed.

Recommendations

Add an ESLint no-restricted-syntax rule that matches vi.mock calls whose first argument starts with pdfjs-dist. This turns a ~2-minute CI wait into an immediate lint failure on save. The rule body should include a message referencing ADR 012 so the developer understands why.
Consolidate to one test file after migration. Delete spec.ts; the migrated test.ts becomes the canonical specification for PdfViewer.svelte. One component, one test file.
No documentation updates are needed — ADR 012 is current and the enforcement note was added in commit 7f07180c.

## 🏛️ Markus Keller — Application Architect ### Observations - The `libLoader` prop pattern is correct architectural thinking: inversion of control at the component boundary. The production default is the real dynamic import; tests supply a synchronous fake via prop. No global module state is manipulated. - ADR 012 documents exactly this decision — the fact that the bug slipped through despite the ADR demonstrates that documentation + late CI guard alone is insufficient enforcement for a "never do this" rule. - Two parallel test files for the same component (`PdfViewer.svelte.spec.ts` with 3 tests and `PdfViewer.svelte.test.ts` with 32 tests) is an architectural smell. They will diverge further over time, and neither file owns the full behavioral specification. - The CI guard in `ci.yml` (lines 48–58) runs `if: always()` and grep-checks the coverage log — that is structurally correct. The problem is that it is the **only** line of defense, which is why this regression landed. ### Recommendations - **Add an ESLint `no-restricted-syntax` rule** that matches `vi.mock` calls whose first argument starts with `pdfjs-dist`. This turns a ~2-minute CI wait into an immediate lint failure on save. The rule body should include a message referencing ADR 012 so the developer understands why. - **Consolidate to one test file** after migration. Delete `spec.ts`; the migrated `test.ts` becomes the canonical specification for `PdfViewer.svelte`. One component, one test file. - No documentation updates are needed — ADR 012 is current and the enforcement note was added in commit `7f07180c`.

marcel commented

2026-05-12 10:34:46 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

The CI guard in .gitea/workflows/ci.yml (lines 48–58) is structurally correct: it uses if: always(), tee's the coverage run to /tmp/coverage-test-${{ github.run_id }}.log, and grep-fails on [birpc] rpc is closed. It works.
Per ADR 012, the guard intentionally covers the coverage run (not the initial npm test step) because that is where the log file is captured. This is a documented design choice, not a gap.
The guard is the last line of defense. There is no early-exit mechanism: a developer can commit the banned pattern, push, and the CI will fail only after the full test suite runs (~2 minutes). The pattern slipped through PR review, which is what created this issue.
The npm run test step would also exit 1 directly (as described in the issue), so CI does catch this at two points — but neither is fast.

Recommendations

Add a static grep check as a dedicated early CI step, before the test suite runs:

- name: Assert no banned vi.mock patterns
  run: |
    if grep -rn "vi\.mock('pdfjs-dist'" frontend/src/; then
      echo "FAIL: banned vi.mock('pdfjs-dist') pattern found — see ADR 012"
      exit 1
    fi

This costs ~1 second and fails the build before Playwright spins up. Place it in the lint job or as a pre-test step.

This is purely additive — the existing guard remains as a belt-and-suspenders check for the coverage run.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - The CI guard in `.gitea/workflows/ci.yml` (lines 48–58) is structurally correct: it uses `if: always()`, tee's the coverage run to `/tmp/coverage-test-${{ github.run_id }}.log`, and grep-fails on `[birpc] rpc is closed`. It works. - Per ADR 012, the guard intentionally covers the **coverage run** (not the initial `npm test` step) because that is where the log file is captured. This is a documented design choice, not a gap. - The guard is the **last** line of defense. There is no early-exit mechanism: a developer can commit the banned pattern, push, and the CI will fail only after the full test suite runs (~2 minutes). The pattern slipped through PR review, which is what created this issue. - The `npm run test` step would also exit 1 directly (as described in the issue), so CI does catch this at two points — but neither is fast. ### Recommendations - Add a **static grep check as a dedicated early CI step**, before the test suite runs: ```yaml - name: Assert no banned vi.mock patterns run: | if grep -rn "vi\.mock('pdfjs-dist'" frontend/src/; then echo "FAIL: banned vi.mock('pdfjs-dist') pattern found — see ADR 012" exit 1 fi ``` This costs ~1 second and fails the build before Playwright spins up. Place it in the lint job or as a pre-test step. - This is purely additive — the existing guard remains as a belt-and-suspenders check for the coverage run.

marcel commented

2026-05-12 10:34:54 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

PdfViewer.svelte.test.ts contains 32 test cases covering loaded state, empty/error states, pagination, zoom, text layer rendering, and canvas output. PdfViewer.svelte.spec.ts contains 3 tests. These are not equivalent files — they test different behavioral slices.
The current acceptance criteria (no vi.mock call, npm run test exits 0, CI guard green) are necessary but not sufficient. A migration that removes 29 tests to make the error go away would satisfy all three criteria while destroying coverage.
The birpc failure is a particularly deceptive failure mode: all 32 test assertions pass, but the process exits 1. The team sees "tests passing" and "CI red" simultaneously — this erodes trust in the suite faster than a normal failing test.
Two files (spec.ts + test.ts) for one component creates split coverage ownership. After migration, it is unclear which file should receive new tests for PdfViewer.svelte.

Recommendations

Add a fourth acceptance criterion: "All test cases present in PdfViewer.svelte.test.ts before migration continue to exist and pass after migration." This prevents a lazy fix that silently reduces test coverage.
After the migration, consolidate into one file — keep test.ts (the comprehensive one), delete spec.ts. One component, one test file, unambiguous coverage ownership.
Reuse the makeFakeLibLoader() helper from spec.ts as the foundation. It already handles the TextLayerMock class that the 32 tests in test.ts depend on for text layer assertions.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - `PdfViewer.svelte.test.ts` contains **32 test cases** covering loaded state, empty/error states, pagination, zoom, text layer rendering, and canvas output. `PdfViewer.svelte.spec.ts` contains **3 tests**. These are not equivalent files — they test different behavioral slices. - The current acceptance criteria (no `vi.mock` call, `npm run test` exits 0, CI guard green) are **necessary but not sufficient**. A migration that removes 29 tests to make the error go away would satisfy all three criteria while destroying coverage. - The birpc failure is a particularly deceptive failure mode: all 32 test assertions pass, but the process exits 1. The team sees "tests passing" and "CI red" simultaneously — this erodes trust in the suite faster than a normal failing test. - Two files (`spec.ts` + `test.ts`) for one component creates split coverage ownership. After migration, it is unclear which file should receive new tests for `PdfViewer.svelte`. ### Recommendations - **Add a fourth acceptance criterion**: "All test cases present in `PdfViewer.svelte.test.ts` before migration continue to exist and pass after migration." This prevents a lazy fix that silently reduces test coverage. - After the migration, **consolidate into one file** — keep `test.ts` (the comprehensive one), delete `spec.ts`. One component, one test file, unambiguous coverage ownership. - Reuse the `makeFakeLibLoader()` helper from `spec.ts` as the foundation. It already handles the `TextLayerMock` class that the 32 tests in `test.ts` depend on for text layer assertions.

marcel commented

2026-05-12 10:34:58 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

This issue is entirely test infrastructure. No production code changes, no new endpoints, no auth or permission changes.
The libLoader prop is an internal testing seam: it defaults to the real dynamic import in production (() => Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')])), and is only overridden in tests. This pattern does not widen the attack surface.
The vi.mock approach that is being removed operates at the Vite module resolution layer during test runs — it has no runtime presence and no security implications in either direction.

No security concerns from my angle. The dependency injection pattern being adopted is strictly tighter than the global mock approach: it narrows the surface by eliminating test-time global module state manipulation.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - This issue is entirely test infrastructure. No production code changes, no new endpoints, no auth or permission changes. - The `libLoader` prop is an internal testing seam: it defaults to the real dynamic import in production (`() => Promise.all([import('pdfjs-dist'), import('pdfjs-dist/build/pdf.worker.min.mjs?url')])`), and is only overridden in tests. This pattern does not widen the attack surface. - The `vi.mock` approach that is being removed operates at the Vite module resolution layer during test runs — it has no runtime presence and no security implications in either direction. No security concerns from my angle. The dependency injection pattern being adopted is strictly tighter than the global mock approach: it narrows the surface by eliminating test-time global module state manipulation.

marcel commented

2026-05-12 10:35:02 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

This issue is a test infrastructure fix only. PdfViewer.svelte itself is not modified — no component markup, props visible to users, interaction states, focus behavior, or brand tokens change.
The libLoader prop is an optional internal testing seam (libLoader?: LibLoader). It is not rendered, not announced to assistive technology, and has no effect on the PDF viewer's visual or interactive behavior in production.

No UX or accessibility concerns from my angle. The PDF viewer's rendered experience, touch targets, keyboard navigation, and ARIA semantics are unchanged by this fix.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - This issue is a test infrastructure fix only. `PdfViewer.svelte` itself is not modified — no component markup, props visible to users, interaction states, focus behavior, or brand tokens change. - The `libLoader` prop is an optional internal testing seam (`libLoader?: LibLoader`). It is not rendered, not announced to assistive technology, and has no effect on the PDF viewer's visual or interactive behavior in production. No UX or accessibility concerns from my angle. The PDF viewer's rendered experience, touch targets, keyboard navigation, and ARIA semantics are unchanged by this fix.

marcel commented

2026-05-12 10:35:09 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is well-specified for a test bug: root cause, affected files, code snippets, and the fix are all present. The acceptance criteria are testable and unambiguous as written.
Gap — coverage preservation: AC says "no vi.mock('pdfjs-dist', ...) call" and "npm run test exits 0," but nothing protects the 32 behavioral test cases that currently live in PdfViewer.svelte.test.ts. A migration that satisfies the three listed criteria while deleting tests is technically compliant with the issue as written.
Gap — scope boundary: The issue body states "no production code change is needed" but this is not expressed as an acceptance criterion. If someone accidentally modifies PdfViewer.svelte or usePdfRenderer.svelte.ts as part of this fix, nothing in the AC would catch it.
The "fix" section provides a concrete code snippet that makes the implementation path unambiguous — this is good practice for test bugs where the desired end state is specific.

Recommendations

Add one AC: "All test cases that existed in PdfViewer.svelte.test.ts before this fix continue to exist and pass after migration." This closes the coverage-reduction gap.
Optionally add: "No files outside frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts are modified" — makes the scope boundary explicit and reviewable.

## 📋 Elicit — Requirements Engineer ### Observations - The issue is well-specified for a test bug: root cause, affected files, code snippets, and the fix are all present. The acceptance criteria are testable and unambiguous as written. - **Gap — coverage preservation:** AC says "no `vi.mock('pdfjs-dist', ...)` call" and "npm run test exits 0," but nothing protects the 32 behavioral test cases that currently live in `PdfViewer.svelte.test.ts`. A migration that satisfies the three listed criteria while deleting tests is technically compliant with the issue as written. - **Gap — scope boundary:** The issue body states "no production code change is needed" but this is not expressed as an acceptance criterion. If someone accidentally modifies `PdfViewer.svelte` or `usePdfRenderer.svelte.ts` as part of this fix, nothing in the AC would catch it. - The "fix" section provides a concrete code snippet that makes the implementation path unambiguous — this is good practice for test bugs where the desired end state is specific. ### Recommendations - Add one AC: **"All test cases that existed in `PdfViewer.svelte.test.ts` before this fix continue to exist and pass after migration."** This closes the coverage-reduction gap. - Optionally add: **"No files outside `frontend/src/lib/document/viewer/PdfViewer.svelte.test.ts` are modified"** — makes the scope boundary explicit and reviewable.

marcel referenced this issue

2026-05-12 11:12:45 +02:00

test: fix flaky browser-mode tests in AnnotationShape and OcrTrainingCard specs #547

marcel referenced this issue

2026-05-12 11:12:52 +02:00

test: fix flaky browser-mode tests in AnnotationShape and OcrTrainingCard specs #547

marcel referenced this issue

2026-05-12 11:13:00 +02:00

test: fix flaky browser-mode tests in AnnotationShape and OcrTrainingCard specs #547

marcel referenced this issue

2026-05-12 11:13:22 +02:00

test: fix flaky browser-mode tests in AnnotationShape and OcrTrainingCard specs #547