codeant-ai-for-open-source[bot] commented on code in PR #38563:
URL: https://github.com/apache/superset/pull/38563#discussion_r2924138550
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx:
##########
@@ -146,15 +147,21 @@ describe('SaveDatasetModal', () => {
// Click the overwrite select component
const select = screen.getByRole('combobox', { name: /existing dataset/i
})!;
- userEvent.click(select);
+ await userEvent.click(select);
- await waitFor(() =>
- expect(screen.queryByText('Loading...')).not.toBeVisible(),
- );
+ // Advance timers to flush debounced fetches in AsyncSelect
+ await act(async () => {
+ jest.runAllTimers();
+ });
+
+ await waitFor(() => {
+ const loading = screen.queryByText('Loading...');
+ expect(loading === null || !loading.checkVisibility()).toBe(true);
+ });
Review Comment:
**Suggestion:** The assertion uses `loading.checkVisibility()`, but
`checkVisibility` is not reliably implemented in Jest/JSDOM environments and
can throw a runtime `TypeError` when the loading element exists. Use a Testing
Library assertion that checks removal from the DOM instead of calling
browser-only visibility APIs. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SaveDatasetModal overwrite tests can fail in CI.
- ⚠️ React 18 migration test suite becomes flaky.
- ❌ PR validation may fail on frontend pipeline.
```
</details>
```suggestion
await waitFor(() => {
expect(screen.queryByText('Loading...')).not.toBeInTheDocument();
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run frontend Jest tests that include
`superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx`
(matched by `superset-frontend/jest.config.js:23` testRegex).
2. Jest uses custom JSDOM environment from
`superset-frontend/jest.config.js:38` and
`spec/helpers/jsDomWithFetchAPI.ts:20-43`; no `checkVisibility` polyfill is
added in setup
(`spec/helpers/setup.ts:19-40`).
3. In tests `renders the overwrite button...` and `renders a confirm
overwrite screen...`,
the callback at `SaveDatasetModal.test.tsx:157-160` (also `190-192`)
evaluates
`loading.checkVisibility()` when `screen.queryByText('Loading...')` returns
an element.
4. When that branch executes, JSDOM can throw `TypeError:
loading.checkVisibility is not a
function`, failing/flaking these overwrite-flow tests.
```
</details>
<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:** 157:160
**Comment:**
*Type Error: The assertion uses `loading.checkVisibility()`, but
`checkVisibility` is not reliably implemented in Jest/JSDOM environments and
can throw a runtime `TypeError` when the loading element exists. Use a Testing
Library assertion that checks removal from the DOM instead of calling
browser-only visibility APIs.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38563&comment_hash=7e58dbae65c27b6ee5489273e344a23cb66a4f755f3c8bce3c4fce4879f89bb8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38563&comment_hash=7e58dbae65c27b6ee5489273e344a23cb66a4f755f3c8bce3c4fce4879f89bb8&reaction=dislike'>👎</a>
##########
superset-frontend/src/features/roles/RoleListEditModal.test.tsx:
##########
@@ -145,6 +145,17 @@ describe('RoleListEditModal', () => {
render(<RoleListEditModal {...mockProps} />);
+ // Wait for user hydration to complete before submitting
+ await waitFor(() => {
+ expect(
+ (SupersetClient.get as jest.Mock).mock.calls.some(([call]) =>
+ call.endpoint.includes('/api/v1/security/users/'),
+ ),
+ ).toBe(true);
+ });
+ // Allow the setFieldsValue effect to fire after loading state updates
+ await waitFor(() => Promise.resolve());
Review Comment:
**Suggestion:** The added `waitFor(() => Promise.resolve())` is a no-op and
does not actually wait for hydration to finish, so the test can still submit
before `setFieldsValue` updates `roleUsers`, causing flaky failures. Replace it
with a real observable assertion that confirms hydrated user values are
rendered before clicking save. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Flaky role edit submission test in CI.
- ⚠️ React 18 timing changes amplify race windows.
- ❌ User-ID mapping assertion intermittently fails.
```
</details>
```suggestion
await waitFor(() => {
expect(screen.getByText('johndoe')).toBeInTheDocument();
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `RoleListEditModal` test `submit maps {value,label} form values to
numeric ID
arrays` in `superset-frontend/src/features/roles/RoleListEditModal.test.tsx`
(render at
line 146, submit click at line 163).
2. The test only waits for `SupersetClient.get` call presence (lines
149-155), then
executes `await waitFor(() => Promise.resolve())` (line 157), which does not
assert any
hydration-visible condition.
3. In `superset-frontend/src/features/roles/RoleListEditModal.tsx`, user
form hydration
happens later in `useEffect` at lines 215-226 via
`formRef.current.setFieldsValue({
roleUsers: ... })`, after `loadingRoleUsers` flips false in
`fetchPaginatedData` finally
block (`superset-frontend/src/utils/fetchOptions.ts:103-112`).
4. If save is clicked before that effect commits, `handleFormSubmit`
(`RoleListEditModal.tsx:280-289`) reads `values.roleUsers` as empty and calls
`updateRoleUsers(id, [])`, causing intermittent failure of expected `[5, 7]`
assertion in
the test.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/roles/RoleListEditModal.test.tsx
**Line:** 156:157
**Comment:**
*Logic Error: The added `waitFor(() => Promise.resolve())` is a no-op
and does not actually wait for hydration to finish, so the test can still
submit before `setFieldsValue` updates `roleUsers`, causing flaky failures.
Replace it with a real observable assertion that confirms hydrated user values
are rendered before clicking save.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38563&comment_hash=1036dca4dc7647b6d5fb9557f2a81b318e85d845fc45c3cae4c5765303150eb8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38563&comment_hash=1036dca4dc7647b6d5fb9557f2a81b318e85d845fc45c3cae4c5765303150eb8&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]
