bug(test): flaky browser-mode test — admin edit-user unsaved-changes guard #538

New Issue

marcel · 2026-05-11T22:46:07+02:00

marcel commented

2026-05-11 22:46:07 +02:00

Problem

src/routes/admin/users/[id]/page.svelte.spec.ts — the "unsaved-changes guard" suite intermittently fails in browser mode (Chromium/vitest-browser):

AssertionError: expected "vi.fn()" to be called with arguments: [ 'http://localhost/admin/users/u2' ]
Number of calls: 0

> Admin edit user page – unsaved-changes guard > discard button calls goto with the target URL

The goto mock records zero calls, which means the discard button click either didn't fire or the navigation side-effect didn't complete before the assertion ran.

Observed on

Branch feat/issue-535-birpc-teardown-race (and presumably main) — surfaced while reviewing PR #536. The failures appear consistently across multiple runs in this test session, suggesting the issue is reproducible but timing-sensitive.

Root cause hypothesis

The test clicks a button and then immediately asserts goto was called. In browser-mode vitest (real Chromium), click events and the resulting Svelte reactive updates may be async. The assertion likely needs an await on the click or a waitFor / expect.poll wrapper rather than a synchronous check.

Acceptance criteria

The "unsaved-changes guard" test suite in page.svelte.spec.ts passes consistently across 10 local runs
No Thread.sleep or arbitrary timeouts — use expect.poll or the vitest-browser auto-wait pattern
All other tests in the file remain green

## Problem `src/routes/admin/users/[id]/page.svelte.spec.ts` — the "unsaved-changes guard" suite intermittently fails in browser mode (Chromium/vitest-browser): ``` AssertionError: expected "vi.fn()" to be called with arguments: [ 'http://localhost/admin/users/u2' ] Number of calls: 0 > Admin edit user page – unsaved-changes guard > discard button calls goto with the target URL ``` The `goto` mock records zero calls, which means the discard button click either didn't fire or the navigation side-effect didn't complete before the assertion ran. ## Observed on Branch `feat/issue-535-birpc-teardown-race` (and presumably `main`) — surfaced while reviewing PR #536. The failures appear consistently across multiple runs in this test session, suggesting the issue is reproducible but timing-sensitive. ## Root cause hypothesis The test clicks a button and then immediately asserts `goto` was called. In browser-mode vitest (real Chromium), click events and the resulting Svelte reactive updates may be async. The assertion likely needs an `await` on the click or a `waitFor` / `expect.poll` wrapper rather than a synchronous check. ## Acceptance criteria - [ ] The "unsaved-changes guard" test suite in `page.svelte.spec.ts` passes consistently across 10 local runs - [ ] No `Thread.sleep` or arbitrary timeouts — use `expect.poll` or the vitest-browser auto-wait pattern - [ ] All other tests in the file remain green

marcel added the P2-medium bug test labels 2026-05-11 22:46:11 +02:00

marcel referenced this issue

2026-05-11 22:55:27 +02:00

fix(#535): eliminate vi.mock(pdfjs-dist) birpc teardown race via libLoader injection #536

marcel referenced this issue

2026-05-11 23:21:33 +02:00

fix(#535): eliminate vi.mock(pdfjs-dist) birpc teardown race via libLoader injection #536

marcel referenced this issue

2026-05-12 07:58:45 +02:00

fix(#535): eliminate vi.mock(pdfjs-dist) birpc teardown race via libLoader injection #536

marcel commented

2026-05-14 11:30:18 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Root cause confirmed. Line 256 (await page.getByRole('button', { name: /verwerfen/i }).click()) awaits the browser's delivery of the click event — not the Svelte effect triggered by it. goto() is called inside a Svelte reactive handler that runs asynchronously after the click returns. Line 258 then asserts synchronously before that effect has settled, which is why the mock call count is zero.
The fix is a one-liner. The file already uses vi.waitFor for exactly this pattern in the delete-confirmation suite (lines 190, 192, 203, 205). The discard-button test is just missing the same wrap:
```
// before (line 258)
expect(vi.mocked(goto)).toHaveBeenCalledWith('http://localhost/admin/users/u2');

// after
await vi.waitFor(() =>
  expect(vi.mocked(goto)).toHaveBeenCalledWith('http://localhost/admin/users/u2')
);
```
vi.waitFor polls until the assertion passes or the default timeout (1 000 ms) is reached — zero arbitrary delay, no Thread.sleep.
No other tests in this file share the problem. All other async outcomes are either checked via expect.element(...) (which auto-waits through vitest-browser) or via vi.waitFor. Only the discard-button test uses a bare sync assertion after an awaited browser click.
The beforeEach(() => vi.clearAllMocks()) on line 214 is correct — it ensures goto starts at zero calls for each test in the suite.

Recommendations

Apply the vi.waitFor wrap to line 258 as shown above. Nothing else needs changing.
Run 10 consecutive local runs with vitest --reporter=verbose to confirm AC1 before closing.
No additional abstractions needed; the fix should stay as an inline vi.waitFor call consistent with the rest of the file.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - **Root cause confirmed.** Line 256 (`await page.getByRole('button', { name: /verwerfen/i }).click()`) awaits the browser's delivery of the click event — not the Svelte effect triggered by it. `goto()` is called inside a Svelte reactive handler that runs asynchronously after the click returns. Line 258 then asserts synchronously before that effect has settled, which is why the mock call count is zero. - **The fix is a one-liner.** The file already uses `vi.waitFor` for exactly this pattern in the delete-confirmation suite (lines 190, 192, 203, 205). The discard-button test is just missing the same wrap: ```typescript // before (line 258) expect(vi.mocked(goto)).toHaveBeenCalledWith('http://localhost/admin/users/u2'); // after await vi.waitFor(() => expect(vi.mocked(goto)).toHaveBeenCalledWith('http://localhost/admin/users/u2') ); ``` `vi.waitFor` polls until the assertion passes or the default timeout (1 000 ms) is reached — zero arbitrary delay, no `Thread.sleep`. - **No other tests in this file share the problem.** All other async outcomes are either checked via `expect.element(...)` (which auto-waits through vitest-browser) or via `vi.waitFor`. Only the discard-button test uses a bare sync assertion after an awaited browser click. - **The `beforeEach(() => vi.clearAllMocks())` on line 214 is correct** — it ensures `goto` starts at zero calls for each test in the suite. ### Recommendations - Apply the `vi.waitFor` wrap to line 258 as shown above. Nothing else needs changing. - Run 10 consecutive local runs with `vitest --reporter=verbose` to confirm AC1 before closing. - No additional abstractions needed; the fix should stay as an inline `vi.waitFor` call consistent with the rest of the file.

marcel commented

2026-05-14 11:30:28 +02:00

🧪 Sara Holt — QA Engineer & Test Automation Specialist

Observations

This is the classic "await click, sync assert" anti-pattern in browser-mode testing. The problem is structural: await click() resolves when the browser dispatches the event, not when all downstream reactive effects have settled. Asserting on a mock call immediately after is a race condition by design.
The file is otherwise well-structured. The three other async outcomes in the delete-confirmation suite are all correctly guarded with vi.waitFor. The unsaved-changes guard suite correctly uses expect.element(...) for DOM assertions (which have built-in retries). The discard-button test is the sole exception — it falls into the sync-assert trap.
The acceptance criteria are achievable and well-scoped. "10 local runs consistently green" is a pragmatic bar. I'd additionally recommend a CI run on the fixed branch before closing — flaky tests sometimes only surface under the slower resource contention of a shared runner.
No other tests in the file need the same fix. I reviewed every assertion after a browser interaction:
- Delete confirmation tests: vi.waitFor already present ✅
- DOM assertions in unsaved-changes guard: expect.element(...) auto-waits ✅
- rerender-based test (line 261): re-renders are synchronous in vitest-browser, assertion is safe ✅

Recommendations

Fix: wrap line 258 in await vi.waitFor(() => expect(vi.mocked(goto)).toHaveBeenCalledWith('http://localhost/admin/users/u2')). One line, no test logic changes.
After the fix, confirm with vitest run --reporter=verbose over 10 runs locally, then verify CI green before closing the issue.
Add a note to the team's testing conventions (or a comment in the test file's describe block) that mock-call assertions after browser interactions always need vi.waitFor — this is the most common misunderstanding in vitest-browser migrations.

## 🧪 Sara Holt — QA Engineer & Test Automation Specialist ### Observations - This is the classic **"await click, sync assert"** anti-pattern in browser-mode testing. The problem is structural: `await click()` resolves when the browser dispatches the event, not when all downstream reactive effects have settled. Asserting on a mock call immediately after is a race condition by design. - **The file is otherwise well-structured.** The three other async outcomes in the delete-confirmation suite are all correctly guarded with `vi.waitFor`. The unsaved-changes guard suite correctly uses `expect.element(...)` for DOM assertions (which have built-in retries). The discard-button test is the sole exception — it falls into the sync-assert trap. - **The acceptance criteria are achievable and well-scoped.** "10 local runs consistently green" is a pragmatic bar. I'd additionally recommend a CI run on the fixed branch before closing — flaky tests sometimes only surface under the slower resource contention of a shared runner. - **No other tests in the file need the same fix.** I reviewed every assertion after a browser interaction: - Delete confirmation tests: `vi.waitFor` already present ✅ - DOM assertions in unsaved-changes guard: `expect.element(...)` auto-waits ✅ - `rerender`-based test (line 261): re-renders are synchronous in vitest-browser, assertion is safe ✅ ### Recommendations - **Fix**: wrap line 258 in `await vi.waitFor(() => expect(vi.mocked(goto)).toHaveBeenCalledWith('http://localhost/admin/users/u2'))`. One line, no test logic changes. - After the fix, confirm with `vitest run --reporter=verbose` over 10 runs locally, then verify CI green before closing the issue. - Add a note to the team's testing conventions (or a comment in the test file's describe block) that mock-call assertions after browser interactions always need `vi.waitFor` — this is the most common misunderstanding in vitest-browser migrations.

marcel commented

2026-05-14 11:30:36 +02:00

🛠️ Tobias Wendt — DevOps & Platform Engineer

Observations

Flaky tests are CI debt. A test that passes 80–90% of the time trains the team to re-run pipelines rather than investigate failures — real regressions start hiding behind noise. P2-medium is the right priority for this: not urgent, but not deferrable either.
The fix is entirely in source code — no CI config changes, no runner changes, no workflow changes. The scope is minimal.
The failure mode described (zero-calls on goto mock) is consistent and timing-sensitive rather than environment-specific, so the fix should hold on the Gitea act_runner too, not just locally.

Recommendations

No infrastructure changes needed. Apply the vi.waitFor fix and run a CI pass to confirm the test is stable under runner resource contention (which is typically slower than local Chromium).
Once fixed, consider adding a re-run-on-failure count of 1 to the frontend unit test job in the CI workflow as a general safety net for the vitest-browser suite. A single allowed retry on a genuinely stable test costs ~10 extra seconds and saves the pipeline from one-off Chromium startup hiccups. This is optional — only worth it if the suite shows other intermittent failures.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer ### Observations - Flaky tests are CI debt. A test that passes 80–90% of the time trains the team to re-run pipelines rather than investigate failures — real regressions start hiding behind noise. P2-medium is the right priority for this: not urgent, but not deferrable either. - The fix is entirely in source code — no CI config changes, no runner changes, no workflow changes. The scope is minimal. - The failure mode described (zero-calls on `goto` mock) is consistent and timing-sensitive rather than environment-specific, so the fix should hold on the Gitea act_runner too, not just locally. ### Recommendations - No infrastructure changes needed. Apply the `vi.waitFor` fix and run a CI pass to confirm the test is stable under runner resource contention (which is typically slower than local Chromium). - Once fixed, consider adding a **re-run-on-failure count of 1** to the frontend unit test job in the CI workflow as a general safety net for the vitest-browser suite. A single allowed retry on a genuinely stable test costs ~10 extra seconds and saves the pipeline from one-off Chromium startup hiccups. This is optional — only worth it if the suite shows other intermittent failures.

marcel commented

2026-05-14 11:30:43 +02:00

🏗️ Markus Keller — Application Architect

Observations

No architectural concerns. This issue is entirely within the test layer — a single test assertion is in the wrong execution phase relative to Svelte's async reactive model. The application code, module boundaries, and service layer are unaffected.
The fix does not touch production code, does not cross module boundaries, and does not require any API, schema, or documentation changes.

Recommendations

Apply the vi.waitFor fix as described by Felix and Sara — it's the right call, consistent with patterns already in the file.
Preventive measure worth considering: run a codebase-wide grep for the same anti-pattern in other *.spec.ts files — specifically, await.*\.click() on one line followed immediately by a sync expect(vi.mocked(...)) on the next. This pattern can exist silently in other tests that happen to be fast enough to not surface the race on current hardware. Surfacing them before they become CI failures is cheap:
```
grep -n "expect(vi.mocked" frontend/src/**/*.spec.ts
```
Then check each match: if the preceding line is an awaited browser interaction (not a vi.waitFor), it needs the same fix.

No open decisions from my side — the approach is clear.

## 🏗️ Markus Keller — Application Architect ### Observations - No architectural concerns. This issue is entirely within the test layer — a single test assertion is in the wrong execution phase relative to Svelte's async reactive model. The application code, module boundaries, and service layer are unaffected. - The fix does not touch production code, does not cross module boundaries, and does not require any API, schema, or documentation changes. ### Recommendations - Apply the `vi.waitFor` fix as described by Felix and Sara — it's the right call, consistent with patterns already in the file. - **Preventive measure worth considering:** run a codebase-wide grep for the same anti-pattern in other `*.spec.ts` files — specifically, `await.*\.click()` on one line followed immediately by a sync `expect(vi.mocked(...))` on the next. This pattern can exist silently in other tests that happen to be fast enough to not surface the race on current hardware. Surfacing them before they become CI failures is cheap: ```bash grep -n "expect(vi.mocked" frontend/src/**/*.spec.ts ``` Then check each match: if the preceding line is an awaited browser interaction (not a `vi.waitFor`), it needs the same fix. No open decisions from my side — the approach is clear.

marcel commented

2026-05-14 11:30:48 +02:00

🔐 Nora "NullX" Steiner — Application Security Engineer

No security concerns from this angle.

The affected test exercises the unsaved-changes guard — a pure UX flow that calls goto() with a URL already present in the component's beforeNavigate callback context. No user input is reflected into the mock call, no auth state is involved, and no security control is bypassed. The fix (wrapping a mock assertion in vi.waitFor) has no security surface.

I checked that the goto mock is properly scoped via vi.mock('$app/navigation') and cleared via vi.clearAllMocks() in beforeEach — no risk of cross-test mock state leaking authorization assumptions.

## 🔐 Nora "NullX" Steiner — Application Security Engineer No security concerns from this angle. The affected test exercises the unsaved-changes guard — a pure UX flow that calls `goto()` with a URL already present in the component's `beforeNavigate` callback context. No user input is reflected into the mock call, no auth state is involved, and no security control is bypassed. The fix (wrapping a mock assertion in `vi.waitFor`) has no security surface. I checked that the `goto` mock is properly scoped via `vi.mock('$app/navigation')` and cleared via `vi.clearAllMocks()` in `beforeEach` — no risk of cross-test mock state leaking authorization assumptions.

marcel commented

2026-05-14 11:30:51 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

No UX or accessibility concerns from this angle.

This is a pure test infrastructure fix. The "discard changes" flow itself — showing an unsaved-changes warning banner and offering a "Verwerfen" button — is a sound UX pattern (Nielsen's heuristic #3: user control and freedom). The test correctly verifies that the discard button navigates the user to their intended destination. The issue is only that the assertion runs too early in browser mode, not that the interaction design or accessibility of the feature is incorrect.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist No UX or accessibility concerns from this angle. This is a pure test infrastructure fix. The "discard changes" flow itself — showing an unsaved-changes warning banner and offering a "Verwerfen" button — is a sound UX pattern (Nielsen's heuristic #3: user control and freedom). The test correctly verifies that the discard button navigates the user to their intended destination. The issue is only that the assertion runs too early in browser mode, not that the interaction design or accessibility of the feature is incorrect.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#538