Copilot commented on code in PR #35810:
URL: https://github.com/apache/superset/pull/35810#discussion_r2579068087
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditor.test.tsx:
##########
@@ -18,295 +18,366 @@
*/
import fetchMock from 'fetch-mock';
import {
- render,
+ fireEvent,
screen,
waitFor,
userEvent,
- cleanup,
+ within,
} from 'spec/helpers/testing-library';
-import mockDatasource from 'spec/fixtures/mockDatasource';
import { DatasourceType, isFeatureEnabled } from '@superset-ui/core';
-import type { DatasetObject } from 'src/features/datasets/types';
-import DatasourceEditor from '..';
+import {
+ props,
+ DATASOURCE_ENDPOINT,
+ asyncRender,
+ fastRender,
+ setupDatasourceEditorMocks,
+ cleanupAsyncOperations,
+ dismissDatasourceWarning,
+} from './DatasourceEditor.test.utils';
-/* eslint-disable jest/no-export */
jest.mock('@superset-ui/core', () => ({
...jest.requireActual('@superset-ui/core'),
isFeatureEnabled: jest.fn(),
}));
-interface DatasourceEditorProps {
- datasource: DatasetObject;
- addSuccessToast: () => void;
- addDangerToast: () => void;
- onChange: jest.Mock;
- columnLabels?: Record<string, string>;
- columnLabelTooltips?: Record<string, string>;
-}
-
-// Common setup for tests
-export const props: DatasourceEditorProps = {
- datasource: mockDatasource['7__table'],
- addSuccessToast: () => {},
- addDangerToast: () => {},
- onChange: jest.fn(),
- columnLabels: {
- state: 'State',
- },
- columnLabelTooltips: {
- state: 'This is a tooltip for state',
- },
-};
-
-export const DATASOURCE_ENDPOINT =
- 'glob:*/datasource/external_metadata_by_name/*';
-
-const routeProps = {
- history: {},
- location: {},
- match: {},
-};
-
-export const asyncRender = (renderProps: DatasourceEditorProps) =>
- waitFor(() =>
- render(<DatasourceEditor {...renderProps} {...routeProps} />, {
- useRedux: true,
- initialState: { common: { currencies: ['USD', 'GBP', 'EUR'] } },
- useRouter: true,
- }),
- );
+beforeEach(() => {
+ fetchMock.get(DATASOURCE_ENDPOINT, [], { overwriteRoutes: true });
+ setupDatasourceEditorMocks();
+ jest.clearAllMocks();
+});
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('DatasourceEditor', () => {
- beforeAll(() => {
- jest.clearAllMocks();
+afterEach(async () => {
+ await cleanupAsyncOperations();
+ fetchMock.restore();
+ // Reset module mock since jest.fn() doesn't support mockRestore()
+ jest.mocked(isFeatureEnabled).mockReset();
+});
+
+test('renders Tabs', async () => {
+ await asyncRender({
+ ...props,
+ datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
});
- beforeEach(async () => {
- fetchMock.get(DATASOURCE_ENDPOINT, [], { overwriteRoutes: true });
- await asyncRender({
- ...props,
- datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
- });
+ expect(screen.getByTestId('edit-dataset-tabs')).toBeInTheDocument();
+});
+
+test('can sync columns from source', async () => {
+ await asyncRender({
+ ...props,
+ datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
});
- afterEach(() => {
- fetchMock.restore();
- // jest.clearAllMocks();
+ const columnsTab = screen.getByTestId('collection-tab-Columns');
+ await userEvent.click(columnsTab);
+
+ const syncButton = screen.getByText(/sync columns from source/i);
+ expect(syncButton).toBeInTheDocument();
+
+ // Use a Promise to track when fetchMock is called
+ const fetchPromise = new Promise<string>(resolve => {
+ fetchMock.get(
+ DATASOURCE_ENDPOINT,
+ (url: string) => {
+ resolve(url);
+ return [];
+ },
+ { overwriteRoutes: true },
+ );
});
- test('renders Tabs', () => {
- expect(screen.getByTestId('edit-dataset-tabs')).toBeInTheDocument();
+ await userEvent.click(syncButton);
+
+ // Wait for the fetch to be called
+ const url = await fetchPromise;
+ expect(url).toContain('Vehicle+Sales%20%2B');
+});
+
+// to add, remove and modify columns accordingly
+test('can modify columns', async () => {
+ const limitedProps = {
+ ...props,
+ onChange: jest.fn(),
+ datasource: {
+ ...props.datasource,
+ table_name: 'Vehicle Sales +',
+ columns: props.datasource.columns
+ .slice(0, 1)
+ .map(column => ({ ...column })),
+ },
+ };
+
+ fastRender(limitedProps);
+
+ await dismissDatasourceWarning();
+
+ const columnsTab = await screen.findByTestId('collection-tab-Columns');
+ await userEvent.click(columnsTab);
+
+ const getToggles = await screen.findAllByRole('button', {
+ name: /expand row/i,
});
+ await userEvent.click(getToggles[0]);
+
+ const getTextboxes = await screen.findAllByRole('textbox');
+ expect(getTextboxes.length).toBeGreaterThanOrEqual(5);
+
+ const inputLabel = screen.getByPlaceholderText('Label');
+ const inputCertDetails = screen.getByPlaceholderText('Certification
details');
- test('can sync columns from source', async () => {
- const columnsTab = screen.getByTestId('collection-tab-Columns');
- await userEvent.click(columnsTab);
-
- const syncButton = screen.getByText(/sync columns from source/i);
- expect(syncButton).toBeInTheDocument();
-
- // Use a Promise to track when fetchMock is called
- const fetchPromise = new Promise<string>(resolve => {
- fetchMock.get(
- DATASOURCE_ENDPOINT,
- (url: string) => {
- resolve(url);
- return [];
- },
- { overwriteRoutes: true },
- );
- });
-
- await userEvent.click(syncButton);
-
- // Wait for the fetch to be called
- const url = await fetchPromise;
- expect(url).toContain('Vehicle+Sales%20%2B');
+ // Clear onChange mock to track user action callbacks
+ limitedProps.onChange.mockClear();
+
+ // Use fireEvent.change for speed - testing wiring, not per-keystroke
behavior
+ fireEvent.change(inputLabel, { target: { value: 'test_label' } });
+ fireEvent.change(inputCertDetails, { target: { value: 'test_details' } });
+
+ // Verify the inputs were updated and onChange was triggered
+ await waitFor(() => {
+ expect(inputLabel).toHaveValue('test_label');
+ expect(inputCertDetails).toHaveValue('test_details');
+ expect(limitedProps.onChange).toHaveBeenCalled();
});
+});
- // to add, remove and modify columns accordingly
- test('can modify columns', async () => {
- const columnsTab = screen.getByTestId('collection-tab-Columns');
- await userEvent.click(columnsTab);
-
- const getToggles = screen.getAllByRole('button', {
- name: /expand row/i,
- });
- await userEvent.click(getToggles[0]);
-
- const getTextboxes = await screen.findAllByRole('textbox');
- expect(getTextboxes.length).toBeGreaterThanOrEqual(5);
-
- const inputLabel = screen.getByPlaceholderText('Label');
- const inputDescription = screen.getByPlaceholderText('Description');
- const inputDtmFormat = screen.getByPlaceholderText('%Y-%m-%d');
- const inputCertifiedBy = screen.getByPlaceholderText('Certified by');
- const inputCertDetails = screen.getByPlaceholderText(
- 'Certification details',
- );
+test('can delete columns', async () => {
+ const limitedProps = {
+ ...props,
+ onChange: jest.fn(),
+ datasource: {
+ ...props.datasource,
+ table_name: 'Vehicle Sales +',
+ columns: props.datasource.columns
+ .slice(0, 1)
+ .map(column => ({ ...column })),
+ },
+ };
+
+ fastRender(limitedProps);
- // Clear onChange mock to track user action callbacks
- props.onChange.mockClear();
-
- await userEvent.type(inputLabel, 'test_label');
- await userEvent.type(inputDescription, 'test');
- await userEvent.type(inputDtmFormat, 'test');
- await userEvent.type(inputCertifiedBy, 'test');
- await userEvent.type(inputCertDetails, 'test');
-
- // Verify the inputs were updated with the typed values
- await waitFor(() => {
- expect(inputLabel).toHaveValue('test_label');
- expect(inputDescription).toHaveValue('test');
- expect(inputDtmFormat).toHaveValue('test');
- expect(inputCertifiedBy).toHaveValue('test');
- expect(inputCertDetails).toHaveValue('test');
- });
-
- // Verify that onChange was triggered by user actions
- await waitFor(() => {
- expect(props.onChange).toHaveBeenCalled();
- });
- }, 40000);
-
- test('can delete columns', async () => {
- const columnsTab = screen.getByTestId('collection-tab-Columns');
- await userEvent.click(columnsTab);
-
- const getToggles = screen.getAllByRole('button', {
- name: /expand row/i,
- });
-
- await userEvent.click(getToggles[0]);
-
- const deleteButtons = await screen.findAllByRole('button', {
- name: /delete item/i,
- });
- const initialCount = deleteButtons.length;
- expect(initialCount).toBeGreaterThan(0);
-
- await userEvent.click(deleteButtons[0]);
-
- await waitFor(() => {
- const countRows = screen.getAllByRole('button', { name: /delete item/i
});
- expect(countRows.length).toBe(initialCount - 1);
- });
- }, 60000); // 60 seconds timeout to avoid timeouts
-
- test('can add new columns', async () => {
- const calcColsTab = screen.getByTestId('collection-tab-Calculated
columns');
- await userEvent.click(calcColsTab);
-
- const addBtn = screen.getByRole('button', {
- name: /add item/i,
- });
- expect(addBtn).toBeInTheDocument();
-
- await userEvent.click(addBtn);
-
- // newColumn (Column name) is the first textbox in the tab
- await waitFor(() => {
- const newColumn = screen.getAllByRole('textbox')[0];
- expect(newColumn).toHaveValue('<new column>');
- });
- }, 60000);
-
- test('renders isSqla fields', async () => {
- const columnsTab = screen.getByRole('tab', {
- name: /settings/i,
- });
- await userEvent.click(columnsTab);
-
- const extraField = screen.getAllByText(/extra/i);
- expect(extraField.length).toBeGreaterThan(0);
+ await dismissDatasourceWarning();
+
+ const columnsTab = await screen.findByTestId('collection-tab-Columns');
+ await userEvent.click(columnsTab);
+
+ const columnsPanel = within(
+ await screen.findByRole('tabpanel', { name: /columns/i }),
+ );
+
+ const getToggles = await columnsPanel.findAllByRole('button', {
+ name: /expand row/i,
+ });
+
+ await userEvent.click(getToggles[0]);
+
+ const deleteButtons = await columnsPanel.findAllByRole('button', {
+ name: /delete item/i,
+ });
+ const initialCount = deleteButtons.length;
+ expect(initialCount).toBeGreaterThan(0);
+
+ // Clear onChange mock to track delete action
+ limitedProps.onChange.mockClear();
+
+ await userEvent.click(deleteButtons[0]);
+
+ await waitFor(() =>
expect(
- screen.getByText(/autocomplete query predicate/i),
- ).toBeInTheDocument();
- expect(screen.getByText(/template parameters/i)).toBeInTheDocument();
+ columnsPanel.queryAllByRole('button', { name: /delete item/i }),
+ ).toHaveLength(initialCount - 1),
+ );
+ await waitFor(() => expect(limitedProps.onChange).toHaveBeenCalled());
+});
+
+test('can add new columns', async () => {
+ await asyncRender({
+ ...props,
+ datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
+ });
+
+ const calcColsTab = screen.getByTestId('collection-tab-Calculated columns');
+ await userEvent.click(calcColsTab);
+
+ const addBtn = screen.getByRole('button', {
+ name: /add item/i,
+ });
+ expect(addBtn).toBeInTheDocument();
+
+ await userEvent.click(addBtn);
+
+ // newColumn (Column name) is the first textbox in the tab
+ await waitFor(() => {
+ const newColumn = screen.getAllByRole('textbox')[0];
+ expect(newColumn).toHaveValue('<new column>');
});
});
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('DatasourceEditor Source Tab', () => {
- beforeAll(() => {
- (isFeatureEnabled as jest.Mock).mockImplementation(() => false);
+test('renders isSqla fields', async () => {
+ await asyncRender({
+ ...props,
+ datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
});
- beforeEach(async () => {
- fetchMock.get(DATASOURCE_ENDPOINT, [], { overwriteRoutes: true });
- await asyncRender({
- ...props,
- datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
- });
+ const columnsTab = screen.getByRole('tab', {
+ name: /settings/i,
});
+ await userEvent.click(columnsTab);
+
+ const extraField = screen.getAllByText(/extra/i);
+ expect(extraField.length).toBeGreaterThan(0);
+ expect(screen.getByText(/autocomplete query
predicate/i)).toBeInTheDocument();
+ expect(screen.getByText(/template parameters/i)).toBeInTheDocument();
+});
+
+test('Source Tab: edit mode', async () => {
+ (isFeatureEnabled as jest.Mock).mockImplementation(() => false);
- afterEach(() => {
- fetchMock.restore();
+ await asyncRender({
+ ...props,
+ datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
});
- afterAll(() => {
- (isFeatureEnabled as jest.Mock).mockRestore();
+ const getLockBtn = screen.getByRole('img', { name: /lock/i });
+ await userEvent.click(getLockBtn);
+
+ const physicalRadioBtn = screen.getByRole('radio', {
+ name: /physical \(table or view\)/i,
+ });
+ const virtualRadioBtn = screen.getByRole('radio', {
+ name: /virtual \(sql\)/i,
});
- test('Source Tab: edit mode', async () => {
- const getLockBtn = screen.getByRole('img', { name: /lock/i });
- await userEvent.click(getLockBtn);
+ expect(physicalRadioBtn).toBeEnabled();
+ expect(virtualRadioBtn).toBeEnabled();
+});
- const physicalRadioBtn = screen.getByRole('radio', {
- name: /physical \(table or view\)/i,
- });
- const virtualRadioBtn = screen.getByRole('radio', {
- name: /virtual \(sql\)/i,
- });
+test('Source Tab: readOnly mode', async () => {
+ (isFeatureEnabled as jest.Mock).mockImplementation(() => false);
- expect(physicalRadioBtn).toBeEnabled();
- expect(virtualRadioBtn).toBeEnabled();
+ await asyncRender({
+ ...props,
+ datasource: { ...props.datasource, table_name: 'Vehicle Sales +' },
});
- test('Source Tab: readOnly mode', () => {
- const getLockBtn = screen.getByRole('img', { name: /lock/i });
- expect(getLockBtn).toBeInTheDocument();
+ const getLockBtn = screen.getByRole('img', { name: /lock/i });
+ expect(getLockBtn).toBeInTheDocument();
+
+ const physicalRadioBtn = screen.getByRole('radio', {
+ name: /physical \(table or view\)/i,
+ });
+ const virtualRadioBtn = screen.getByRole('radio', {
+ name: /virtual \(sql\)/i,
+ });
+
+ expect(physicalRadioBtn).toBeDisabled();
+ expect(virtualRadioBtn).toBeDisabled();
+});
+
+test('calls onChange with empty SQL when switching to physical dataset', async
() => {
+ (isFeatureEnabled as jest.Mock).mockImplementation(() => false);
- const physicalRadioBtn = screen.getByRole('radio', {
- name: /physical \(table or view\)/i,
- });
- const virtualRadioBtn = screen.getByRole('radio', {
- name: /virtual \(sql\)/i,
- });
+ props.onChange.mockClear();
Review Comment:
This test mutates the shared `props.onChange` mock by calling `mockClear()`,
which could cause test pollution if other tests rely on the mock's call
history. Since a factory function `createProps()` is available in the utils
file (as mentioned in the deprecation comment), consider using `const testProps
= createProps(); testProps.onChange.mockClear();` or simply using a fresh
instance from `createProps()` without needing to clear the mock.
--
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]
