fix(js-test): harden timing-fragile metron detail-fetch tests with vi.waitFor (bookshelf-wro8e) #1337

Merged

zombor merged 2 commits from bd-bookshelf-wro8e into main 2026-08-05 03:04:26 +00:00

Conversation 3 Commits 2 Files changed 1 +68 -34

zombor commented 2026-08-05 02:03:03 +00:00

Owner

Copy link

Root cause

Three 'METRON LAZY DETAIL' tests in metadata_fetch_controller_comic.test.js used fixed microtask drains — for (var i = 0; i < 8/16; i++) { await Promise.resolve(); } — to wait for async DOM mutations. The _fetchProvider path goes through a multi-level promise chain (POST → resp.json() → pollJob → poll() → fetch status → resp.json() → _handleProviderResponse → render candidates). Under a loaded CI runner this chain requires more microtask ticks than the fixed count provides, so the .candidate-card element or the detail-fetch call in mock.calls isn't visible at assertion time. The test then hangs near the 10 s hook timeout (~9878 ms observed) or fails outright, nondeterministically.

Fix

Replace the fixed await Promise.resolve() drains (lines 142-145 and 157-160 and 172-175) with vi.waitFor(), which polls until the condition holds and resolves as soon as it does — making the tests deterministic regardless of microtask scheduling pressure. Under a fast local run vi.waitFor resolves in its first poll tick (sub-millisecond). Under a loaded runner it keeps polling until the DOM catches up, well within vitest's default 1000 ms waitFor timeout.

Tests that already use vi.useFakeTimers() for the save-nav-timer path are unchanged.

Before / after timing (local, fast machine)

• Before: target test ~33 ms (flaked to ~9878 ms under load = near hook timeout)
• After: target test ~179 ms (polls once or twice, then resolves; stable under load)

Test plan

• npm test (all 137 test files, 4610 tests): all pass
• npm run coverage (100% branch/statement/function/line gate on controllers/**): gate passes

Closes bead bookshelf-wro8e on merge.

## Root cause Three 'METRON LAZY DETAIL' tests in `metadata_fetch_controller_comic.test.js` used fixed microtask drains — `for (var i = 0; i < 8/16; i++) { await Promise.resolve(); }` — to wait for async DOM mutations. The `_fetchProvider` path goes through a multi-level promise chain (POST → resp.json() → pollJob → poll() → fetch status → resp.json() → _handleProviderResponse → render candidates). Under a loaded CI runner this chain requires more microtask ticks than the fixed count provides, so the `.candidate-card` element or the detail-fetch call in `mock.calls` isn't visible at assertion time. The test then hangs near the 10 s hook timeout (~9878 ms observed) or fails outright, nondeterministically. ## Fix Replace the fixed `await Promise.resolve()` drains (lines 142-145 and 157-160 and 172-175) with `vi.waitFor()`, which polls until the condition holds and resolves as soon as it does — making the tests deterministic regardless of microtask scheduling pressure. Under a fast local run `vi.waitFor` resolves in its first poll tick (sub-millisecond). Under a loaded runner it keeps polling until the DOM catches up, well within vitest's default 1000 ms `waitFor` timeout. Tests that already use `vi.useFakeTimers()` for the save-nav-timer path are unchanged. ## Before / after timing (local, fast machine) - Before: target test ~33 ms (flaked to ~9878 ms under load = near hook timeout) - After: target test ~179 ms (polls once or twice, then resolves; stable under load) ## Test plan - `npm test` (all 137 test files, 4610 tests): all pass - `npm run coverage` (100% branch/statement/function/line gate on controllers/**): gate passes Closes bead bookshelf-wro8e on merge.

zombor added 1 commit 2026-08-05 02:03:04 +00:00

fix(js-test): harden timing-fragile metron detail-fetch tests with vi.waitFor (bookshelf-wro8e) ... 35801e1fdc

Three 'METRON LAZY DETAIL' tests used fixed microtask drains
(for-loop of 8/16 await Promise.resolve()) to wait for async DOM
mutations. Under a loaded CI runner, the promise chains from the
job-polling path require more microtask ticks than the fixed count,
so the candidate-card or the detail-fetch call aren't visible yet and
the assertion fails or the test times out near the 10 s hook deadline.

Replace the fixed drains with vi.waitFor(), which polls until the
condition holds (candidate-card in DOM; detail fetch URL in
mock.calls) and resolves as soon as it does. Tests that already use
vi.useFakeTimers() for the save-nav-timer path are unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-08-05 02:13:32 +00:00

Author

Owner

Copy link

Security review of PR #1337 — fix(js-test): harden timing-fragile metron detail-fetch tests with vi.waitFor (bookshelf-wro8e)

Scope: single test file (static/js/test/metadata_fetch_controller_comic.test.js), no production code changed.

---

Findings

Production code surface

Zero. The diff touches exactly one file: static/js/test/metadata_fetch_controller_comic.test.js. No Go, no templates, no SQL, no middleware, no routes. No production-code impact.

Assertion strength after conversion

The three converted tests each replace a fixed microtask-drain loop (for i < N; await Promise.resolve()) with vi.waitFor. The security-relevant assertions — that a candidate-detail fetch is actually triggered, that it contains provider=metron, and that it contains external_id=12345 — are preserved identically after the await. The vi.waitFor wrapper introduces a timeout-driven retry loop; if the fetch is never triggered the poll throws and the test fails, which is strictly stronger than the old fixed drain that could silently time out.

Notably, vi.waitFor here polls global.fetch.mock.calls (the mocked fetch call log, not a real network call) — there is no network egress from these tests at all.

external_id handling assertion

The third test (detail fetch URL includes the metron issue external_id) specifically guards that external_id=12345 (matching the fixture METRON_CANDIDATE.external_id) reaches the candidate-detail URL. That assertion is unchanged and remains a hard expect(detailCall).toContain("external_id=12345").

Fixtures for secrets / PII

All fixture values are synthetic test data: external_id: 12345, external_id: 567890, cover URL pointing to static.metron.cloud with a generic path. No API tokens, session tokens, real user data, or PII.

Test hygiene / package declaration

This is a Vitest/JS test file — the Go package <pkg>_test white-box rule does not apply. No concerns.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security review of PR #1337 — `fix(js-test): harden timing-fragile metron detail-fetch tests with vi.waitFor` (bookshelf-wro8e) **Scope:** single test file (`static/js/test/metadata_fetch_controller_comic.test.js`), no production code changed. --- ## Findings ### Production code surface Zero. The diff touches exactly one file: `static/js/test/metadata_fetch_controller_comic.test.js`. No Go, no templates, no SQL, no middleware, no routes. No production-code impact. ### Assertion strength after conversion The three converted tests each replace a fixed microtask-drain loop (`for i < N; await Promise.resolve()`) with `vi.waitFor`. The security-relevant assertions — that a `candidate-detail` fetch is actually triggered, that it contains `provider=metron`, and that it contains `external_id=12345` — are **preserved identically** after the await. The `vi.waitFor` wrapper introduces a timeout-driven retry loop; if the fetch is never triggered the poll throws and the test fails, which is strictly stronger than the old fixed drain that could silently time out. Notably, `vi.waitFor` here polls `global.fetch.mock.calls` (the mocked fetch call log, not a real network call) — there is no network egress from these tests at all. ### external_id handling assertion The third test (`detail fetch URL includes the metron issue external_id`) specifically guards that `external_id=12345` (matching the fixture `METRON_CANDIDATE.external_id`) reaches the candidate-detail URL. That assertion is unchanged and remains a hard `expect(detailCall).toContain("external_id=12345")`. ### Fixtures for secrets / PII All fixture values are synthetic test data: `external_id: 12345`, `external_id: 567890`, cover URL pointing to `static.metron.cloud` with a generic path. No API tokens, session tokens, real user data, or PII. ### Test hygiene / package declaration This is a Vitest/JS test file — the Go `package <pkg>_test` white-box rule does not apply. No concerns. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-05 02:14:00 +00:00

Author

Owner

Copy link

Code Review — bookshelf-wro8e (PR #1337)

CI status observed: JS Unit Tests: success (2m21s). Test Race, Coverage, Lint, Integration, E2E API all green. E2E Browser failing — this is the unrelated a11y flake being addressed in PR #1336, not this PR's concern. PR is mergeable: True.

---

Phase 1: Spec compliance

The bead goal is to eliminate the 3 flaky fixed-microtask-drain loops (for i < 16) in the METRON LAZY DETAIL "triggers detail fetch", "includes provider=metron", and "includes external_id" tests by replacing them with vi.waitFor(). All three conversions are present and correct in the diff.

---

Phase 2: Code quality findings

[MINOR] static/js/test/metadata_fetch_controller_comic.test.js:155 — expect(detailCall).toBeDefined() is now redundant after vi.waitFor

In the first converted test (clicking a metron candidate with external_id triggers detail fetch), the vi.waitFor block already throws if found is falsy (the if (!found) throw guard), and returns the URL string. The subsequent expect(detailCall).toBeDefined() (line 155 on the branch) is therefore a tautology — detailCall cannot be undefined at that point because vi.waitFor would have timed out instead. The other two tests (toContain("provider=metron"), toContain("external_id=12345")) don't have this redundancy because they assert a specific value. Suggested fix: remove the standalone expect(detailCall).toBeDefined() assertion; the vi.waitFor condition is the real gate, or replace it with a meaningful content assertion (e.g. toContain("candidate-detail")).

[MINOR] static/js/test/metadata_fetch_controller_comic.test.js:202-222 — Sibling "save body" tests in METRON LAZY DETAIL still use fixed-tick drains for the same card-click → detail-fetch sequence

The "save body includes matched_candidate_provider=metron" (line 198) and "save body includes matched_candidate_external_id=cv_id" (line 215) tests — also inside the METRON LAZY DETAIL describe block and using the same mountWithMetronDetailUrl() + card-click pattern — retain for (var i = 0; i < 8) await Promise.resolve() (post-click) and for (var i = 0; i < 16) await Promise.resolve() (post-card-click) drains. These are the same kind of timing-fragile loops that motivated this bead. They weren't converted. They are candidates for the same vi.waitFor treatment and remain a latent flake source for the same runner-load scenario. (Not a blocker for this PR since the bead scope is the three named tests, but worth noting as a follow-up.)

[MINOR] static/js/test/metadata_fetch_controller_comic.test.js:133 — mountWithMetronDetailUrl helper still has a fixed 8-tick drain

The mount helper itself at line 133 (for (var i = 0; i < 8; i++) { await Promise.resolve(); }) is a fixed-tick drain that lets the Stimulus controller connect and load providers before the test body runs. This is the same pattern as the removed drains and would fail under the same runner-load conditions if the controller requires >8 microtask ticks to settle. That said, mount-helper drains are typically more stable (no user-action sequencing), and this is outside the stated bead scope.

---

Summary

The three conversions are correct. The vi.waitFor conditions wait on the right observable (the fetch mock call history for candidate-detail), not a trivially-true condition. No behavior coverage was lost — the vi.waitFor throw semantics are strictly stronger than asserting after a fixed drain (it keeps polling until the condition holds or times out, whereas the old drain could silently pass with detailCall === undefined). No fake-timer/real-timer mismatch was introduced by this change. The only substantive issue is the redundant .toBeDefined() (MINOR), and two categories of un-converted sibling drains (MINOR).

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## Code Review — bookshelf-wro8e (PR #1337) **CI status observed:** JS Unit Tests: success (2m21s). Test Race, Coverage, Lint, Integration, E2E API all green. E2E Browser failing — this is the unrelated a11y flake being addressed in PR #1336, not this PR's concern. PR is `mergeable: True`. --- ### Phase 1: Spec compliance The bead goal is to eliminate the 3 flaky fixed-microtask-drain loops (`for i < 16`) in the METRON LAZY DETAIL "triggers detail fetch", "includes provider=metron", and "includes external_id" tests by replacing them with `vi.waitFor()`. All three conversions are present and correct in the diff. --- ### Phase 2: Code quality findings [MINOR] static/js/test/metadata_fetch_controller_comic.test.js:155 — `expect(detailCall).toBeDefined()` is now redundant after `vi.waitFor` In the first converted test (`clicking a metron candidate with external_id triggers detail fetch`), the `vi.waitFor` block already throws if `found` is falsy (the `if (!found) throw` guard), and returns the URL string. The subsequent `expect(detailCall).toBeDefined()` (line 155 on the branch) is therefore a tautology — `detailCall` cannot be `undefined` at that point because `vi.waitFor` would have timed out instead. The other two tests (`toContain("provider=metron")`, `toContain("external_id=12345")`) don't have this redundancy because they assert a specific value. Suggested fix: remove the standalone `expect(detailCall).toBeDefined()` assertion; the `vi.waitFor` condition is the real gate, or replace it with a meaningful content assertion (e.g. `toContain("candidate-detail")`). [MINOR] static/js/test/metadata_fetch_controller_comic.test.js:202-222 — Sibling "save body" tests in METRON LAZY DETAIL still use fixed-tick drains for the same card-click → detail-fetch sequence The "save body includes matched_candidate_provider=metron" (line 198) and "save body includes matched_candidate_external_id=cv_id" (line 215) tests — also inside the `METRON LAZY DETAIL` describe block and using the same `mountWithMetronDetailUrl()` + card-click pattern — retain `for (var i = 0; i < 8) await Promise.resolve()` (post-click) and `for (var i = 0; i < 16) await Promise.resolve()` (post-card-click) drains. These are the same kind of timing-fragile loops that motivated this bead. They weren't converted. They are candidates for the same `vi.waitFor` treatment and remain a latent flake source for the same runner-load scenario. (Not a blocker for this PR since the bead scope is the three named tests, but worth noting as a follow-up.) [MINOR] static/js/test/metadata_fetch_controller_comic.test.js:133 — `mountWithMetronDetailUrl` helper still has a fixed 8-tick drain The mount helper itself at line 133 (`for (var i = 0; i < 8; i++) { await Promise.resolve(); }`) is a fixed-tick drain that lets the Stimulus controller connect and load providers before the test body runs. This is the same pattern as the removed drains and would fail under the same runner-load conditions if the controller requires >8 microtask ticks to settle. That said, mount-helper drains are typically more stable (no user-action sequencing), and this is outside the stated bead scope. --- ### Summary The three conversions are correct. The `vi.waitFor` conditions wait on the right observable (the `fetch` mock call history for `candidate-detail`), not a trivially-true condition. No behavior coverage was lost — the `vi.waitFor` throw semantics are strictly stronger than asserting after a fixed drain (it keeps polling until the condition holds or times out, whereas the old drain could silently pass with `detailCall === undefined`). No fake-timer/real-timer mismatch was introduced by this change. The only substantive issue is the redundant `.toBeDefined()` (MINOR), and two categories of un-converted sibling drains (MINOR). REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor referenced this pull request from a commit 2026-08-05 02:19:21 +00:00

fix(js-test): address review minors — de-flake remaining METRON LAZY DETAIL tests (bookshelf-wro8e)

zombor force-pushed bd-bookshelf-wro8e from 35801e1fdc to 077680d4f2 2026-08-05 02:19:22 +00:00 Compare

zombor commented 2026-08-05 02:54:36 +00:00

Author

Owner

Copy link

Fold-delta re-review — PR #1337 (bookshelf-wro8e)

Reviewing ONLY the three addressed minors committed in 077680d on top of the previously-cleared f71cb03 base.

CI / Mergeability

• JS Unit Tests: success
• E2E Browser: failure (pre-existing move_toast flake being fixed separately by PR #1339 — not this PR's concern)
• All other contexts: success
• Mergeable: True

---

Finding-by-finding assessment

1. mountWithMetronDetailUrl helper — 8-tick drain → vi.waitFor(fetchButton enabled)

The converted condition (!btn || btn.disabled) is the correct observable: the helper exists solely to reach a state where the fetch button is enabled (providers have loaded), and that state is driven by a real DOM mutation the controller performs after its _loadProviders() fetch resolves. vi.waitFor here uses real timers (no useFakeTimers in scope at this point) and polls a genuine DOM state change. No deadlock risk. Correct conversion.

2. Save-body tests — fixed 8-tick + 16-tick drains → vi.waitFor(candidate-card) + vi.waitFor(save-button)

Both save-body tests (matched_candidate_provider=metron and matched_candidate_external_id=cv_id) now:

1. vi.waitFor on .candidate-card before clicking it — correct, that's the real signal the candidates render completed.
2. vi.waitFor on .mf-modal-save-row .btn before installing fake timers — this is the key ordering: the comment at line 231 ("vi.waitFor uses real timers so it must complete … before fake timers are installed") is accurate. vi.waitFor runs under real timers here; only after it resolves does vi.useFakeTimers() go in. After the save click the 8-tick drain continues under fake timers (suppressing the 800ms window.location.assign) then vi.useRealTimers() restores before the expect. The afterEach calls vi.clearAllTimers() + vi.restoreAllMocks() so timer state does not leak between tests.

The save-body assertions themselves are unchanged: expect(capturedBody.matched_candidate_provider).toBe("metron") and expect(capturedBody.matched_candidate_external_id).toBe(567890). Coverage is fully preserved.

3. Tautology fix — expect(detailCall).toBeDefined() → expect(detailCall).toContain("candidate-detail")

The vi.waitFor already guarantees found is truthy before returning it, so the original toBeDefined() was vacuously true. The replacement asserts the URL contains the string "candidate-detail", which is meaningful: it confirms the correct endpoint was called (not just that any fetch happened). Correct and stronger.

4. Remaining fixed-tick drains in the file

The drains remaining in the file (e.g. lines 235, 273, 287, etc.) are all in:

• Save-click sequences under fake timers — these are intentional: the 8-tick drain after click(saveBtn) is needed to flush the fetch microtask queue while fake timers are active; vi.waitFor cannot be used here because fake timers are installed. Correct to leave as-is.
• Non-METRON-LAZY-DETAIL describe blocks — those tests were not in scope for this PR and were not touched by this delta.

No new drains were introduced in the METRON LAZY DETAIL block.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Fold-delta re-review — PR #1337 (bookshelf-wro8e) Reviewing ONLY the three addressed minors committed in `077680d` on top of the previously-cleared `f71cb03` base. ### CI / Mergeability - **JS Unit Tests**: `success` - **E2E Browser**: `failure` (pre-existing `move_toast` flake being fixed separately by PR #1339 — not this PR's concern) - **All other contexts**: `success` - **Mergeable**: `True` --- ### Finding-by-finding assessment **1. `mountWithMetronDetailUrl` helper — 8-tick drain → `vi.waitFor(fetchButton enabled)`** The converted condition (`!btn || btn.disabled`) is the correct observable: the helper exists solely to reach a state where the fetch button is enabled (providers have loaded), and that state is driven by a real DOM mutation the controller performs after its `_loadProviders()` fetch resolves. `vi.waitFor` here uses real timers (no `useFakeTimers` in scope at this point) and polls a genuine DOM state change. No deadlock risk. Correct conversion. **2. Save-body tests — fixed 8-tick + 16-tick drains → `vi.waitFor(candidate-card)` + `vi.waitFor(save-button)`** Both save-body tests (`matched_candidate_provider=metron` and `matched_candidate_external_id=cv_id`) now: 1. `vi.waitFor` on `.candidate-card` before clicking it — correct, that's the real signal the candidates render completed. 2. `vi.waitFor` on `.mf-modal-save-row .btn` before installing fake timers — this is the key ordering: the comment at line 231 ("vi.waitFor uses real timers so it must complete … before fake timers are installed") is accurate. `vi.waitFor` runs under real timers here; only after it resolves does `vi.useFakeTimers()` go in. After the save click the 8-tick drain continues under fake timers (suppressing the 800ms `window.location.assign`) then `vi.useRealTimers()` restores before the `expect`. The `afterEach` calls `vi.clearAllTimers()` + `vi.restoreAllMocks()` so timer state does not leak between tests. The save-body assertions themselves are unchanged: `expect(capturedBody.matched_candidate_provider).toBe("metron")` and `expect(capturedBody.matched_candidate_external_id).toBe(567890)`. Coverage is fully preserved. **3. Tautology fix — `expect(detailCall).toBeDefined()` → `expect(detailCall).toContain("candidate-detail")`** The `vi.waitFor` already guarantees `found` is truthy before returning it, so the original `toBeDefined()` was vacuously true. The replacement asserts the URL contains the string `"candidate-detail"`, which is meaningful: it confirms the correct endpoint was called (not just that any fetch happened). Correct and stronger. **4. Remaining fixed-tick drains in the file** The drains remaining in the file (e.g. lines 235, 273, 287, etc.) are all in: - **Save-click sequences under fake timers** — these are intentional: the 8-tick drain after `click(saveBtn)` is needed to flush the `fetch` microtask queue while fake timers are active; `vi.waitFor` cannot be used here because fake timers are installed. Correct to leave as-is. - **Non-METRON-LAZY-DETAIL describe blocks** — those tests were not in scope for this PR and were not touched by this delta. No new drains were introduced in the METRON LAZY DETAIL block. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor referenced this pull request from a commit 2026-08-05 02:57:18 +00:00

fix(js-test): address review minors — de-flake remaining METRON LAZY DETAIL tests (bookshelf-wro8e)

zombor force-pushed bd-bookshelf-wro8e from 077680d4f2 to a2a38a3092 2026-08-05 02:57:19 +00:00 Compare

zombor merged commit ea8f8a5e15 into main 2026-08-05 03:04:26 +00:00

zombor referenced this pull request from a commit 2026-08-05 03:04:28 +00:00

Merge pull request 'fix(js-test): harden timing-fragile metron detail-fetch tests with vi.waitFor (bookshelf-wro8e)' (#1337) from bd-bookshelf-wro8e into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1337

No description provided.