fix(pdf-viewer): remove banned vi.mock('pdfjs-dist') — ADR 012 enforcement (issue #546) #549

Merged
marcel merged 4 commits from worktree-feat+issue-546-pdf-viewer-test-fix into main 2026-05-12 12:32:24 +02:00
5 changed files with 85 additions and 89 deletions

View File

@@ -36,6 +36,14 @@ jobs:
run: npm run lint
working-directory: frontend
- name: Assert no banned vi.mock patterns
shell: bash
run: |
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
- name: Run unit and component tests with coverage
shell: bash
run: |

View File

@@ -86,5 +86,5 @@ These modules are resolved at static import time (before any test runs). Their `
- 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.
- **Enforcement:** An ESLint `no-restricted-syntax` rule in `eslint.config.js` (scoped to `**/*.spec.ts` and `**/*.test.ts`) flags any `vi.mock('pdfjs-dist', ...)` call with a message referencing this ADR. This turns a ~2-minute CI wait into an immediate lint error on save. The CI birpc grep guard remains as belt-and-suspenders for the coverage run. A CI static grep step (added in issue #546) also catches the pattern before the test suite launches.
- **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.

View File

@@ -72,6 +72,20 @@ export default defineConfig(
]
}
},
{
files: ['**/*.spec.ts', '**/*.test.ts'],
rules: {
'no-restricted-syntax': [
'error',
{
selector:
"CallExpression[callee.object.name='vi'][callee.property.name='mock'] > Literal[value=/^pdfjs-dist/]",
message:
"Banned: vi.mock('pdfjs-dist', factory) causes a birpc teardown race in browser-mode specs — see ADR 012. Use the libLoader prop injection pattern instead."
}
]
}
},
{
plugins: { boundaries },
settings: {

View File

@@ -1,56 +0,0 @@
import { vi, describe, it, expect, afterEach } from 'vitest';
import { cleanup, render } from 'vitest-browser-svelte';
import { page } from 'vitest/browser';
import type { LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte';
import PdfViewer from './PdfViewer.svelte';
afterEach(cleanup);
function makeFakePdfjsLib() {
class TextLayerMock {
render() {
return Promise.resolve();
}
cancel() {}
}
return {
GlobalWorkerOptions: { workerSrc: '' },
getDocument: vi.fn().mockReturnValue({
promise: Promise.resolve({
numPages: 2,
getPage: vi.fn().mockResolvedValue({
getViewport: vi.fn().mockReturnValue({ width: 595, height: 842 }),
render: vi.fn().mockReturnValue({ promise: Promise.resolve() }),
streamTextContent: vi.fn().mockReturnValue(new ReadableStream())
})
})
}),
TextLayer: TextLayerMock
} as unknown as typeof import('pdfjs-dist');
}
function makeFakeLibLoader(): LibLoader {
const fakePdfjs = makeFakePdfjsLib();
return vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
}
describe('PdfViewer', () => {
it('shows previous and next page navigation buttons', async () => {
render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
await expect.element(page.getByRole('button', { name: /zurück/i })).toBeInTheDocument();
await expect.element(page.getByRole('button', { name: /weiter/i })).toBeInTheDocument();
});
it('shows zoom controls', async () => {
render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
await expect.element(page.getByRole('button', { name: /vergrößern/i })).toBeInTheDocument();
await expect.element(page.getByRole('button', { name: /verkleinern/i })).toBeInTheDocument();
});
it('displays the page counter once the PDF has loaded', async () => {
render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
// Fake loader resolves synchronously, so "1 / 2" should appear quickly
await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument();
});
});

View File

@@ -1,11 +1,18 @@
import { describe, it, expect, vi, afterEach } from 'vitest';
import { cleanup, render } from 'vitest-browser-svelte';
import { page } from 'vitest/browser';
import type { LibLoader } from '$lib/document/viewer/usePdfRenderer.svelte';
import PdfViewer from './PdfViewer.svelte';
vi.mock('pdfjs-dist', () => {
function TextLayerMock() {}
TextLayerMock.prototype.render = () => Promise.resolve();
TextLayerMock.prototype.cancel = () => {};
afterEach(cleanup);
function makeFakePdfjsLib() {
class TextLayerMock {
render() {
return Promise.resolve();
}
cancel() {}
}
return {
GlobalWorkerOptions: { workerSrc: '' },
@@ -20,24 +27,23 @@ vi.mock('pdfjs-dist', () => {
})
}),
TextLayer: TextLayerMock
};
});
} as unknown as typeof import('pdfjs-dist');
}
vi.mock('pdfjs-dist/build/pdf.worker.min.mjs?url', () => ({ default: '' }));
const { default: PdfViewer } = await import('./PdfViewer.svelte');
afterEach(cleanup);
function makeFakeLibLoader(): LibLoader {
const fakePdfjs = makeFakePdfjsLib();
return vi.fn().mockResolvedValue([fakePdfjs, { default: '' }] as const);
}
describe('PdfViewer — empty / error states', () => {
it('renders the no-file placeholder when url is empty', async () => {
render(PdfViewer, { url: '' });
render(PdfViewer, { url: '', libLoader: makeFakeLibLoader() });
await expect.element(page.getByText('Keine Datei vorhanden')).toBeVisible();
});
it('does not render the controls when url is empty', async () => {
render(PdfViewer, { url: '' });
render(PdfViewer, { url: '', libLoader: makeFakeLibLoader() });
const buttons = document.querySelectorAll('button');
expect(buttons.length).toBe(0);
@@ -49,10 +55,10 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
annotationReloadKey: 0
annotationReloadKey: 0,
libLoader: makeFakeLibLoader()
});
// PdfControls renders its nav + zoom buttons once the document.promise resolves.
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
await expect.element(page.getByRole('button', { name: 'Weiter' })).toBeVisible();
await expect.element(page.getByRole('button', { name: 'Vergrößern' })).toBeVisible();
@@ -63,7 +69,8 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
annotationsDimmed: true
annotationsDimmed: true,
libLoader: makeFakeLibLoader()
});
await vi.waitFor(() => {
@@ -96,11 +103,10 @@ describe('PdfViewer — loaded state', () => {
url: '/api/documents/test/file',
documentId: 'test',
transcribeMode: true,
documentFileHash: 'match'
documentFileHash: 'match',
libLoader: makeFakeLibLoader()
});
// transcribeMode forces showAnnotations=true; toggle button surfaces with "hide" label
// (only when annotationCount > 0).
await expect
.element(page.getByRole('button', { name: /annotierungen verbergen/i }))
.toBeVisible();
@@ -113,7 +119,8 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
documentFileHash: 'abc123'
documentFileHash: 'abc123',
libLoader: makeFakeLibLoader()
});
await vi.waitFor(() => {
@@ -125,7 +132,8 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
flashAnnotationId: 'ann-flashing'
flashAnnotationId: 'ann-flashing',
libLoader: makeFakeLibLoader()
});
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
@@ -135,7 +143,8 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
blockNumbers: { 'ann-1': 1, 'ann-2': 2 }
blockNumbers: { 'ann-1': 1, 'ann-2': 2 },
libLoader: makeFakeLibLoader()
});
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
@@ -145,7 +154,8 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
activeAnnotationId: 'ann-1'
activeAnnotationId: 'ann-1',
libLoader: makeFakeLibLoader()
});
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
@@ -156,10 +166,10 @@ describe('PdfViewer — loaded state', () => {
url: '/api/documents/test/file',
documentId: 'test',
transcribeMode: true,
activeAnnotationId: 'ann-1'
activeAnnotationId: 'ann-1',
libLoader: makeFakeLibLoader()
});
// Without an annotations fetch, the visibility toggle is hidden — just assert the always-on nav.
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
await expect.element(page.getByRole('button', { name: 'Weiter' })).toBeVisible();
});
@@ -169,7 +179,8 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
onAnnotationClick
onAnnotationClick,
libLoader: makeFakeLibLoader()
});
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
@@ -199,7 +210,8 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
documentFileHash: 'new-hash'
documentFileHash: 'new-hash',
libLoader: makeFakeLibLoader()
});
await vi.waitFor(() => {
@@ -234,10 +246,10 @@ describe('PdfViewer — loaded state', () => {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test',
documentFileHash: 'matching-hash'
documentFileHash: 'matching-hash',
libLoader: makeFakeLibLoader()
});
// Controls finish mounting, and the outdated notice stays absent.
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
expect(document.querySelector('[data-testid="annotation-outdated-notice"]')).toBeNull();
} finally {
@@ -250,10 +262,10 @@ describe('PdfViewer — loaded state', () => {
try {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test'
documentId: 'test',
libLoader: makeFakeLibLoader()
});
// PDF rendering does not depend on the annotations fetch — controls still appear.
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
expect(document.querySelector('[data-testid="annotation-outdated-notice"]')).toBeNull();
} finally {
@@ -268,7 +280,8 @@ describe('PdfViewer — loaded state', () => {
try {
render(PdfViewer, {
url: '/api/documents/test/file',
documentId: 'test'
documentId: 'test',
libLoader: makeFakeLibLoader()
});
await expect.element(page.getByRole('button', { name: 'Zurück' })).toBeVisible();
@@ -277,4 +290,21 @@ describe('PdfViewer — loaded state', () => {
fetchSpy.mockRestore();
}
});
it('shows previous and next page navigation buttons', async () => {
render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
await expect.element(page.getByRole('button', { name: /zurück/i })).toBeInTheDocument();
await expect.element(page.getByRole('button', { name: /weiter/i })).toBeInTheDocument();
});
it('shows zoom controls', async () => {
render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
await expect.element(page.getByRole('button', { name: /vergrößern/i })).toBeInTheDocument();
await expect.element(page.getByRole('button', { name: /verkleinern/i })).toBeInTheDocument();
});
it('displays the page counter once the PDF has loaded', async () => {
render(PdfViewer, { url: '/api/documents/test-id/file', libLoader: makeFakeLibLoader() });
await expect.element(page.getByText(/1\s*\/\s*2/)).toBeInTheDocument();
});
});