Skip to content

test(ui): fix timing flake in SettingsPage Dynamic Tools tests#1931

Merged
itomek-amd merged 4 commits into
mainfrom
fix/settingspage-test-flake
Jul 6, 2026
Merged

test(ui): fix timing flake in SettingsPage Dynamic Tools tests#1931
itomek-amd merged 4 commits into
mainfrom
fix/settingspage-test-flake

Conversation

@itomek

@itomek itomek commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1933.

The Agent UI Vitest job could fail on unrelated PRs: SettingsPage.test.tsx > is disabled and reflects the env value when locked flaked on CI (Dependabot #1802 run 28763820070 and #1803) while passing on identical code elsewhere, eroding trust in a red check. The tests waited for the toggle to exist, but it renders from first paint (unchecked, disabled until settings load), so assertions and clicks raced the async getSettings() flush — clicks on the still-disabled toggle silently no-op. Five of the seven tests carried the race; CI load determined whether it fired. After: the tests wait for the settled state (toggle enabled, or the lock note for the locked case), and the API mocks resolve one macrotask later — like real HTTP — so the race stays permanently exposed instead of masked. (No env-var or cross-file state leakage was involved; the "env value" in the test name is the GAIA_DYNAMIC_TOOLS lock note rendered by the UI.)

Proof

Race reproduced deterministically on unfixed code by giving the mocks a realistic 30 ms response latency (kept in this PR as the regression guard) — including the exact assertion that failed in CI:

❯ src/components/__tests__/SettingsPage.test.tsx (7 tests | 5 failed)
   × renders the loaded value (off by default)
   × toggles when the visible row/label is clicked, not just the hidden input
   × is disabled and reflects the env value when locked
   × guards against a double-click while a save is in flight (single PUT)
   × reverts and surfaces an error when the save fails (no silent fallback)

With the fix, stable under shuffle and repetition (run in src/gaia/apps/webui):

$ for seed in 1 42 777 20260706 999983; do npx vitest run --sequence.shuffle --sequence.seed=$seed; done
seed=1         Tests  109 passed (109)
seed=42        Tests  109 passed (109)
seed=777       Tests  109 passed (109)
seed=20260706  Tests  109 passed (109)
seed=999983    Tests  109 passed (109)

15 consecutive runs of the file: 0 failures. npx tsc --noEmit clean. Also fixed: getSystemStatus was mocked as null, throwing a TypeError inside the component in every test's stderr; it now returns a valid SystemStatus.

Test plan

  • cd src/gaia/apps/webui && npx vitest run — 109/109 pass
  • npx vitest run --sequence.shuffle (any seed) — 109/109 pass
  • Vitest CI job green on this PR

The SettingsPage Dynamic Tools tests waited for the toggle's presence,
but the toggle renders unconditionally from first paint (unchecked,
disabled until settingsLoaded), so waitFor(getDynamicToolsToggle)
resolved before the mocked getSettings() flushed into React state.
Assertions and userEvent clicks (a no-op on a disabled input) then
raced the async settings load — a race the immediately-resolved mocks
usually masked locally but that fired under CI load ('is disabled and
reflects the env value when locked' failed on runs of #1802/#1803).

Fix: wait for the settled state instead of presence — an enabled
toggle for the unlocked tests, the GAIA_DYNAMIC_TOOLS lock note for
the locked test. The API mocks now resolve on a 30ms macrotask, like
a real HTTP response, so the race stays exposed: before the fix that
delay failed 5/7 tests deterministically. Also mock getSystemStatus
with a valid SystemStatus instead of 'null as never', which threw a
TypeError inside the component on every test.
@itomek

itomek commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

CI evidence: the full "Test Electron Framework and Apps" workflow — including the "Test Agent UI Components (Vitest)" job this PR fixes — is green on this branch: run 28820012621 (manually dispatched after updating the branch from main).

🔍 Why a manual dispatch, and history of the earlier red run

@itomek itomek self-assigned this Jul 6, 2026
@itomek itomek marked this pull request as ready for review July 6, 2026 19:22
@itomek itomek requested a review from kovtcharov-amd as a code owner July 6, 2026 19:22
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Verdict: Approve

This is a clean, test-only fix for a real CI flake (#1933) in the SettingsPage Dynamic Tools tests. The old tests waited for the toggle merely to exist — but it renders from first paint (disabled until getSettings() resolves), so assertions and clicks raced the async load and clicks on the still-disabled toggle silently no-op'd. The fix waits for the settled state instead (toggle enabled, or the lock note for the locked case) and gives the API mocks realistic one-macrotask latency so the race stays permanently exposed rather than masked. No blocking issues; production code is untouched.

The root-cause writeup and the deterministic 30 ms repro (kept in-tree as the regression guard) are exactly what a flake fix should carry, and it also quietly fixes the getSystemStatus mock that was throwing a TypeError in every test's stderr.

🔍 Technical details

Verified against the component:

  • SettingsPage.tsx:485disabled={!settingsLoaded || savingDynamicTools || dynamicToolsLocked} confirms the new findLoadedDynamicToolsToggle() helper (not.toBeDisabled()) is a correct proxy for "settings landed" in the unlocked/not-saving path.
  • The locked test correctly can't use that helper (the toggle stays disabled when locked) and instead awaits screen.findByText(/GAIA_DYNAMIC_TOOLS/), which only renders after settings load — right call.
  • makeSystemStatus() supplies every required field of SystemStatus (types/index.ts:400); the remaining fields (init_state, init_tasks, detected_devices) are optional, consistent with the PR's "tsc --noEmit clean" claim.

Strengths:

  • 🟢 Fixes the actual mechanism, not the symptom — waiting on settled UI state rather than adding sleeps/retries, and keeping the injected latency as a standing regression guard so the race can't silently return.
  • 🟢 Incidental correctness win: replacing the null as never getSystemStatus mock with a valid object removes a TypeError that was firing in every test.
  • 🟢 Scope-clean and well-evidenced (shuffle across five seeds + 15 repeat runs).

No issues found.

@itomek

itomek commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Live real-surface verification of the behavior the flaky test asserts — real Agent UI (built frontend + gaia.ui.server) started with GAIA_DYNAMIC_TOOLS=1:

  • GET /api/settings{"custom_model":null,"model_status":null,"context_size":null,"agent_mode":"autonomous","dynamic_tools":true,"dynamic_tools_locked":true}
  • Settings page renders the Dynamic Tools toggle on and disabled, with the lock note: "Controlled by GAIA_DYNAMIC_TOOLS — unset that environment variable to change this here."
  • Clicking the toggle/label while locked changes nothing and issues no PUT /api/settings (network log shows only GETs) — the exact no-op the test suite relies on.

Tested by driving the served UI in a headless browser against the locked backend; screenshot captured in-session.

@itomek-amd itomek-amd added this pull request to the merge queue Jul 6, 2026
Merged via the queue into main with commit c26836b Jul 6, 2026
32 checks passed
@itomek-amd itomek-amd deleted the fix/settingspage-test-flake branch July 6, 2026 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky Agent UI Vitest test: SettingsPage Dynamic Tools races the async settings load

2 participants