Skip to content

Comments

test(dashboard): eliminate race condition in FiltersConfigModal test#35717

Merged
sadpandajoe merged 1 commit intomasterfrom
fix-flakey-filters-config-modal-test
Oct 22, 2025
Merged

test(dashboard): eliminate race condition in FiltersConfigModal test#35717
sadpandajoe merged 1 commit intomasterfrom
fix-flakey-filters-config-modal-test

Conversation

@sadpandajoe
Copy link
Member

@sadpandajoe sadpandajoe commented Oct 17, 2025

SUMMARY

Fixes a race condition in the "validates the pre-filter value" test that caused intermittent failures and required a 50-second timeout.

Root Cause:
The test used synchronous userEvent.click() calls followed by jest.runOnlyPendingTimers(), creating a race
condition where:

  • User interactions didn't complete before the timer flush
  • Validation timers scheduled by user-event might not execute
  • Test would fail intermittently looking for validation messages

Solution:

  • Await userEvent calls: Ensures clicks complete before runAllTimers() executes (v12-compatible async approach)
  • Use runAllTimers(): Changed from runOnlyPendingTimers() to execute ALL queued timers including debounced handlers
  • try/finally cleanup: Guarantees timer restoration even if test fails
  • Explicit waitFor(): Safety net for async validation promises

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@sadpandajoe @claude
fix(dashboard): eliminate race condition in FiltersConfigModal test
cda7458 
The "validates the pre-filter value" test had a race condition that caused
intermittent failures and required a 50-second timeout.

**Root Cause:**
- Test used synchronous `userEvent.click()` calls
- Immediately called `jest.runOnlyPendingTimers()` (wrong method)
- User interactions didn't complete before timer flush
- Created race where validation timers might not execute

**Fix:**
- Await `userEvent.click()` calls (v12-compatible async approach)
- Use `jest.runAllTimers()` instead of `runOnlyPendingTimers()`
- Wrap timer manipulation in try/finally for cleanup
- Add explicit `waitFor()` after restoring timers

**Results:**
- Test now passes consistently (3/3 runs)
- Runs in ~25-29s vs 50s timeout
- No regressions (16 passed, 2 skipped)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
korbit-ai[bot]
korbit-ai bot approved these changes Oct 17, 2025
View reviewed changes
Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

@dosubot dosubot bot added change:frontend Requires changing the frontend dashboard:editmode Related to te Dashboard edit mode labels Oct 17, 2025
@sadpandajoe sadpandajoe changed the title fix(dashboard): eliminate race condition in FiltersConfigModal test test(dashboard): eliminate race condition in FiltersConfigModal test Oct 17, 2025
@yousoph yousoph added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Oct 21, 2025
@github-actions github-actions bot added 🎪 cda7458 🚦 building Environment cda7458 status: building 🎪 cda7458 📅 2025-10-21T23-15 Environment cda7458 created at 2025-10-21T23-15 🎪 cda7458 ⌛ 48h Environment cda7458 expires after 48h 🎪 cda7458 🤡 yousoph Environment cda7458 requested by yousoph and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Oct 21, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 21, 2025

🎪 Showtime is building environment on GHA for cda7458

@github-actions github-actions bot added 🎪 cda7458 🚦 deploying Environment cda7458 status: deploying 🎪 cda7458 🚦 running Environment cda7458 status: running 🎪 🎯 cda7458 Active environment pointer - cda7458 is receiving traffic 🎪 cda7458 🌐 52.27.129.129:8080 Environment cda7458 URL: http://52.27.129.129:8080 (click to visit) and removed 🎪 cda7458 🚦 building Environment cda7458 status: building 🎪 cda7458 🚦 deploying Environment cda7458 status: deploying 🎪 cda7458 🚦 running Environment cda7458 status: running 🎪 🎯 cda7458 Active environment pointer - cda7458 is receiving traffic labels Oct 21, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 21, 2025

🎪 Showtime deployed environment on GHA for cda7458

• Environment: http://52.27.129.129:8080 (admin/admin)
• Lifetime: 48h auto-cleanup
• Updates: New commits create fresh environments automatically

@sadpandajoe sadpandajoe merged commit d094212 into master Oct 22, 2025
93 of 97 checks passed
@sadpandajoe sadpandajoe deleted the fix-flakey-filters-config-modal-test branch October 22, 2025 21:56
sadpandajoe added a commit that referenced this pull request Nov 7, 2025
@sadpandajoe @claude
fix(tests): add await to userEvent calls to fix race conditions
f71b2e4 
- PropertiesModal.test.tsx: add await to 18+ userEvent calls
- FiltersConfigModal.test.tsx: add await to 26 userEvent calls
- Fix anti-pattern: remove userEvent from waitFor blocks
- Fix cache_timeout test expectation to match API string type
- Resolves race condition causing PropertiesModal test failures

Root cause: Tests written in 2021 with synchronous userEvent pattern
became problematic as components gained more async complexity.
Recent changes (PR #33392) made race conditions consistently reproducible.

Follows pattern from PR #35717, #35918, and DatasourceControl fixes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@sadpandajoe sadpandajoe mentioned this pull request Nov 7, 2025
Merged
9 tasks
sadpandajoe added a commit that referenced this pull request Nov 7, 2025
@sadpandajoe @claude
fix(tests): add await to userEvent calls to fix race conditions
a97a8c3 
- PropertiesModal.test.tsx: add await to 18+ userEvent calls
- FiltersConfigModal.test.tsx: add await to 26 userEvent calls
- Fix anti-pattern: remove userEvent from waitFor blocks
- Fix cache_timeout test expectation to match API string type
- Resolves race condition causing PropertiesModal test failures

Root cause: Tests written in 2021 with synchronous userEvent pattern
became problematic as components gained more async complexity.
Recent changes (PR #33392) made race conditions consistently reproducible.

Follows pattern from PR #35717, #35918, and DatasourceControl fixes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eschutho eschutho eschutho approved these changes

@justinpark justinpark Awaiting requested review from justinpark

+2 more reviewers

@aminghadersohi aminghadersohi aminghadersohi approved these changes

@korbit-ai korbit-ai[bot] korbit-ai[bot] approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

🎪 cda7458 🚦 running Environment cda7458 status: running 🎪 cda7458 🤡 yousoph Environment cda7458 requested by yousoph 🎪 cda7458 ⌛ 48h Environment cda7458 expires after 48h 🎪 cda7458 🌐 52.27.129.129:8080 Environment cda7458 URL: http://52.27.129.129:8080 (click to visit) 🎪 cda7458 📅 2025-10-21T23-15 Environment cda7458 created at 2025-10-21T23-15 change:frontend Requires changing the frontend dashboard:editmode Related to te Dashboard edit mode size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sadpandajoe @aminghadersohi @eschutho @yousoph