revert(test): abandon shared-mock dedup — infeasible in vitest browser mode
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 2m42s
CI / OCR Service Tests (pull_request) Successful in 22s
CI / Backend Unit Tests (pull_request) Successful in 3m31s
CI / fail2ban Regex (pull_request) Successful in 46s
CI / Semgrep Security Scan (pull_request) Successful in 21s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 2m42s
CI / OCR Service Tests (pull_request) Successful in 22s
CI / Backend Unit Tests (pull_request) Successful in 3m31s
CI / fail2ban Regex (pull_request) Successful in 46s
CI / Semgrep Security Scan (pull_request) Successful in 21s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
|
||||
@@ -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<string, unknown> };
|
||||
|
||||
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<void> }) => Promise<void>
|
||||
): { 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' };
|
||||
});
|
||||
@@ -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<typeof vi.fn> {
|
||||
const cancel = vi.fn();
|
||||
_registeredBeforeNavigate?.({ cancel, to: href ? { url: { href } } : null });
|
||||
return cancel;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
_registeredBeforeNavigate = null;
|
||||
_navMocks.forEach((mock) => mock.mockClear());
|
||||
});
|
||||
@@ -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<string, unknown> };
|
||||
update: () => Promise<void>;
|
||||
}) => Promise<void>
|
||||
) {
|
||||
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<void> }) => Promise<void>
|
||||
)({ result: mockFormResult, update: async () => {} });
|
||||
}
|
||||
};
|
||||
node.addEventListener('submit', handler);
|
||||
return { destroy: () => node.removeEventListener('submit', handler) };
|
||||
}
|
||||
}));
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
mockFormResult.type = 'success';
|
||||
});
|
||||
|
||||
function notif(partial: Partial<NotificationItem>): 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(),
|
||||
|
||||
@@ -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<string, unknown> };
|
||||
update: () => Promise<void>;
|
||||
}) => Promise<void>
|
||||
) {
|
||||
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<void> }) => Promise<void>
|
||||
)({ result: mockFormResult, update: async () => {} });
|
||||
}
|
||||
};
|
||||
node.addEventListener('submit', handler);
|
||||
return { destroy: () => node.removeEventListener('submit', handler) };
|
||||
}
|
||||
}));
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
mockFormResult.type = 'success';
|
||||
});
|
||||
|
||||
const mention = (overrides: Partial<NotificationItem> = {}): 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' })],
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<string, unknown> };
|
||||
update: () => Promise<void>;
|
||||
}) => Promise<void>
|
||||
) {
|
||||
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<string, unknown> = {}) => ({
|
||||
@@ -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, {
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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']
|
||||
},
|
||||
|
||||
@@ -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']
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user