Skip to content

test: harden three flaky test areas against CI races - #7342

Open
wpfleger96 wants to merge 3 commits into
mainfrom
duncan/harden-flaky-tests
Open

test: harden three flaky test areas against CI races#7342
wpfleger96 wants to merge 3 commits into
mainfrom
duncan/harden-flaky-tests

Conversation

@wpfleger96

Copy link
Copy Markdown
Member

Fix three independently-flaking test areas that caused the last four main merges to fail CI (runs 33891898259, 33895981950, 33896008491, 33901221965).

Status-expiry E2E (profile-custom-emoji-status.spec.ts)

The test seeded a status expiring 2 s ahead using wall-clock time, then raced the UI waiting for the expiry indicator to disappear. Under CI load the indicator never disappeared within the timeout.

Fix: install Playwright's fake clock at Date.now() + 3_000 (past the 2 s expiry) and call a new E2E bridge function (__BUZZ_E2E_EXPIRE_USER_STATUS_QUERIES__) that calls expireUserStatusQueries on the app's QueryClient. The fake Date.now() makes the dialog's expirationIsFuture guard re-evaluate to false on its next render; the cache sweep removes the sidebar indicator. No timers, no polling.

Workflow E2Es (workflow-local-controls.spec.ts, workflows.spec.ts)

One-shot boundingBox() reads during inspector-open transitions produced null or reversed coordinates, causing alternating assertion failures.

Fix: await all three operator buttons visible + waitForAnimations before reading geometry; wrap layout assertions in expect.poll to re-measure until stable. In workflows.spec.ts, await the "Trigger event" button visible before clicking it (the inspector may still be animating in after clicking "Trigger: Message Posted").

Rust cancellation test (fake_llm.rs)

h.send("session/cancel", ...) is request-like; releasing the round-2 gate before the cancel ACK arrived let round 2 win the race, producing stopReason: null.

Fix: drain stdout into a buffer until the cancel ACK is received, then release the gate. The cancel token is guaranteed set before round 2 can respond. Buffered messages are replayed through the same classification logic.

Pre-existing biome lint (utilities.css)

Add biome-ignore for dynamic-range-limit (emerging CSS property, Chromium 133+ / WebKit 26+) that was blocking the pre-commit hook for all committers. Unrelated to the flake fixes.


Local verification

Rust cancellation test — 25/25 iterations:

test cancelled_turn_with_usage_emits_notification_before_response ... ok  (×25)

Full hook suite passed clean on push (desktop-check, desktop-typecheck, desktop-test, desktop-tauri-checks, rust-tests).

Fix three independently-flaking test areas that caused the last four
main merges to fail CI.

**Status-expiry E2E (profile-custom-emoji-status.spec.ts)**
The test seeded a status expiring 2 s ahead then waited on wall-clock
time for it to disappear, racing slow CI workers. Fix: install
Playwright's fake clock at +3 s and call a new E2E bridge function
(__BUZZ_E2E_EXPIRE_USER_STATUS_QUERIES__) to sweep the query cache
immediately. The fake Date.now() makes the dialog's expirationIsFuture
re-evaluate to false on its next render; the cache sweep removes the
sidebar indicator. No timers, no polling.

**Workflow E2Es (workflow-local-controls.spec.ts, workflows.spec.ts)**
One-shot boundingBox() reads during inspector-open transitions produced
null or reversed coordinates. Fix: await all three operator buttons
visible + waitForAnimations before reading geometry; use expect.poll to
re-measure until the layout invariant holds. In workflows.spec.ts, await
the Trigger event button visible before clicking it (inspector may still
be animating in after the Trigger: Message Posted click).

**Rust cancellation test (fake_llm.rs)**
h.send(session/cancel) is request-like; releasing the round-2 gate
before the ACK arrived let round 2 win the race, producing stopReason:
null. Fix: drain stdout into a buffer until the cancel ACK is received,
then release the gate. Cancel token is set before round 2 can respond.
Replay buffered messages through the same classification logic.

**Pre-existing biome lint (utilities.css)**
Add biome-ignore for dynamic-range-limit (emerging CSS property,
Chromium 133+ / WebKit 26+) that was blocking the pre-commit hook for
all committers.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 requested a review from a team as a code owner September 4, 2026 19:33
@wpfleger96
wpfleger96 deployed to codex-review September 4, 2026 19:33 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: 4beffef6979347f7ebdc760705865bdd04d23508...a14d1e3c2afb6725ae4d4d47f162ff94de98fcf4
  • Model: gpt-5.6-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability issues found in the authorized PR range. Changes are limited to test synchronization, a test-only status-expiry hook, and a CSS lint suppression.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Static inspection only; repository scripts, builds, and tests were not executed as required.

Generated by Codex Security Review |
Requested by: @wpfleger96 |
Workflow run

…d, reopenWorkflow, openCreateWorkflow, openTriggerInspector

Replace four one-shot isVisible() reads in workflow-local-controls helpers
with poll-based state resolution:

- openCreateWorkflow: poll until channelList appears, clicking combobox when
  not yet visible, instead of a racy one-shot isVisible() check on mount
- openTriggerInspector: poll until Trigger event button appears, clicking the
  collapsed Trigger: header when needed, instead of one-shot isVisible()
  during tab-switch/dialog-mount transitions
- createEnabled: poll until confirmation dialog appears OR create dialog
  closes (narrow triggers skip the dialog entirely), then await explicit
  create dialog closure — removes the racy one-shot isVisible() that let
  execution proceed before state was determined
- reopenWorkflow: await the named card visible before clicking its action
  button; createEnabled() now guarantees dialog closure but the card render
  is async

These were the synchronization races Thufir identified as the root cause of
the workflow E2E historical failures in CI runs 33891898259/33896008491.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 4, 2026
@wpfleger96
wpfleger96 deployed to codex-review September 4, 2026 20:31 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 4, 2026
@TheSentinel454

Copy link
Copy Markdown
Contributor

Review feedback at exact head 5a3a393c57e52a621732382332eca9240c213313:

P1 — status-expiry E2E bypasses the production scheduler

desktop/tests/e2e/profile-custom-emoji-status.spec.ts installs a fake clock after app startup and directly invokes __BUZZ_E2E_EXPIRE_USER_STATUS_QUERIES__ through desktop/src/testing/e2eBridge.ts. That bridge calls the cache sweep with an artificial future timestamp, bypassing the production scheduleExpirationsetTimeout lifecycle in desktop/src/features/user-status/hooks.ts.

As a result, the test would still pass if the scheduler stopped arming or firing. The sweep already has unit coverage; this E2E should bind the production lifecycle seam, consistent with TESTING.md:25-31.

Suggested fix: install Playwright's clock before page.goto, seed a status expiring at fixed time +2s, fast-forward the clock, and observe automatic sidebar removal while preserving the unsaved draft. The E2E should fail if scheduleExpiration no longer invokes its timer.

P2 — cancel-ACK assertion is tautological

In crates/buzz-agent/tests/fake_llm.rs, the drain loop can exit only after observing the cancel response, after which saw_cancel_ok is assigned true. The later assert!(saw_cancel_ok, ...) therefore cannot fail and suggests independent coverage that the control flow already provides.

Please remove saw_cancel_ok and its assertion while retaining the ACK-gated loop. The cancellation ordering mechanism itself looks sound.

The workflow polling/readiness changes also look sound; they replace one-shot visibility reads with condition-based synchronization.

The prefix locator `[data-testid^="workflow-card-"]` matched both the
container div (`workflow-card-<id>`) and the child `<p
data-testid="workflow-card-name">`, causing Playwright strict-mode to
resolve to 2 elements and fail the `toBeVisible()` assertion in
`reopenWorkflow()` — deterministic CI failure in all 3 retries.

Scope to the container via `filter({ has: getByTestId("workflow-card-name")
.filter({ hasText: name }) })`, which uniquely selects the parent div
(the only element that *contains* the name child with the right text).

The previous local 12/12 pass came from a stale build that was serving
the pre-R1 spec without the new `toBeVisible()` call; the strict-mode
violation is deterministic in CI where the DOM structure is identical.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 4, 2026
@wpfleger96
wpfleger96 deployed to codex-review September 4, 2026 21:39 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Sep 4, 2026

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the expire-via-e2e-bridge + fake clock approach is solid. one hang risk: the cancel-ack drain is now an unbounded loop { recv } — if the agent never acks cancel the test wedges instead of failing the old 40-iter budget. worth a timeout or iter cap around that drain.

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poll helpers that click on every failed check can toggle the combobox/inspector if isVisible flickers. also the cancel-ack drain has no timeout now — if the ack never shows up that test hangs instead of failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants