Skip to content

test(webui): deflake SettingsPage dynamic-tools toggle tests#1938

Closed
itomek wants to merge 2 commits into
mainfrom
fix-settings-toggle-flake
Closed

test(webui): deflake SettingsPage dynamic-tools toggle tests#1938
itomek wants to merge 2 commits into
mainfrom
fix-settings-toggle-flake

Conversation

@itomek

@itomek itomek commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Fixes #1937.

Two SettingsPage tests raced the settings fetch: they grabbed the dynamic-tools toggle as soon as it rendered (disabled + unchecked while loading) and immediately asserted its final state, which fails on slow CI runners — seen on PR #1930's "Test Agent UI Components (Vitest)" run against code that doesn't touch the UI. Both now wait for the loaded state inside waitFor, the pattern the file's other five tests already use. Test-only change; no component code touched.

Test plan

  • cd src/gaia/apps/webui && npx vitest run src/components/__tests__/SettingsPage.test.tsx → 7 passed (verified 3× in a row locally)
  • npx vitest run (full webui suite) → 13 files / 109 tests passed

Two dynamic-tools tests grabbed the toggle as soon as it existed and
immediately asserted its final state. The toggle renders disabled and
unchecked while getSettings is in flight, so slow CI runners asserted
against the loading state ("element is not checked ... disabled").
Wait for the loaded state inside waitFor, as the file's other five
tests already do.
@itomek itomek self-assigned this Jul 6, 2026
@itomek itomek marked this pull request as ready for review July 6, 2026 20:05
@itomek itomek requested a review from kovtcharov-amd as a code owner July 6, 2026 20:05
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Verdict: Approve

This deflakes two SettingsPage toggle tests that grabbed the dynamic-tools control while it was still loading (disabled + unchecked) and asserted its final state immediately, so slow CI runners lost the race. Both now wait for a loaded-state signal before asserting — the same waitFor pattern the file's other five tests already use. Test-only, no component code touched. No blocking issues; safe to merge.

🔍 Technical details

Correctness of the wait conditions — both new predicates correctly discriminate the loaded state from the loading state, which is the whole point:

  • renders the loaded value (off by default) (line 54): loading = disabled+unchecked, loaded = enabled+unchecked. Waiting on not.toBeDisabled() is the right discriminator (enabled only happens post-load), then asserting not.toBeChecked(). ✅
  • is disabled and reflects the env value when locked (line 100): loading = disabled+unchecked, loaded = disabled+checked. Here disabled is true in both states, so waiting on it would be a no-op race — waiting on toBeChecked() is correct, then asserting toBeDisabled(). ✅

Switching from a captured toggle handle to re-querying via getDynamicToolsToggle() on each assertion is also correct here, since the label flips between "Enable/Disable dynamic tools" across states and the helper resolves either.

Strengths

  • Root cause fixed, not papered over — the tests now assert on the state transition rather than element existence, and the fix is consistent with the file's established waitFor(() => expect(...)) pattern.
  • The two WHY comments (lines 52–53, 98–99) are one-liners explaining the race, matching the repo's "short or skip" comment guidance.
  • Correctly identifies that toBeChecked() (not toBeDisabled()) is the load discriminator for the locked case — an easy detail to get wrong.

@itomek itomek enabled auto-merge July 6, 2026 20:10
@itomek

itomek commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded — main already fixed this flake more comprehensively in #1931 (findLoadedDynamicToolsToggle helper + loaded-state waits across the describe block), which is what this branch now conflicts with. Nothing here survives the rebase.

@itomek itomek closed this Jul 6, 2026
auto-merge was automatically disabled July 6, 2026 21:42

Pull request was closed

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 Vitest: SettingsPage dynamic-tools tests race the settings fetch (checked/disabled asserted before load)

2 participants