Copilot commented on code in PR #35993:
URL: https://github.com/apache/superset/pull/35993#discussion_r2493058376
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -551,39 +578,39 @@ test('should allow creating new metrics in dataset
editor', async () => {
});
// Open datasource menu and click edit dataset
- userEvent.click(screen.getByTestId('datasource-menu-trigger'));
- userEvent.click(await screen.findByTestId('edit-dataset'));
+ await userEvent.click(screen.getByTestId('datasource-menu-trigger'));
+ await userEvent.click(await screen.findByTestId('edit-dataset'));
// Wait for modal to appear and navigate to Metrics tab
await waitFor(() => {
expect(screen.getByText('Metrics')).toBeInTheDocument();
});
- userEvent.click(screen.getByText('Metrics'));
+ await userEvent.click(screen.getByText('Metrics'));
// Click add new metric button
- await waitFor(() => {
+ await waitFor(async () => {
const addButton = screen.getByTestId('crud-add-table-item');
expect(addButton).toBeInTheDocument();
- userEvent.click(addButton);
+ await userEvent.click(addButton);
});
Review Comment:
Using an async callback in `waitFor` is an anti-pattern. The `waitFor`
utility expects synchronous callbacks and handles retries internally. User
interactions should happen outside of `waitFor` blocks. Refactor to: `await
waitFor(() => {
expect(screen.getByTestId('crud-add-table-item')).toBeInTheDocument(); });
await userEvent.click(screen.getByTestId('crud-add-table-item'));`
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -551,39 +578,39 @@ test('should allow creating new metrics in dataset
editor', async () => {
});
// Open datasource menu and click edit dataset
- userEvent.click(screen.getByTestId('datasource-menu-trigger'));
- userEvent.click(await screen.findByTestId('edit-dataset'));
+ await userEvent.click(screen.getByTestId('datasource-menu-trigger'));
+ await userEvent.click(await screen.findByTestId('edit-dataset'));
// Wait for modal to appear and navigate to Metrics tab
await waitFor(() => {
expect(screen.getByText('Metrics')).toBeInTheDocument();
});
- userEvent.click(screen.getByText('Metrics'));
+ await userEvent.click(screen.getByText('Metrics'));
// Click add new metric button
- await waitFor(() => {
+ await waitFor(async () => {
const addButton = screen.getByTestId('crud-add-table-item');
expect(addButton).toBeInTheDocument();
- userEvent.click(addButton);
+ await userEvent.click(addButton);
});
// Find and fill in the metric name
- await waitFor(() => {
+ await waitFor(async () => {
const nameInput = screen.getByTestId('textarea-editable-title-input');
expect(nameInput).toBeInTheDocument();
- userEvent.clear(nameInput);
- userEvent.type(nameInput, newMetricName);
+ await userEvent.clear(nameInput);
+ await userEvent.type(nameInput, newMetricName);
});
// Save the modal
- userEvent.click(screen.getByTestId('datasource-modal-save'));
+ await userEvent.click(screen.getByTestId('datasource-modal-save'));
// Confirm the save
- await waitFor(() => {
+ await waitFor(async () => {
const okButton = screen.getByText('OK');
expect(okButton).toBeInTheDocument();
- userEvent.click(okButton);
+ await userEvent.click(okButton);
});
Review Comment:
Using an async callback in `waitFor` is an anti-pattern. User interactions
should not be performed inside `waitFor` blocks. Refactor to: `const okButton =
await screen.findByText('OK'); await userEvent.click(okButton);`
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -26,15 +26,26 @@ import {
act,
userEvent,
waitFor,
+ cleanup,
} from 'spec/helpers/testing-library';
import { fallbackExploreInitialData } from 'src/explore/fixtures';
+import type { DatasetObject } from 'src/features/datasets/types';
import DatasourceControl from '.';
const SupersetClientGet = jest.spyOn(SupersetClient, 'get');
+let originalLocation: Location;
+
+beforeEach(() => {
+ originalLocation = window.location;
+});
+
afterEach(() => {
+ window.location = originalLocation;
+ cleanup();
Review Comment:
The explicit `cleanup()` call is unnecessary. React Testing Library
automatically cleans up after each test since version 9.0.0. This call is
redundant and can be removed.
```suggestion
```
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -551,39 +578,39 @@ test('should allow creating new metrics in dataset
editor', async () => {
});
// Open datasource menu and click edit dataset
- userEvent.click(screen.getByTestId('datasource-menu-trigger'));
- userEvent.click(await screen.findByTestId('edit-dataset'));
+ await userEvent.click(screen.getByTestId('datasource-menu-trigger'));
+ await userEvent.click(await screen.findByTestId('edit-dataset'));
// Wait for modal to appear and navigate to Metrics tab
await waitFor(() => {
expect(screen.getByText('Metrics')).toBeInTheDocument();
});
- userEvent.click(screen.getByText('Metrics'));
+ await userEvent.click(screen.getByText('Metrics'));
// Click add new metric button
- await waitFor(() => {
+ await waitFor(async () => {
const addButton = screen.getByTestId('crud-add-table-item');
expect(addButton).toBeInTheDocument();
- userEvent.click(addButton);
+ await userEvent.click(addButton);
});
// Find and fill in the metric name
- await waitFor(() => {
+ await waitFor(async () => {
const nameInput = screen.getByTestId('textarea-editable-title-input');
expect(nameInput).toBeInTheDocument();
- userEvent.clear(nameInput);
- userEvent.type(nameInput, newMetricName);
+ await userEvent.clear(nameInput);
+ await userEvent.type(nameInput, newMetricName);
});
Review Comment:
Using an async callback in `waitFor` is an anti-pattern. User interactions
should not be performed inside `waitFor` blocks. Refactor to: `const nameInput
= await screen.findByTestId('textarea-editable-title-input'); await
userEvent.clear(nameInput); await userEvent.type(nameInput, newMetricName);`
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -689,14 +728,14 @@ test('should handle metric save confirmation modal',
async () => {
});
// Open edit dataset modal
- userEvent.click(screen.getByTestId('datasource-menu-trigger'));
- userEvent.click(await screen.findByTestId('edit-dataset'));
+ await userEvent.click(screen.getByTestId('datasource-menu-trigger'));
+ await userEvent.click(await screen.findByTestId('edit-dataset'));
// Save without making changes
- await waitFor(() => {
+ await waitFor(async () => {
const saveButton = screen.getByTestId('datasource-modal-save');
expect(saveButton).toBeInTheDocument();
- userEvent.click(saveButton);
+ await userEvent.click(saveButton);
});
Review Comment:
Using an async callback in `waitFor` is an anti-pattern. User interactions
should not be performed inside `waitFor` blocks. Refactor to: `const saveButton
= await screen.findByTestId('datasource-modal-save'); await
userEvent.click(saveButton);`
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -626,35 +659,35 @@ test('should allow deleting metrics in dataset editor',
async () => {
});
// Open edit dataset modal
- userEvent.click(screen.getByTestId('datasource-menu-trigger'));
- userEvent.click(await screen.findByTestId('edit-dataset'));
+ await userEvent.click(screen.getByTestId('datasource-menu-trigger'));
+ await userEvent.click(await screen.findByTestId('edit-dataset'));
// Navigate to Metrics tab
await waitFor(() => {
expect(screen.getByText('Metrics')).toBeInTheDocument();
});
- userEvent.click(screen.getByText('Metrics'));
+ await userEvent.click(screen.getByText('Metrics'));
// Find existing metric and delete it
- await waitFor(() => {
+ await waitFor(async () => {
const metricRow = screen.getByText(existingMetricName).closest('tr');
expect(metricRow).toBeInTheDocument();
const deleteButton = metricRow?.querySelector(
'[data-test="crud-delete-icon"]',
);
expect(deleteButton).toBeInTheDocument();
- userEvent.click(deleteButton!);
+ await userEvent.click(deleteButton!);
});
// Save the changes
- userEvent.click(screen.getByTestId('datasource-modal-save'));
+ await userEvent.click(screen.getByTestId('datasource-modal-save'));
// Confirm the save
- await waitFor(() => {
+ await waitFor(async () => {
const okButton = screen.getByText('OK');
expect(okButton).toBeInTheDocument();
- userEvent.click(okButton);
+ await userEvent.click(okButton);
});
Review Comment:
Using an async callback in `waitFor` is an anti-pattern. User interactions
should not be performed inside `waitFor` blocks. Refactor to: `const okButton =
await screen.findByText('OK'); await userEvent.click(okButton);`
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -750,20 +795,20 @@ test('should verify real DatasourceControl callback fires
on save', async () =>
expect(screen.getByTestId('datasource-control')).toBeInTheDocument();
// Open dataset editor
- userEvent.click(screen.getByTestId('datasource-menu-trigger'));
- userEvent.click(await screen.findByTestId('edit-dataset'));
+ await userEvent.click(screen.getByTestId('datasource-menu-trigger'));
+ await userEvent.click(await screen.findByTestId('edit-dataset'));
// Wait for modal to open
await waitFor(() => {
expect(screen.getByText('Columns')).toBeInTheDocument();
});
// Save without making changes (this should still trigger the callback)
- userEvent.click(screen.getByTestId('datasource-modal-save'));
- await waitFor(() => {
+ await userEvent.click(screen.getByTestId('datasource-modal-save'));
+ await waitFor(async () => {
const okButton = screen.getByText('OK');
expect(okButton).toBeInTheDocument();
- userEvent.click(okButton);
+ await userEvent.click(okButton);
});
Review Comment:
Using an async callback in `waitFor` is an anti-pattern. User interactions
should not be performed inside `waitFor` blocks. Refactor to: `const okButton =
await screen.findByText('OK'); await userEvent.click(okButton);`
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -626,35 +659,35 @@ test('should allow deleting metrics in dataset editor',
async () => {
});
// Open edit dataset modal
- userEvent.click(screen.getByTestId('datasource-menu-trigger'));
- userEvent.click(await screen.findByTestId('edit-dataset'));
+ await userEvent.click(screen.getByTestId('datasource-menu-trigger'));
+ await userEvent.click(await screen.findByTestId('edit-dataset'));
// Navigate to Metrics tab
await waitFor(() => {
expect(screen.getByText('Metrics')).toBeInTheDocument();
});
- userEvent.click(screen.getByText('Metrics'));
+ await userEvent.click(screen.getByText('Metrics'));
// Find existing metric and delete it
- await waitFor(() => {
+ await waitFor(async () => {
const metricRow = screen.getByText(existingMetricName).closest('tr');
expect(metricRow).toBeInTheDocument();
const deleteButton = metricRow?.querySelector(
'[data-test="crud-delete-icon"]',
);
expect(deleteButton).toBeInTheDocument();
- userEvent.click(deleteButton!);
+ await userEvent.click(deleteButton!);
});
Review Comment:
Using an async callback in `waitFor` is an anti-pattern. User interactions
should not be performed inside `waitFor` blocks. Refactor to: `await waitFor(()
=> { const metricRow = screen.getByText(existingMetricName).closest('tr');
expect(metricRow).toBeInTheDocument(); const deleteButton =
metricRow?.querySelector('[data-test=\"crud-delete-icon\"]');
expect(deleteButton).toBeInTheDocument(); }); const metricRow =
screen.getByText(existingMetricName).closest('tr'); const deleteButton =
metricRow?.querySelector('[data-test=\"crud-delete-icon\"]')!; await
userEvent.click(deleteButton);`
--
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]
