Skip to content

Conversation

@sadpandajoe
Copy link
Member

SUMMARY

Fixed race conditions in useThemeMenuItems test suite by adding await to all 20 unawaited userEvent calls (16 hover + 4 click). This prevents intermittent test failures caused by queries executing before async user interactions complete.

  • Awaited all userEvent.hover() calls (16 occurrences)
  • Awaited all userEvent.click() calls (4 occurrences)
  • Follows established pattern from commit 4932f3522f (DatasourceControl fix)
  • Aligns with React Testing Library best practices

BEFORE/AFTER

Before:

userEvent.hover(await screen.findByRole('menuitem'));
const menu = await findMenuWithText('Light');

After:

await userEvent.hover(await screen.findByRole('menuitem'));
const menu = await findMenuWithText('Light');

TESTING INSTRUCTIONS

Run the test suite multiple times to verify consistency:

npm run test -- useThemeMenuItems.test.tsx
npm run test -- useThemeMenuItems.test.tsx
npm run test -- useThemeMenuItems.test.tsx

Expected result: All 20 tests pass consistently across multiple runs with no race condition warnings.

Validation performed:

  • ✅ 4 consecutive test runs: 20/20 tests passing each time
  • ✅ No "unmounted component" warnings
  • ✅ All pre-commit hooks passing

ADDITIONAL INFORMATION

This fix addresses potential flakiness by ensuring that async hover and click actions complete before subsequent queries and assertions execute. The @testing-library/user-event library returns Promises for all user interactions, which must be awaited to prevent race conditions.

Similar fixes have been applied across the codebase (28+ test files properly await userEvent calls). This change brings useThemeMenuItems.test.tsx in line with established testing patterns.

🤖 Generated with Claude Code

@bito-code-review
Copy link
Contributor

bito-code-review bot commented Oct 30, 2025

Code Review Agent Run #84dada

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 49653de..49653de
    • superset-frontend/src/hooks/useThemeMenuItems.test.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

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

…race conditions

Fixed 20 unawaited userEvent calls (16 hover + 4 click) that could cause race conditions
between user interactions and assertions. This ensures menu rendering completes before
queries execute, preventing intermittent test failures.

Pattern follows commit 4932f3522f which fixed the same issue in DatasourceControl tests.

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

Co-Authored-By: Claude <noreply@anthropic.com>
@sadpandajoe sadpandajoe force-pushed the fix-use-theme-menu-test branch from 49653de to 3eccd57 Compare October 30, 2025 21:45
@bito-code-review
Copy link
Contributor

Interaction Diagram by Bito
sequenceDiagram
participant TestRunner as Test Runner
participant TestFile as useThemeMenuItems.test.tsx<br/>🔄 Updated | ●●○ Medium
participant UserEvent as userEvent (Testing Library)<br/>●●○ Medium
participant ScreenAPI as screen (Testing Library)
participant Hook as useThemeMenuItems Hook
participant Component as TestComponent
Note over TestFile: All userEvent.hover() and<br/>userEvent.click() calls now awaited
TestRunner->>TestFile: Execute test suite
TestFile->>Component: renderThemeMenu()
Component->>Hook: useThemeMenuItems(props)
Hook-->>Component: Return MenuItem
TestFile->>UserEvent: await userEvent.hover(menuitem)
UserEvent-->>TestFile: Hover action completed
TestFile->>ScreenAPI: findByRole/queryByText
ScreenAPI-->>TestFile: Return menu element
TestFile->>UserEvent: await userEvent.click(element)
UserEvent-->>TestFile: Click action completed
TestFile->>TestRunner: Assert results
Loading

Critical path: Test Runner -> useThemeMenuItems.test.tsx -> userEvent (Testing Library) -> screen (Testing Library) -> Hook

Note: All userEvent async operations (hover, click) now properly await their promises. This ensures test stability by waiting for DOM updates before assertions. No changes to the actual hook implementation; only test synchronization improved.

@rusackas rusackas merged commit 5224347 into master Nov 3, 2025
62 checks passed
@rusackas rusackas deleted the fix-use-theme-menu-test branch November 3, 2025 18:28
sadpandajoe added a commit that referenced this pull request Nov 3, 2025
…t calls (#35918)

Co-authored-by: Claude <noreply@anthropic.com>
(cherry picked from commit 5224347)
@sadpandajoe sadpandajoe added v6.0 Label added by the release manager to track PRs to be included in the 6.0 branch 6.0:checkpoint and removed 6.0:checkpoint labels Nov 3, 2025
sadpandajoe added a commit that referenced this pull request Nov 7, 2025
- 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 added a commit that referenced this pull request Nov 7, 2025
- 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

Labels

size/M v6.0 Label added by the release manager to track PRs to be included in the 6.0 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants