codeant-ai-for-open-source[bot] commented on code in PR #39957:
URL: https://github.com/apache/superset/pull/39957#discussion_r3211378254
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx:
##########
@@ -1160,7 +1160,7 @@ test('does not fire onChange if the same value is
selected in single mode', asyn
// Reference for the bug this tests:
https://github.com/apache/superset/pull/33043#issuecomment-2809419640
test('typing and deleting the last character for a new option displays
correctly', async () => {
- jest.useFakeTimers();
+ jest.useFakeTimers({ advanceTimers: true });
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Failing 'typing and deleting...' test leaves Jest in fake-timer mode.
- ⚠️ Subsequent Select tests may hang or behave nondeterministically.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fcomponents%2FSelect%2FSelect.test.tsx%0A%2A%2ALine%3A%2A%2A%201163%3A1163%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20Enabling%20fake%20timers%20inside%20the%20test%20without%20guaranteed%20teardown%20can%20leak%20timer%20mode%20if%20the%20test%20throws%20before%20the%20end%2C%20which%20will%20make%20later%20tests%20in%20this%20file%20run%20under%20fake%20timers%20unexpectedly.%20Move%20timer%20restoration%20to%20an%20%60afterEach%60%20or%20a%20%60try%2Ffinally%60%20around%20the%20test%20body%20so%20%60useRealTimers%60%20is%20always%20executed.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix
%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fcomponents%2FSelect%2FSelect.test.tsx%0A%2A%2ALine%3A%2A%2A%201163%3A1163%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20Enabling%20fake%20timers%20inside%20the%20test%20without%20guaranteed%20teardown%20can%20leak%20timer%20mode%20if%20the%20test%20throws%20before%20the%20end%2C%20which%20will%20make%20later%20tests%20in%20this%20file%20run%20under%20fake%20timers%20unexpectedly.%20Move%20timer%20restoration%20to%20an%20%60afterEach%60
%20or%20a%20%60try%2Ffinally%60%20around%20the%20test%20body%20so%20%60useRealTimers%60%20is%20always%20executed.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39957&comment_hash=48e1fe327a2f5d03cb89a40ed46a10d169833b1f382e05ef4048517fd104bb79&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39957&comment_hash=48e1fe327a2f5d03cb89a40ed46a10d169833b1f382e05ef4048517fd104bb79&reaction=dislike'>👎</a>
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx:
##########
@@ -45,7 +45,7 @@ fetchMock.get('glob:*/api/v1/dataset/?*', {
dataset_count: 3,
});
-jest.useFakeTimers();
+jest.useFakeTimers({ advanceTimers: true });
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2FSqlLab%2Fcomponents%2FSaveDatasetModal%2FSaveDatasetModal.test.tsx%0A%2A%2ALine%3A%2A%2A%2048%3A48%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20Fake%20timers%20are%20turned%20on%20at%20module%20scope%20but%20this%20file%20does%20not%20flush%20pending%20timers%20or%20restore%20real%20timers%20between%20tests%2C%20so%20queued%20callbacks%20can%20leak%20across%20test%20cases%20and%20introduce%20order-dependent%20flakiness%2Ftimeouts.%20Add%20per-test%20teardown%20%28%60runOnlyPendingTimers%60%20%2B%20%60useRealTimers%60%2C%20then%20re-enable%20as%20needed%29%20instead%20of%20a%20one-time%20global%20switch.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make
%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2FSqlLab%2Fcomponents%2FSaveDatasetModal%2FSaveDatasetModal.test.tsx%0A%2A%2ALine%3A%2A%2A%2048%3A48%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20Fake%20timers%20are%20turned%20on%20at%20module%20scope%20but%20this%20file%20does%20not%20flush%20pending%20timers%20or%20restore%20real%20timers%20between%20tests%2C%20so%20queued%20callbacks%20can%20leak%20across%20test%20cases%20and%20introduce%20order-dependent%20flakiness%2Ftimeouts.%20Ad
d%20per-test%20teardown%20%28%60runOnlyPendingTimers%60%20%2B%20%60useRealTimers%60%2C%20then%20re-enable%20as%20needed%29%20instead%20of%20a%20one-time%20global%20switch.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39957&comment_hash=c824e33ca5413cc6d4df0e5c39757f1787d55293661458597fa3818225c78682&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39957&comment_hash=c824e33ca5413cc6d4df0e5c39757f1787d55293661458597fa3818225c78682&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeControl.test.tsx:
##########
@@ -45,7 +45,7 @@ import VizTypeControl, { VIZ_TYPE_CONTROL_TEST_ID } from
'./index';
// Mock scrollIntoView to avoid errors in test environment
jest.mock('scroll-into-view-if-needed', () => jest.fn());
-jest.useFakeTimers();
+jest.useFakeTimers({ advanceTimers: true });
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ 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.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2Fcontrols%2FVizTypeControl%2FVizTypeControl.test.tsx%0A%2A%2ALine%3A%2A%2A%2048%3A48%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20This%20file%20enables%20fake%20timers%20globally%20but%20never%20restores%20real%20timers%20or%20drains%20the%20timer%20queue%20in%20teardown%2C%20so%20asynchronous%20work%20scheduled%20by%20one%20test%20can%20execute%20during%20another%20and%20create%20nondeterministic%20failures.%20Add%20timer%20cleanup%20in%20%60afterEach%60%20and%20scope%20fake%20timers%20per%20test%20or%20per%20suite%20lifecycle.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20
also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fexplore%2Fcomponents%2Fcontrols%2FVizTypeControl%2FVizTypeControl.test.tsx%0A%2A%2ALine%3A%2A%2A%2048%3A48%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20This%20file%20enables%20fake%20timers%20globally%20but%20never%20restores%20real%20timers%20or%20drains%20the%20timer%20queue%20in%20teardown%2C%20so%20asynchronous%20work%20scheduled%20by%20one%20test%20can%20execute%20during%20another%20and%20create%20nondeterministic%20failures.%20Add%20timer%20cleanup%20in%20%60afterEach%60%20and%20sco
pe%20fake%20timers%20per%20test%20or%20per%20suite%20lifecycle.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39957&comment_hash=b388f928cad36ef90c192189e5ec4c3ea3be34ea2d863c7cbcfda635faca1472&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39957&comment_hash=b388f928cad36ef90c192189e5ec4c3ea3be34ea2d863c7cbcfda635faca1472&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
