[sadpandajoe]

Summary

Several frontend Jest tests hang indefinitely until the test runner times out. The hang is non-deterministic in some files (depends on test ordering, parallelism, and dependency-tree changes that shift React's render-scheduling timing) and consistent in others (every test in FileHandler/index.test.tsx times out at the per-test 60s limit, totalling ~7 minutes).

Root Cause

superset-frontend/spec/helpers/jsDomWithFetchAPI.ts patches out MessageChannel and MessagePort to work around an rc-overflow leak (#34871):

this.global.MessageChannel = undefined as any;
this.global.MessagePort = undefined as any;

With MessageChannel undefined, React's scheduler falls back to setTimeout for scheduling renders. This produces two distinct failure modes:

Mode 1 — Fake-timer deadlock

When a test calls jest.useFakeTimers(), React's setTimeout-scheduled renders sit in the fake timer queue and never fire (nothing advances the fake clock between renders). Two consequences:

1. waitFor deadlock — RTL's waitFor polls every 50ms expecting eventual consistency. With fake timers active and React renders blocked on a fake setTimeout, the polled assertion never becomes true, so waitFor runs until its 1s timeout (or the suite-level timeout), per test.
2. afterEach cleanup deadlock — an afterEach that does await act(async () => { await new Promise(r => setTimeout(r, 0)); }) to flush pending state will park forever if the prior test threw mid-flight while fake timers were still installed.

This is latent — it existed in the affected files for some time, but a recent dependency change (#37815, which pulled in @jsonforms/* and @rjsf/antd) shifted React/scheduler timing enough to expose it.

Mode 2 — Synchronous-consumer-during-useEffect deadlock

FileHandler/index.test.tsx doesn't use fake timers. Its hang has a different mechanism: setupLaunchQueue mocks window.launchQueue.setConsumer to call the async consumer synchronously inside the component's useEffect. The consumer's state setters (setUploadFile, setUploadType, …) then fire on a microtask continuation during React's commit phase. With React's scheduler on the setTimeout fallback, this combination deadlocks Jest — every test times out at its 60s limit (~7 minutes total). #37815 removed the macrotask defer that previously avoided this; PR #38430 had originally added it.

Fix

Mode 1 fixes — SemanticLayerModal.test.tsx, SelectFilterPlugin.test.tsx, and 12 additional fake-timer test files (Select.test.tsx, TimezoneSelector*.test.tsx, QueryAutoRefresh.test.tsx, SaveDatasetModal.test.tsx, Header.test.tsx, FilterBar.test.tsx, TreeInitialization.test.tsx, TreeSelection.test.tsx, TimeGrainPreFilter.test.tsx, CustomFrame.test.tsx, VizTypeControl.test.tsx): pass { advanceTimers: true } to every jest.useFakeTimers() call. This auto-advances the fake clock based on real-time elapsed, so React's scheduled setTimeout-based renders fire and waitFor can observe state changes. Manual jest.advanceTimersByTime(...) for explicit debounce control still works on top of the auto-advance.

Mode 1 cleanup robustness — DatasetList.test.tsx: reorder the afterEach so jest.useRealTimers() runs before the await act flush. If a test throws mid-flight and leaves fake timers installed, cleanup no longer deadlocks.

Mode 2 fix — FileHandler/index.test.tsx: restore the macrotask defer in setupLaunchQueue's mock so the async consumer is called via setTimeout(..., 0) instead of synchronously. State setters then fire after React's commit phase finishes. Test runtime: 7+ min hang → 2.7s.

Test Plan

• SemanticLayerModal.test.tsx — passes locally
• DatasetList.test.tsx — passes locally; cleanup is robust to mid-test throws
• SelectFilterPlugin.test.tsx — 33/33 tests pass locally in 15.9s
• FileHandler/index.test.tsx — 10/10 tests pass locally in 2.7s (was hanging ~7min)
• Full CI passes without per-shard hangs

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes a frontend unit test hang caused by the interaction between Jest fake timers and React’s fallback scheduling when MessageChannel is unavailable in the JSDOM environment.

Changes:

• Enable auto-advancing fake timers in SemanticLayerModal.test.tsx to allow React render scheduling to proceed while still using fake timers for the debounce.
• Reorder DatasetList.test.tsx cleanup to restore real timers before flushing pending React updates, preventing a potential afterEach deadlock when a test fails while fake timers are active.

Reviewed changes

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

File	Description
superset-frontend/src/pages/DatasetList/DatasetList.test.tsx	Restores real timers before the async act() flush in afterEach to avoid hangs when fake timers are left enabled.
superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx	Uses jest.useFakeTimers({ advanceTimers: true }) to prevent the modal test from hanging indefinitely under the patched JSDOM environment.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.82%. Comparing base (cfb0b6e) to head (6bc5277).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #39957      +/-   ##
==========================================
- Coverage   63.86%   63.82%   -0.05%     
==========================================
  Files        2584     2589       +5     
  Lines      136674   137801    +1127     
  Branches    31513    31919     +406     
==========================================
+ Hits        87290    87952     +662     
- Misses      47868    48333     +465     
  Partials     1516     1516              

Flag	Coverage Δ
javascript	66.51% <ø> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[aminghadersohi]

PR #39957 — fix(tests): prevent jest hangs caused by MessageChannel-mocked React scheduler

HEAD SHA: 4a28fdb

Review mode: Code (all 16 changed files are TypeScript test files — no Python, no architecture changes)

---

Scan Coverage

All 14 automated scans were run. Scans 1–5, 8–9, 11, 13–14 are Python-specific and do not apply to TypeScript test files.

• Scan 1 (inline imports): N/A (TypeScript)
• Scan 2 (config.get() with defaults): N/A (Python)
• Scan 3 (f-string logging): N/A (Python)
• Scan 4 (broad exception handling): N/A (Python)
• Scan 5 (sensitive info in error messages): N/A (test files)
• Scan 6 (module-level mutable globals): 1 match — pendingTimerIds in FileHandler/index.test.tsx:118. Acceptable: it's a test file and afterEach at line 162–163 clears it after every test.
• Scan 7 (print/console.log statements): 0 violations across all 16 changed files.
• Scan 8 (db.session.commit): N/A (Python/ORM)
• Scan 9 (JWT decode without verification): N/A
• Scan 10 (hardcoded values): 1 match — magic number 50 at FileHandler/index.test.tsx:144 (max poll iterations). See NIT below.
• Scan 11 (missing type hints on test methods): N/A (TypeScript)
• Scan 12 (any type usage): 2 pre-existing matches — window as any at FileHandler/index.test.tsx:158 and 164. See NIT below.
• Scan 13 (module-level app context references): N/A (Python/Flask)
• Scan 14 (SQL injection / RLS bypass): N/A (test files)

---

MEDIUM (should fix)

• [FileHandler/index.test.tsx:151] savedConsumer?.(params) silently does nothing when the consumer never registered — the ?. optional-chaining swallows the case where useEffect hasn't fired after all 50 polling iterations. Any test that calls triggerConsumer() expecting consumer behavior would then fail with a confusing RTL waitFor timeout rather than an explicit assertion. Prefer: if (!savedConsumer) throw new Error('LaunchQueue consumer was never registered after polling'); followed by await savedConsumer(params) (no optional chain).

• [TimezoneSelector.test.tsx:27-28] [TimezoneSelector.DaylightSavingTime.test.tsx:27-28] advanceTimers: true makes Date.now() drift from the value set by jest.setSystemTime(): as real time passes during the test, the fake clock advances with it. This changes the semantics from a pinned clock to a drifting one. For the current tested dates (2022-01-01, 2022-07-01) the drift is a few milliseconds and is well inside DST-transition margins — the tests pass in practice. But it is a latent trap: a future test that pins a date and assumes Date.now() returns exactly that value will silently get a drifting result. Consider pairing setSystemTime tests with advanceTimers: false (the original behavior before this PR) and only using advanceTimers: true in tests that actually need React's scheduler to fire. The DST tests do not need React renders to proceed (they load the component then check DOM), so advanceTimers: false + explicit jest.runAllTimers() would work without semantic drift.

---

NIT (non-blocking)

• [FileHandler/index.test.tsx:144] Magic number 50 for the max polling iterations. Name it: const MAX_CONSUMER_POLL_ATTEMPTS = 50; at module level alongside pendingTimerIds. This also makes the comment at line 141–142 redundant (the constant name carries the intent).

• [FileHandler/index.test.tsx:205-206] test.skip('handles CSV file correctly') is still skipped with an eslint-disable. The PR fixes the Mode 2 deadlock that was blocking all FileHandler tests; the macrotask defer in setupLaunchQueue should now let the CSV test run. Consider re-enabling and verifying — it would close the only gap in file-type coverage (CSV via the auto-trigger path).

• [FileHandler/index.test.tsx:158, 164] delete (window as any).launchQueue uses a less type-safe cast than the structured (window as unknown as Window & { launchQueue: LaunchQueue }) already at line 127 in setupLaunchQueue. Minor consistency issue — both casts are pre-existing; only the new afterEach lines (162–163) were added by this PR, but the surrounding pattern is worth harmonizing.

---

PRAISE

• Root cause analysis (PR description): Distinguishing Mode 1 (fake-timer deadlock via waitFor + React scheduler on setTimeout fallback) from Mode 2 (synchronous consumer during useEffect deadlocking Jest's event loop) is genuinely rigorous. The PR description is one of the better root-cause write-ups in this codebase.

• [DatasetList.test.tsx:44-58] The afterEach reordering — jest.useRealTimers() before await act() — is the correct fix and the comment explains exactly why the order matters. Future readers will understand the invariant without needing to rediscover it.

• [FileHandler/index.test.tsx:118-135, 162-163] The pendingTimerIds lifecycle is handled correctly: add to set on creation (line 135), remove on fire (line 132), cancel all remaining in afterEach (lines 162–163). No timer leaks possible even when tests fail mid-flight.

• [FileHandler/index.test.tsx:120-122] The explanatory comment documenting WHY the macrotask defer is needed — specifically naming the jsDomWithFetchAPI MessageChannel mock and how React falls back to setTimeout — is the kind of non-obvious constraint that saves the next engineer from reverting this "unnecessary" async wrapper.

• Intentional asymmetry in DatasetList (lines 71, 94): Leaving those two loading-state tests with jest.useFakeTimers() (no advanceTimers) is the right call. Those tests deliberately freeze the clock to prevent a 10-second setTimeout-gated response from resolving mid-assertion. Adding advanceTimers: true would let real elapsed time advance the fake clock and potentially resolve the delayed response before the loading state assertion fires. Good judgment call.

• Not touching jsDomWithFetchAPI.ts: The root cause is the MessageChannel patch. The PR correctly treats it as load-bearing (fixing the rc-overflow memory leak from issue #34871) and addresses the hang at the test layer instead of removing the patch. This is the right architectural boundary.

• Regression chain (PR description): Explicitly referencing #37815 (dep upgrade that exposed the hang), #34871 (original MessageChannel patch), and #38430 (prior macrotask-defer PR) gives reviewers and future bisectors a complete causal chain.

---

Summary

Solid, well-understood fix for a real infrastructure bug. The three-mode taxonomy in the PR description (Mode 1 fake-timer deadlock, Mode 1 cleanup deadlock, Mode 2 synchronous consumer) is correct and the fixes match. The two MEDIUM items are: (1) a silent-failure path in triggerConsumer that could produce confusing test failures instead of clear errors, and (2) a semantic drift introduced by pairing advanceTimers: true with setSystemTime in the timezone tests — benign today, latent trap for future authors. No blockers.

[codeant-ai-for-open-source]

Suggestion: Enabling fake timers inside the test without guaranteed teardown can leak timer mode if the test throws before the end, which will make later tests in this file run under fake timers unexpectedly. Move timer restoration to an afterEach or a try/finally around the test body so useRealTimers is always executed. [missing cleanup]

Severity Level: Major ⚠️

- ❌ Failing 'typing and deleting...' test leaves Jest in fake-timer mode.
- ⚠️ Subsequent Select tests may hang or behave nondeterministically.

Steps of Reproduction ✅

1. Run the Jest suite that includes
`superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx`; when
the test `"typing and deleting the last character for a new option displays correctly"` at
lines 1162-1183 executes, it calls `jest.useFakeTimers({ advanceTimers: true })` at line
1163.

2. Inside this test, timers are used via `jest.runAllTimers()` at lines 1169 and 1175 and
are only restored with `jest.useRealTimers()` at line 1182; there is no surrounding
`try/finally` or file-level `afterEach` that guarantees cleanup (verified in the file
header at lines 19-149).

3. If the test throws or fails (for example, by changing the expectations at lines
1177-1180 so they no longer match the component behavior), Jest aborts the test before
reaching `jest.useRealTimers()` at line 1182, leaving the environment in fake-timer mode.

4. Subsequent tests in `Select.test.tsx`, such as the grouped-options search tests
starting around lines 85-119 in the tail of the file, now run under fake timers
unexpectedly, which can deadlock React Testing Library `waitFor` calls or change render
scheduling, reproducing the timer-mode leakage the suggestion describes.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx
**Line:** 1163:1163
**Comment:**
	*Missing Cleanup: Enabling fake timers inside the test without guaranteed teardown can leak timer mode if the test throws before the end, which will make later tests in this file run under fake timers unexpectedly. Move timer restoration to an `afterEach` or a `try/finally` around the test body so `useRealTimers` is always executed.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[codeant-ai-for-open-source]

Suggestion: Fake timers are turned on at module scope but this file does not flush pending timers or restore real timers between tests, so queued callbacks can leak across test cases and introduce order-dependent flakiness/timeouts. Add per-test teardown (runOnlyPendingTimers + useRealTimers, then re-enable as needed) instead of a one-time global switch. [missing cleanup]

Severity Level: Critical 🚨

- ❌ SaveDatasetModal tests share fake timers across entire file.
- ❌ AsyncSelect debounce timers can leak into later test cases.
- ⚠️ Flaky Jest runs with order-dependent timeouts and hangs.

Steps of Reproduction ✅

1. When Jest loads
`superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx`, it
executes the module-level `jest.useFakeTimers({ advanceTimers: true })` at line 48
immediately after the `fetchMock.get` setup at lines 43-46, enabling fake timers for every
test in this file.

2. In the test `"renders the overwrite button as disabled until an existing dataset is
selected"` at lines 132-168, clicking the AsyncSelect combobox at line 149 schedules
debounced fetch timers, and the test explicitly flushes timers with `act(async () => {
jest.runAllTimers(); })` at lines 152-155.

3. There is no `afterEach` or other lifecycle hook that calls
`jest.runOnlyPendingTimers()` or `jest.useRealTimers()` (the only `beforeEach` at lines
52-55 clears the `useSelector` mock and calls `cleanup()`), so if this test or any earlier
test throws before its `jest.runAllTimers()` block, or if React schedules timers after
that block (e.g., due to the global MessageChannel-to-setTimeout fallback), those
fake-timer callbacks remain queued globally.

4. Later tests such as `"clears dataset cache when creating new dataset"` at lines 158-185
rely on React Testing Library's `waitFor` at lines 183-185, which uses timers; because
fake timers are globally enabled and pending callbacks can leak across tests, asynchronous
work can execute in the wrong test or not be flushed at all, causing nondeterministic
timeouts and order-dependent failures in this suite.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx
**Line:** 48:48
**Comment:**
	*Missing Cleanup: Fake timers are turned on at module scope but this file does not flush pending timers or restore real timers between tests, so queued callbacks can leak across test cases and introduce order-dependent flakiness/timeouts. Add per-test teardown (`runOnlyPendingTimers` + `useRealTimers`, then re-enable as needed) instead of a one-time global switch.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[codeant-ai-for-open-source]

Suggestion: This file enables fake timers globally but never restores real timers or drains the timer queue in teardown, so asynchronous work scheduled by one test can execute during another and create nondeterministic failures. Add timer cleanup in afterEach and scope fake timers per test or per suite lifecycle. [missing cleanup]

Severity Level: Critical 🚨

- ❌ VizTypeControl tests always run under global fake timers.
- ⚠️ Async React renders can spill into subsequent tests' assertions.
- ⚠️ Test failures may cascade causing nondeterministic suite behavior.

Steps of Reproduction ✅

1. When Jest loads
`superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeControl.test.tsx`,
it runs the top-level `jest.useFakeTimers({ advanceTimers: true })` at line 48 immediately
after the `jest.mock('scroll-into-view-if-needed')` at lines 45-46, installing fake timers
for all tests in this file.

2. The `"VizTypeControl"` describe block at lines 92-194 defines `waitForRenderWrapper` at
lines 104-115, which wraps React Testing Library's `waitFor` around `render`; multiple
tests (`"Fast viz switcher tiles render"` at lines 122-163, `"Multi Chart appears with
custom icon when selected"` at 165-182, and others) call this helper and therefore rely on
timers for React's async updates and AntD icon effects.

3. The `afterEach` at lines 117-120 only calls `cleanup()` and `jest.clearAllMocks()` and
never calls `jest.runOnlyPendingTimers()` or `jest.useRealTimers()`, so any timers
scheduled during a test remain in the shared fake-timer queue across tests, and timer mode
is never reset back to real timers.

4. If one of these tests fails mid-execution or leaves asynchronous work pending (for
example, due to a UI change affecting expectations), those fake-timer callbacks can fire
while the next test runs or remain unflushed, leading to cross-test interference, spurious
act() warnings, and potential hangs in later `VizTypeControl` tests that depend on
`waitForRenderWrapper`.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeControl.test.tsx
**Line:** 48:48
**Comment:**
	*Missing Cleanup: This file enables fake timers globally but never restores real timers or drains the timer queue in teardown, so asynchronous work scheduled by one test can execute during another and create nondeterministic failures. Add timer cleanup in `afterEach` and scope fake timers per test or per suite lifecycle.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[codeant-ai-for-open-source]

Suggestion: Enabling fake timers at module scope without per-test teardown makes timer state shared across all tests in this file. Pending timeouts/intervals from one test can bleed into the next test and cause flaky assertions or hangs, especially with React/AntD deferred work. Scope fake timers per test lifecycle (beforeEach + afterEach) and clear/restore timers after each test. [logic error]

Severity Level: Major ⚠️

- ⚠️ Native filter bar tests can become flaky across runs.
- ⚠️ Cross-test timer bleed complicates debugging timer-related regressions.
- ⚠️ Jest suite may intermittently hang due to uncleared timers.

Steps of Reproduction ✅

1. Run the dashboard filter bar tests, for example with `jest
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx`,
which uses the custom JSDOM environment configured in
`superset-frontend/jest.config.js:38-39` pointing to
`spec/helpers/jsDomWithFetchAPI.ts:20-42` (this environment disables `MessageChannel` so
React falls back to `setTimeout`-based scheduling).

2. When Jest loads `FilterBar.test.tsx`, the module-level call `jest.useFakeTimers({
advanceTimers: true });` at
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:42`
enables fake timers for the entire file; only `jest.clearAllMocks()` is run in
`beforeEach` at lines 80-83, and there is no corresponding `afterEach` that calls
`jest.clearAllTimers()` or `jest.useRealTimers()`, so timer state persists across tests.

3. Several tests mount `<FilterBar />` via `renderFilterBar` (e.g., `test('FilterBar
renders dividers with title and description', ...)` at lines 351-381 and `test('FilterBar
does not crash when filter has value but empty extraFormData', ...)` around lines 30-61 in
the tail of the file), which exercise the component logic in
`src/dashboard/components/nativeFilters/FilterBar/index.tsx:97-158` where
`publishDataMask` is defined using `lodash.debounce(...)` and invoked from a `useEffect`
at lines 134-140, scheduling debounced work on the fake timer queue without explicitly
flushing or cancelling it in the tests.

4. Later tests in the same file use `jest.advanceTimersByTime(...)` inside `act`, for
example at `FilterBar.test.tsx:370-371`, 51-52, 84-85, 127-128, 185-186, and 249-250,
which will advance all pending fake timers in the shared queue, including any debounced
`publishDataMask` callbacks left over from previous tests; because fake timers were
enabled once at module scope and never restored, these cross-test timer side effects can
cause flaky assertions (unexpected URL/history updates) or hangs when leftover timers keep
scheduling work that no test explicitly drains, and this behavior can be reproduced by
repeatedly running the suite and observing nondeterministic failures.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx
**Line:** 42:42
**Comment:**
	*Logic Error: Enabling fake timers at module scope without per-test teardown makes timer state shared across all tests in this file. Pending timeouts/intervals from one test can bleed into the next test and cause flaky assertions or hangs, especially with React/AntD deferred work. Scope fake timers per test lifecycle (`beforeEach` + `afterEach`) and clear/restore timers after each test.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it