fix(tests): fix flakey tests with PropertiesModal.test.tsx, FiltersConfigModal.test.tsx and ChartList.listview.test.tsx

[sadpandajoe]

SUMMARY

This PR fixes several React Testing Library anti-patterns in test files that were causing race conditions, test failures, and timer leaks. The changes follow React Testing Library best practices and eliminate potential bug-masking issues.

Changes made:

PropertiesModal.test.tsx:

• Removed double-await pattern where findBy* queries were wrapped in waitFor (redundant waiting)
• Moved userEvent calls outside waitFor callbacks to prevent multiple firings during retry loops
• Removed unnecessary afterEach sleep that added overhead without meaningful benefit

FiltersConfigModal.test.tsx:

• Removed global 60-second timeout that was masking slow tests
• Fixed timer handling in "validates the pre-filter value" test with proper try/finally cleanup
• Fixed timer leak in "modifies the name of a filter" test that caused subsequent tests to timeout
• Updated assertion to use getAllByText() for handling multiple validation error messages

Why these changes matter:

1. Eliminates bug-masking: userEvent calls inside waitFor loops can fire multiple times during retries, potentially
   hiding real timing bugs
2. Prevents timer leaks: Tests that don't restore real timers cause subsequent tests to hang
3. Follows best practices: Aligns with React Testing Library recommendations and patterns used in recent fixes (PR
   test(dashboard): eliminate race condition in FiltersConfigModal test #35717, test(useThemeMenuItems): fix race conditions by awaiting all userEvent calls #35918)
4. Improves test isolation: Proper cleanup ensures tests don't interfere with each other

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

[korbit-ai]

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

[Copilot]

Pull Request Overview

This PR modernizes test patterns in two test files by adopting async/await conventions for userEvent interactions and refactoring asynchronous query patterns to follow Testing Library best practices.

Key changes:

• Convert all userEvent calls to async/await pattern for proper test sequencing
• Replace waitFor + getByRole/getByText patterns with findByRole/findByText for cleaner async queries
• Add try-finally blocks around fake timer usage to ensure cleanup
• Fix test assertion to match actual API response type (cache_timeout as string)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx	Removed unused afterEach cleanup hook, converted userEvent to async/await, refactored waitFor blocks to use findBy queries, and corrected cache_timeout type expectation from number to string
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx	Removed global test timeout, converted userEvent to async/await, refactored waitFor blocks, added proper try-finally blocks for fake timers, and improved test code organization

[Copilot]

Using an async callback function inside waitFor is an anti-pattern. The waitFor utility expects synchronous checks that can be repeatedly executed until they pass. An async function inside waitFor can lead to unpredictable behavior.

Instead, await the user interaction before waitFor, and only include the synchronous expectation inside:

await userEvent.click(screen.getByText(DATASET_REGEX));
await userEvent.click(screen.getByText('birth_names'));
await waitFor(() => {
  expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument();
});

[sadpandajoe]

This test is skipped so we can omit for now

[korbit-ai]

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

[sadpandajoe]

Looks like #33791 fixed some of the flakiness. This is just additional cleanup.

[Copilot]

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

[Copilot]

The checkbox validation after selection has been moved outside of waitFor(), which could cause race conditions. The checkboxes should be queried and validated inside a waitFor() to ensure the UI has updated after the click event. This validation was previously at line ~656-661 inside a waitFor block.

[Copilot]

The checkbox validation after deselection has been moved outside of waitFor(), which could cause race conditions. The checkboxes should be queried and validated inside a waitFor() to ensure the UI has updated after the deselect action. This validation was previously at line ~622-628 inside a waitFor block.

Suggested change

	checkboxes = screen.getAllByRole('checkbox');
	checkboxes.forEach(checkbox => {
	expect(checkbox).not.toBeChecked();
	await waitFor(() => {
	const checkboxes = screen.getAllByRole('checkbox');
	checkboxes.forEach(checkbox => {
	expect(checkbox).not.toBeChecked();
	});

[Copilot]

The expectation for the input value should be wrapped in waitFor() to ensure the user input has been processed. The userEvent.type() is async but the DOM update might not be immediate. Consider: await waitFor(() => expect(cacheTimeout).toHaveValue('1000'));

Suggested change

	expect(cacheTimeout).toHaveValue('1000');
	await waitFor(() => expect(cacheTimeout).toHaveValue('1000'));

[sadpandajoe]

There are two awaits before this, so this should be fine.

[Copilot]

The expectation for the input value should be wrapped in waitFor() to ensure the user input has been processed. The userEvent.type() is async but the DOM update might not be immediate. Consider: await waitFor(() => expect(certifiedBy).toHaveValue('Test certified by'));

Suggested change

	expect(certifiedBy).toHaveValue('Test certified by');
	await waitFor(() => expect(certifiedBy).toHaveValue('Test certified by'));

[sadpandajoe]

this should be fine as we have await for the two steps above this.

[sadpandajoe]

the steps before are already awaiting, so this should be fine.

[Copilot]

The expectation for the input value should be wrapped in waitFor() to ensure the user input has been processed. The userEvent.type() is async but the DOM update might not be immediate. Consider: await waitFor(() => expect(certificationDetails).toHaveValue('Test certification details'));

Suggested change

	expect(certificationDetails).toHaveValue('Test certification details');
	await waitFor(() => expect(certificationDetails).toHaveValue('Test certification details'));

[sadpandajoe]

this should be fine as we have await for the two steps above this.

[Copilot]

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:1

• Corrected test description from '"Save" button should call only "onSave"' to '"Save" button should call "onSave" and "onHide"' to accurately reflect what the test validates.

/**