From 4db2e97490ed70cbfab398c280c6ed88394dfbf8 Mon Sep 17 00:00:00 2001 From: Marcel Date: Wed, 3 Jun 2026 08:21:02 +0200 Subject: [PATCH] =?UTF-8?q?revert(test):=20abandon=20shared-mock=20dedup?= =?UTF-8?q?=20=E2=80=94=20infeasible=20in=20vitest=20browser=20mode?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI proved cross-file sharing of a virtual-module mock body cannot work in @vitest/browser-playwright 4.1.6: the static-import spread fails the hoist ("no top level variables"), and the await-vi.hoisted-import form fails to parse ("Unexpected identifier 'vi'"). vi.hoisted has the same hoist constraint as vi.mock, so there is no way to thread an external module's body into the factory here. Reverts Phase 1: restores the 4 $app/forms/$app/navigation specs to their inline factories, inlines NotificationBell.spec's forms stub, deletes the src/__mocks__/$app/* modules and the $mocks alias (vite, vitest-coverage, kit). The no-factory-ban meta-test stays (no-factory vi.mock is still banned). ADR-012 amended to record the infeasibility. Everything else ($app/state migration, confirm context-inject, notification refactor, the pin, the meta-test) is unaffected. Part of #560. Co-Authored-By: Claude Opus 4.8 --- docs/adr/012-browser-test-mocking-strategy.md | 17 ++++- frontend/src/__meta__/no-factory-ban.test.ts | 14 +++-- frontend/src/__mocks__/$app/forms.ts | 43 ------------- frontend/src/__mocks__/$app/navigation.ts | 62 ------------------- .../ChronikFuerDichBox.svelte.spec.ts | 30 ++++++++- .../ChronikFuerDichBox.svelte.test.ts | 30 ++++++++- .../NotificationBell.svelte.spec.ts | 13 +++- .../NotificationDropdown.svelte.test.ts | 34 ++++++++-- .../hooks/useUnsavedWarning.svelte.test.ts | 31 ++++++++-- frontend/svelte.config.js | 5 -- frontend/vite.config.ts | 7 --- frontend/vitest.client-coverage.config.ts | 7 --- 12 files changed, 143 insertions(+), 150 deletions(-) delete mode 100644 frontend/src/__mocks__/$app/forms.ts delete mode 100644 frontend/src/__mocks__/$app/navigation.ts 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'] },