codeant-ai-for-open-source[bot] commented on code in PR #40036:
URL: https://github.com/apache/superset/pull/40036#discussion_r3221614808
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx:
##########
@@ -356,21 +356,31 @@ describe('SaveDatasetModal', () => {
const setupOverwriteFlow = async () => {
// Select the "Overwrite existing" radio
- userEvent.click(screen.getByRole('radio', { name: /overwrite existing/i
}));
- // Open the select and pick an existing dataset
- userEvent.click(
- screen.getByRole('combobox', { name: /existing dataset/i }),
+ await userEvent.click(
+ screen.getByRole('radio', { name: /overwrite existing/i }),
);
- await waitFor(() =>
- expect(screen.queryByText('Loading...')).not.toBeVisible(),
+ // Open the select to load existing-dataset options
+ await userEvent.click(
+ screen.getByRole('combobox', { name: /existing dataset/i }),
);
- userEvent.click(screen.getAllByText('coolest table 0')[1]);
+ // Advance timers to flush debounced fetches in AsyncSelect
+ await act(async () => {
+ jest.runAllTimers();
+ });
+ // Wait for the loading indicator to clear
+ await waitFor(() => {
+ const loading = screen.queryByText('Loading...');
+ expect(loading === null || !loading.checkVisibility()).toBe(true);
+ });
+ // Pick an existing dataset (use the listbox item, not the input mirror)
+ const options = await screen.findAllByText('coolest table 0');
+ await userEvent.click(options[1]);
Review Comment:
**Suggestion:** This still assumes there will always be at least two
matching nodes for the dataset label, but `findAllByText` only guarantees one
match before resolving. If only one node is rendered (for example when the
input mirror is not present or DOM ordering changes), `options[1]` is
`undefined` and the click throws, reintroducing flaky failures. Select the
option by role/text within the listbox (or assert `options.length > 1` before
indexing) instead of hardcoding index `1`. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Overwrite template_params tests can crash with TypeError.
- ⚠️ CI for SaveDatasetModal overwrite flow becomes flaky.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the SaveDatasetModal tests via `npx jest
src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx` in
`superset-frontend`
(tests defined in
`superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx:67-160`
and `38-65` for `setupOverwriteFlow`).
2. Inside the test file, note that
`fetchMock.get('glob:*/api/v1/dataset/?*', { result:
mockdatasets, ... })` at `SaveDatasetModal.test.tsx:43-46` wires the
AsyncSelect in
`SaveDatasetModal` (see `SaveDatasetModal/index.tsx:214-225`) to return
options built from
`mockdatasets` (`fixtures.ts:19-30`, where `table_name` is `coolest table 0`
for the first
dataset).
3. The tests `"sends template_params when overwriting a dataset with include
template
parameters checked"` and `"does not send template_params when overwriting a
dataset with
include template parameters unchecked"` at
`SaveDatasetModal.test.tsx:67-115` and
`117-159` both call `await setupOverwriteFlow();`, which in turn executes
the helper at
`SaveDatasetModal.test.tsx:38-65`. After opening the combobox and waiting
for loading to
clear, `setupOverwriteFlow` runs `const options = await
screen.findAllByText('coolest
table 0');` and then `await userEvent.click(options[1]);` at lines 57-58
(hunk lines
376-377).
4. React Testing Library's `findAllByText` only guarantees that it resolves
once at least
one matching element exists; if the AsyncSelect DOM for this label ever
renders a single
accessible text node (for example, only the listbox option and no separate
"input mirror",
as is consistent with the AsyncSelect behavior tested in
`packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx:18-20`
where only
one option node is expected), then `options.length === 1`, `options[1]` is
`undefined`,
and `userEvent.click(options[1])` throws a TypeError, causing these
overwrite/template-parameter tests to fail. This brittleness is specific to
the hardcoded
`[1]` index; selecting by role/text within the listbox or asserting
`options.length > 1`
before indexing would avoid the crash.
```
</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%20376%3A377%0A%2A%2AComment%3A%2A%2A%0A%09%2AType%20Error%3A%20This%20still%20assumes%20there%20will%20always%20be%20at%20least%20two%20matching%20nodes%20for%20the%20dataset%20label%2C%20but%20%60findAllByText%60%20only%20guarantees%20one%20match%20before%20resolving.%20If%20only%20one%20node%20is%20rendered%20%28for%20example%20when%20the%20input%20mirror%20is%20not%20present%20or%20DOM%20ordering%20changes%29%2C%20%60options%5B1%5D%60%20is%20%60undefined%60%20and%20the%20click%20throws%2C%20reintroducing%20flaky%20failures.%20Select%20the%20option%20by%20role%2Ftext%20within%20the%20listbox%20%28or%20assert%20%60options.length%20%3E%201%60%20before%20indexing%29%20instead%20of%20hardcoding%20index%20%601%60.%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%20376%3A377%0A%2A%2AComment%3A%2A%2A%0A%09%2AType%20Error%3A%20This%20still%20assumes%20there%20will%20always%20be%20at%20least%20two%20matching%20nodes%20for%20the%20datase
t%20label%2C%20but%20%60findAllByText%60%20only%20guarantees%20one%20match%20before%20resolving.%20If%20only%20one%20node%20is%20rendered%20%28for%20example%20when%20the%20input%20mirror%20is%20not%20present%20or%20DOM%20ordering%20changes%29%2C%20%60options%5B1%5D%60%20is%20%60undefined%60%20and%20the%20click%20throws%2C%20reintroducing%20flaky%20failures.%20Select%20the%20option%20by%20role%2Ftext%20within%20the%20listbox%20%28or%20assert%20%60options.length%20%3E%201%60%20before%20indexing%29%20instead%20of%20hardcoding%20index%20%601%60.%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%2
0the%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:** 376:377
**Comment:**
*Type Error: This still assumes there will always be at least two
matching nodes for the dataset label, but `findAllByText` only guarantees one
match before resolving. If only one node is rendered (for example when the
input mirror is not present or DOM ordering changes), `options[1]` is
`undefined` and the click throws, reintroducing flaky failures. Select the
option by role/text within the listbox (or assert `options.length > 1` before
indexing) instead of hardcoding index `1`.
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%2F40036&comment_hash=3eb81c82c953b522685abacdc1d2d25a26d2484c15516e1cb1a8ee52c3fca3ea&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40036&comment_hash=3eb81c82c953b522685abacdc1d2d25a26d2484c15516e1cb1a8ee52c3fca3ea&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]
