diff --git a/docs/adr/012-browser-test-mocking-strategy.md b/docs/adr/012-browser-test-mocking-strategy.md index 05656bb9..029c1e77 100644 --- a/docs/adr/012-browser-test-mocking-strategy.md +++ b/docs/adr/012-browser-test-mocking-strategy.md @@ -145,16 +145,27 @@ PR #657 attempted to delete `vi.mock` factories entirely and rely on Vitest auto 7. **In-suite no-factory-ban meta-test** at `frontend/src/__meta__/no-factory-ban.test.ts` — same source-scan mechanism as the other meta-tests; fails if any browser spec contains a `vi.mock('mod')` with no second argument. -### Sanctioned dedup: shared mock body + per-spec sync factory +### Cross-file sharing of a virtual-module mock body is infeasible (the third false premise) -To remove duplicated factory bodies without removing the factory, keep one shared mock module per virtual module under `src/__mocks__/` and import it via the `$mocks` alias into a sync factory: +The original #560 plan ("Option A") proposed deduplicating the non-trivial interceptor factories by importing a shared body from `src/__mocks__/` into a sync factory: ```ts import * as formsMock from "$mocks/$app/forms"; vi.mock("$app/forms", () => ({ ...formsMock })); ``` -The shared module owns any non-trivial mock logic and embeds its own `beforeEach` reset of mutable state, so isolation is structural. The `$mocks` alias is declared in **both** `vite.config.ts` and `vitest.client-coverage.config.ts` so it resolves in the coverage job too. Only genuinely-shared logic is consolidated; the ~80 trivial inline factories (`enhance: () => () => {}`, `{ goto: vi.fn() }`) are left untouched. +**CI proved this does not work in `@vitest/browser-playwright` 4.1.6**, across two runs: + +1. The static-import form above fails at runtime — vitest hoists `vi.mock` _above_ the import, so the factory references an uninitialised binding: `vi.mock factory: make sure there are no top level variables inside, since this call is hoisted`. +2. The documented escape, loading the body through an async hoisted import, fails to even parse in browser mode — vitest's hoist transform mangles it: `SyntaxError: Unexpected identifier 'vi'`. + + ```ts + const formsMock = await vi.hoisted(() => import("$mocks/$app/forms")); // parse error in browser mode + ``` + +`vi.hoisted` has the _same_ constraint as `vi.mock` (its factory can't reference top-level imports either, since it too is hoisted above them), so there is no way to get an external module's body into the hoisted context here. **Therefore: do not share virtual-module mock bodies across spec files. Define each `vi.mock` factory inline, with a synchronous body.** Duplicating the handful of interceptor factories is the accepted cost — it is the only pattern that works. The `src/__mocks__/$app/*` modules and the `$mocks` alias added for Option A were removed. (Revisit on a newer `@vitest/browser-playwright` whose hoist transform handles async `vi.hoisted` imports.) + +The no-factory-ban above still stands: every `vi.mock` of a virtual module must pass an _inline_ sync factory — never no factory, never a spread of an imported binding. ### Rejected: Option C (config-level auto-resolve) diff --git a/frontend/src/__meta__/no-factory-ban.test.ts b/frontend/src/__meta__/no-factory-ban.test.ts index 10efcd64..235c8d46 100644 --- a/frontend/src/__meta__/no-factory-ban.test.ts +++ b/frontend/src/__meta__/no-factory-ban.test.ts @@ -11,9 +11,10 @@ import { fileURLToPath } from 'url'; // implementation (replaceState, which delegates through a getter). The result // is a partial mock that crashes when an unsubstituted export is hit. // -// The sanctioned dedup pattern keeps the factory and shares its body: -// import * as formsMock from '$mocks/$app/forms'; -// vi.mock('$app/forms', () => ({ ...formsMock })); +// The sanctioned form keeps an INLINE sync factory: +// vi.mock('$app/forms', () => ({ enhance(node, submit) { ... } })); +// (Sharing the body via a module imported into the factory is infeasible in +// browser mode — vitest hoists vi.mock above the import; see ADR-012.) // // ESLint and the CI grep guard catch the pattern earlier; this in-suite test // catches it at every vitest invocation — the layer hardest to disable. It @@ -60,9 +61,10 @@ describe('scan: hasNoFactoryViMock', () => { ); }); - it('does not flag a vi.mock with a shared-body spread factory', () => { - const fixture = `import * as formsMock from '$mocks/$app/forms'; - vi.mock('$app/forms', () => ({ ...formsMock }));`; + it('does not flag a vi.mock with a multi-line inline factory', () => { + const fixture = `vi.mock('$app/forms', () => ({ + enhance: (node, submit) => ({ destroy() {} }) + }));`; expect(hasNoFactoryViMock(fixture)).toBe(false); }); diff --git a/frontend/src/__mocks__/$app/forms.ts b/frontend/src/__mocks__/$app/forms.ts deleted file mode 100644 index 2c8915dc..00000000 --- a/frontend/src/__mocks__/$app/forms.ts +++ /dev/null @@ -1,43 +0,0 @@ -// Shared browser-test mock body for the SvelteKit `$app/forms` virtual module. -// -// Imported into a sync vi.mock factory via the $mocks alias: -// import * as formsMock from '$mocks/$app/forms'; -// vi.mock('$app/forms', () => ({ ...formsMock })); -// -// `enhance` intercepts the form's submit event, invokes the component's -// SubmitFunction, and — when that returns a post-submit callback — calls it -// with the configurable `_formResult`. Tests drive the success/failure branch -// with `setFormResult({ type: 'failure' })`. The embedded `beforeEach` resets -// the result before every test, so isolation is structural, not per-spec. -// See ADR-012. - -import { beforeEach } from 'vitest'; - -export type FormEnhanceResult = { type: string; data?: Record }; - -let _formResult: FormEnhanceResult = { type: 'success' }; - -export function setFormResult(result: FormEnhanceResult): void { - _formResult = result; -} - -export function enhance( - node: HTMLFormElement, - submit?: (opts: { - formData: FormData; - }) => (opts: { result: FormEnhanceResult; update: () => Promise }) => Promise -): { destroy: () => void } { - const handler = async (e: Event) => { - e.preventDefault(); - const callback = submit?.({ formData: new FormData(node) } as never); - if (typeof callback === 'function') { - await callback({ result: _formResult, update: async () => {} }); - } - }; - node.addEventListener('submit', handler); - return { destroy: () => node.removeEventListener('submit', handler) }; -} - -beforeEach(() => { - _formResult = { type: 'success' }; -}); diff --git a/frontend/src/__mocks__/$app/navigation.ts b/frontend/src/__mocks__/$app/navigation.ts deleted file mode 100644 index 2421289e..00000000 --- a/frontend/src/__mocks__/$app/navigation.ts +++ /dev/null @@ -1,62 +0,0 @@ -// Shared browser-test mock body for the SvelteKit `$app/navigation` virtual module. -// -// Imported into a sync vi.mock factory via the $mocks alias: -// import * as navMock from '$mocks/$app/navigation'; -// vi.mock('$app/navigation', () => ({ ...navMock })); -// -// All navigation functions are vi.fn() stubs. `beforeNavigate` additionally -// captures the registered callback so a test can drive it through the exported -// `simulateNavigate(href)` helper — the whole capture-and-fire pattern lives -// here, not the raw callback. The embedded `beforeEach` clears the captured -// callback and the mock call histories before every test, so isolation is -// structural. See ADR-012. - -import { beforeEach, vi } from 'vitest'; - -type BeforeNavigateCallback = (nav: { - cancel: () => void; - to: { url: { href: string } } | null; -}) => void; - -let _registeredBeforeNavigate: BeforeNavigateCallback | null = null; - -export const goto = vi.fn(); -export const invalidate = vi.fn(); -export const invalidateAll = vi.fn(); -export const beforeNavigate = vi.fn((fn: BeforeNavigateCallback) => { - _registeredBeforeNavigate = fn; -}); -export const afterNavigate = vi.fn(); -export const preloadCode = vi.fn(); -export const preloadData = vi.fn(); -export const pushState = vi.fn(); -export const replaceState = vi.fn(); -export const disableScrollHandling = vi.fn(); -export const onNavigate = vi.fn(); - -const _navMocks = [ - goto, - invalidate, - invalidateAll, - beforeNavigate, - afterNavigate, - preloadCode, - preloadData, - pushState, - replaceState, - disableScrollHandling, - onNavigate -]; - -// Fire the captured beforeNavigate callback as if navigating to `href`. -// Returns the cancel spy so the test can assert whether navigation was blocked. -export function simulateNavigate(href: string | null = '/somewhere'): ReturnType { - const cancel = vi.fn(); - _registeredBeforeNavigate?.({ cancel, to: href ? { url: { href } } : null }); - return cancel; -} - -beforeEach(() => { - _registeredBeforeNavigate = null; - _navMocks.forEach((mock) => mock.mockClear()); -}); diff --git a/frontend/src/lib/activity/ChronikFuerDichBox.svelte.spec.ts b/frontend/src/lib/activity/ChronikFuerDichBox.svelte.spec.ts index c5c26cf7..8ffd6713 100644 --- a/frontend/src/lib/activity/ChronikFuerDichBox.svelte.spec.ts +++ b/frontend/src/lib/activity/ChronikFuerDichBox.svelte.spec.ts @@ -4,12 +4,36 @@ import { page, userEvent } from 'vitest/browser'; import ChronikFuerDichBox from './ChronikFuerDichBox.svelte'; import type { NotificationItem } from '$lib/notification/notifications.svelte'; -const formsMock = await vi.hoisted(() => import('$mocks/$app/forms')); -vi.mock('$app/forms', () => ({ ...formsMock })); +const mockFormResult = vi.hoisted(() => ({ type: 'success' as string })); + +vi.mock('$app/forms', () => ({ + enhance( + node: HTMLFormElement, + submit?: (opts: { + formData: FormData; + }) => (opts: { + result: { type: string; data?: Record }; + update: () => Promise; + }) => Promise + ) { + const handler = async (e: Event) => { + e.preventDefault(); + const cb = submit?.({ formData: new FormData(node) } as never); + if (typeof cb === 'function') { + await ( + cb as (o: { result: typeof mockFormResult; update: () => Promise }) => Promise + )({ result: mockFormResult, update: async () => {} }); + } + }; + node.addEventListener('submit', handler); + return { destroy: () => node.removeEventListener('submit', handler) }; + } +})); afterEach(() => { cleanup(); + mockFormResult.type = 'success'; }); function notif(partial: Partial): NotificationItem { @@ -152,7 +176,7 @@ describe('ChronikFuerDichBox', () => { }); it('shows an accessible error banner when the dismiss action returns a failure', async () => { - formsMock.setFormResult({ type: 'failure' }); + mockFormResult.type = 'failure'; render(ChronikFuerDichBox, { unread: [notif({ id: 'err-1' })], optimisticMarkRead: vi.fn(), diff --git a/frontend/src/lib/activity/ChronikFuerDichBox.svelte.test.ts b/frontend/src/lib/activity/ChronikFuerDichBox.svelte.test.ts index f9386848..2ee844e3 100644 --- a/frontend/src/lib/activity/ChronikFuerDichBox.svelte.test.ts +++ b/frontend/src/lib/activity/ChronikFuerDichBox.svelte.test.ts @@ -3,12 +3,36 @@ import { cleanup, render } from 'vitest-browser-svelte'; import { page } from 'vitest/browser'; import ChronikFuerDichBox from './ChronikFuerDichBox.svelte'; import type { NotificationItem } from '$lib/notification/notifications'; -const formsMock = await vi.hoisted(() => import('$mocks/$app/forms')); -vi.mock('$app/forms', () => ({ ...formsMock })); +const mockFormResult = vi.hoisted(() => ({ type: 'success' as string })); + +vi.mock('$app/forms', () => ({ + enhance( + node: HTMLFormElement, + submit?: (opts: { + formData: FormData; + }) => (opts: { + result: { type: string; data?: Record }; + update: () => Promise; + }) => Promise + ) { + const handler = async (e: Event) => { + e.preventDefault(); + const cb = submit?.({ formData: new FormData(node) } as never); + if (typeof cb === 'function') { + await ( + cb as (o: { result: typeof mockFormResult; update: () => Promise }) => Promise + )({ result: mockFormResult, update: async () => {} }); + } + }; + node.addEventListener('submit', handler); + return { destroy: () => node.removeEventListener('submit', handler) }; + } +})); afterEach(() => { cleanup(); + mockFormResult.type = 'success'; }); const mention = (overrides: Partial = {}): NotificationItem => ({ @@ -136,7 +160,7 @@ describe('ChronikFuerDichBox', () => { }); it('shows an accessible error banner when the dismiss action returns a failure', async () => { - formsMock.setFormResult({ type: 'failure' }); + mockFormResult.type = 'failure'; render(ChronikFuerDichBox, { props: { unread: [mention({ id: 'err-1' })], diff --git a/frontend/src/lib/notification/NotificationBell.svelte.spec.ts b/frontend/src/lib/notification/NotificationBell.svelte.spec.ts index d325bb58..28c09577 100644 --- a/frontend/src/lib/notification/NotificationBell.svelte.spec.ts +++ b/frontend/src/lib/notification/NotificationBell.svelte.spec.ts @@ -4,10 +4,17 @@ import { m } from '$lib/paraglide/messages.js'; import type { NotificationItem } from '$lib/notification/notifications'; import NotificationFixture from './notification.test-fixture.svelte'; -const formsMock = await vi.hoisted(() => import('$mocks/$app/forms')); - vi.mock('$app/navigation', () => ({ goto: vi.fn(), beforeNavigate: vi.fn() })); -vi.mock('$app/forms', () => ({ ...formsMock })); +vi.mock('$app/forms', () => ({ + enhance(node: HTMLFormElement, submit?: (opts: { formData: FormData }) => unknown) { + const handler = (e: Event) => { + e.preventDefault(); + submit?.({ formData: new FormData(node) } as never); + }; + node.addEventListener('submit', handler); + return { destroy: () => node.removeEventListener('submit', handler) }; + } +})); // NotificationBell.onMount calls store.init(), which opens an EventSource and // fetches the unread count. Stub both so no real network or 401 → /login diff --git a/frontend/src/lib/notification/NotificationDropdown.svelte.test.ts b/frontend/src/lib/notification/NotificationDropdown.svelte.test.ts index 0ca8e79b..bdbda7eb 100644 --- a/frontend/src/lib/notification/NotificationDropdown.svelte.test.ts +++ b/frontend/src/lib/notification/NotificationDropdown.svelte.test.ts @@ -3,15 +3,41 @@ import { cleanup, render } from 'vitest-browser-svelte'; import { page } from 'vitest/browser'; import { goto } from '$app/navigation'; import NotificationDropdown from './NotificationDropdown.svelte'; -const formsMock = await vi.hoisted(() => import('$mocks/$app/forms')); vi.mock('$app/navigation', () => ({ goto: vi.fn() })); -vi.mock('$app/forms', () => ({ ...formsMock })); +// Configurable result for the enhance mock — tests that need failure set +// mockFormResult.type = 'failure' before clicking. +const mockFormResult = vi.hoisted(() => ({ type: 'success' as string })); + +// Invoke the SubmitFunction and always call the returned result callback with +// mockFormResult so tests can exercise both success and failure branches. +vi.mock('$app/forms', () => ({ + enhance( + node: HTMLFormElement, + submit?: (opts: { + formData: FormData; + }) => (opts: { + result: { type: string; data?: Record }; + update: () => Promise; + }) => Promise + ) { + const handler = async (e: Event) => { + e.preventDefault(); + const cb = submit?.({ formData: new FormData(node) } as never); + if (typeof cb === 'function') { + await cb({ result: mockFormResult, update: async () => {} } as never); + } + }; + node.addEventListener('submit', handler); + return { destroy: () => node.removeEventListener('submit', handler) }; + } +})); afterEach(() => { cleanup(); vi.clearAllMocks(); + mockFormResult.type = 'success'; // reset to default after each test }); const makeNotification = (overrides: Record = {}) => ({ @@ -209,7 +235,7 @@ describe('NotificationDropdown', () => { }); it('shows a role=alert error banner when mark-all-read returns a failure', async () => { - formsMock.setFormResult({ type: 'failure' }); + mockFormResult.type = 'failure'; render(NotificationDropdown, { props: { notifications: [makeNotification()], @@ -346,7 +372,7 @@ describe('NotificationDropdown', () => { }); it('does NOT call onClose or goto when the dismiss action returns a failure', async () => { - formsMock.setFormResult({ type: 'failure' }); + mockFormResult.type = 'failure'; const onClose = vi.fn(); const n = makeNotification({ id: 'n99', actorName: 'Bob' }); render(NotificationDropdown, { diff --git a/frontend/src/lib/shared/hooks/useUnsavedWarning.svelte.test.ts b/frontend/src/lib/shared/hooks/useUnsavedWarning.svelte.test.ts index 2926d6e3..d85dc4c3 100644 --- a/frontend/src/lib/shared/hooks/useUnsavedWarning.svelte.test.ts +++ b/frontend/src/lib/shared/hooks/useUnsavedWarning.svelte.test.ts @@ -1,12 +1,35 @@ -import { describe, it, expect, vi } from 'vitest'; -const navMock = await vi.hoisted(() => import('$mocks/$app/navigation')); +import { describe, it, expect, vi, beforeEach } from 'vitest'; -vi.mock('$app/navigation', () => ({ ...navMock })); +// Capture the beforeNavigate callback so tests can simulate navigation events +let registeredBeforeNavigate: + | ((nav: { cancel: () => void; to: { url: { href: string } } | null }) => void) + | null = null; -const { simulateNavigate, goto: mockGoto } = navMock; +const mockGoto = vi.fn(); + +vi.mock('$app/navigation', () => ({ + beforeNavigate: vi.fn((fn: typeof registeredBeforeNavigate) => { + registeredBeforeNavigate = fn; + }), + goto: mockGoto +})); const { createUnsavedWarning } = await import('./useUnsavedWarning.svelte'); +function simulateNavigate(href: string | null = '/somewhere') { + const cancel = vi.fn(); + registeredBeforeNavigate?.({ + cancel, + to: href ? { url: { href } } : null + }); + return cancel; +} + +beforeEach(() => { + registeredBeforeNavigate = null; + mockGoto.mockClear(); +}); + describe('createUnsavedWarning', () => { it('isDirty starts false', () => { const w = createUnsavedWarning(); diff --git a/frontend/svelte.config.js b/frontend/svelte.config.js index 3ca6f79a..e94e293c 100644 --- a/frontend/svelte.config.js +++ b/frontend/svelte.config.js @@ -8,11 +8,6 @@ const config = { preprocess: vitePreprocess(), kit: { adapter: adapter(), - // $mocks resolves shared browser-test mock bodies (src/__mocks__). Declared here - // so svelte-check/tsconfig and both vite configs resolve it. See ADR-012. - alias: { - $mocks: 'src/__mocks__' - }, prerender: { entries: ['/hilfe/transkription'], // Disable crawl: by default SvelteKit follows nav links from diff --git a/frontend/vite.config.ts b/frontend/vite.config.ts index 35430747..34686e4b 100644 --- a/frontend/vite.config.ts +++ b/frontend/vite.config.ts @@ -6,15 +6,8 @@ import { defineConfig } from 'vitest/config'; import { playwright } from '@vitest/browser-playwright'; import { sveltekit } from '@sveltejs/kit/vite'; import { viteStaticCopy } from 'vite-plugin-static-copy'; -import { fileURLToPath } from 'node:url'; export default defineConfig({ - resolve: { - alias: { - // Shared browser-test mock bodies, imported into sync vi.mock factories. See ADR-012. - $mocks: fileURLToPath(new URL('./src/__mocks__', import.meta.url)) - } - }, optimizeDeps: { include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention'] }, diff --git a/frontend/vitest.client-coverage.config.ts b/frontend/vitest.client-coverage.config.ts index b7b1ad57..dc822281 100644 --- a/frontend/vitest.client-coverage.config.ts +++ b/frontend/vitest.client-coverage.config.ts @@ -4,7 +4,6 @@ import tailwindcss from '@tailwindcss/vite'; import { defineConfig } from 'vitest/config'; import { playwright } from '@vitest/browser-playwright'; import { sveltekit } from '@sveltejs/kit/vite'; -import { fileURLToPath } from 'node:url'; // Standalone config for browser-project Istanbul coverage. // Uses a dedicated root-level coverage block because Vitest 4 ignores @@ -12,12 +11,6 @@ import { fileURLToPath } from 'node:url'; // Plugins mirrored from vite.config.ts: tailwindcss, sveltekit, devtoolsJson, paraglideVitePlugin // Update here whenever vite.config.ts plugins change. export default defineConfig({ - resolve: { - alias: { - // Shared browser-test mock bodies, imported into sync vi.mock factories. See ADR-012. - $mocks: fileURLToPath(new URL('./src/__mocks__', import.meta.url)) - } - }, optimizeDeps: { include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention'] },