Copilot commented on code in PR #36012:
URL: https://github.com/apache/superset/pull/36012#discussion_r2765151916
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -126,22 +124,38 @@ const datasetResult = (id: number) => ({
show_columns: ['id', 'table_name'],
});
-fetchMock.get('glob:*/api/v1/dataset/1', datasetResult(1));
-fetchMock.get(`glob:*/api/v1/dataset/${id}`, datasetResult(id));
-
-fetchMock.post('glob:*/api/v1/chart/data', {
- result: [
- {
- status: 'success',
- data: [
- { name: 'Aaron', count: 453 },
- { name: 'Abigail', count: 228 },
- { name: 'Adam', count: 454 },
- ],
- applied_filters: [{ column: 'name' }],
- },
- ],
-});
+function setupFetchMocks() {
+ fetchMock.get(`glob:*/api/v1/dataset/${id}`, datasetResult(id));
+ // Mock dataset 1 for buildNativeFilter fixtures which use datasetId: 1
+ fetchMock.get('glob:*/api/v1/dataset/1', datasetResult(1));
+ // Mock the dataset list endpoint for the dataset selector dropdown
+ // Uses id from mockDatasource (7) to match fixture data
+ fetchMock.get('glob:*/api/v1/dataset/?*', {
+ result: [
+ {
+ id,
+ table_name: 'birth_names',
+ database: { database_name: 'examples' },
+ schema: 'public',
+ },
+ ],
+ count: 1,
+ });
+
+ fetchMock.post('glob:*/api/v1/chart/data', {
+ result: [
+ {
+ status: 'success',
+ data: [
+ { name: 'Aaron', count: 453 },
+ { name: 'Abigail', count: 228 },
+ { name: 'Adam', count: 454 },
+ ],
+ applied_filters: [{ column: 'name' }],
+ },
+ ],
+ });
+}
Review Comment:
The setupFetchMocks function is called in beforeEach, which means
fetchMock.get/post are being called on every test run. However, these setup the
same routes repeatedly, which can lead to route conflicts or warnings from
fetch-mock. Consider checking if routes are already registered or using
fetchMock.restore() instead of fetchMock.removeRoutes() in afterEach to clear
both routes and history, or conditionally set up routes only if they haven't
been registered yet.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -126,22 +124,38 @@ const datasetResult = (id: number) => ({
show_columns: ['id', 'table_name'],
});
-fetchMock.get('glob:*/api/v1/dataset/1', datasetResult(1));
-fetchMock.get(`glob:*/api/v1/dataset/${id}`, datasetResult(id));
-
-fetchMock.post('glob:*/api/v1/chart/data', {
- result: [
- {
- status: 'success',
- data: [
- { name: 'Aaron', count: 453 },
- { name: 'Abigail', count: 228 },
- { name: 'Adam', count: 454 },
- ],
- applied_filters: [{ column: 'name' }],
- },
- ],
-});
+function setupFetchMocks() {
+ fetchMock.get(`glob:*/api/v1/dataset/${id}`, datasetResult(id));
+ // Mock dataset 1 for buildNativeFilter fixtures which use datasetId: 1
+ fetchMock.get('glob:*/api/v1/dataset/1', datasetResult(1));
+ // Mock the dataset list endpoint for the dataset selector dropdown
+ // Uses id from mockDatasource (7) to match fixture data
+ fetchMock.get('glob:*/api/v1/dataset/?*', {
+ result: [
+ {
+ id,
Review Comment:
The comment states "Uses id from mockDatasource (7) to match fixture data"
but the code uses the variable 'id' which is defined as 7 at line 32 (from
context). While functionally correct, using a magic number comment alongside a
named variable is confusing. Consider either using the literal 7 with the
comment, or referencing the 'id' constant in the comment for clarity.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -327,68 +346,58 @@ test('validates the column', async () => {
).toBeInTheDocument();
});
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('validates the default value', async () => {
- defaultRender(noTemporalColumnsState());
- expect(await screen.findByText('birth_names')).toBeInTheDocument();
- await userEvent.type(screen.getByRole('combobox'), `Column A{Enter}`);
- await userEvent.click(getCheckbox(DEFAULT_VALUE_REGEX));
- await waitFor(() => {
- expect(
- screen.queryByText(FILL_REQUIRED_FIELDS_REGEX),
- ).not.toBeInTheDocument();
+// This test validates the "default value" field validation.
+//
+// LIMITATION: Does not exercise the full dataset/column selection flow.
+// With createNewOnOpen: true, the modal renders in a state where form fields
+// are visible but selecting dataset/column through async selects requires
+// complex setup that proved unreliable in this unit test environment.
+//
+// What this test covers:
+// - Default value checkbox can be enabled
+// - Validation error appears when default value is enabled without a value
+//
+// What would require integration/E2E testing:
+// - Full flow: open modal → select dataset → select column → enable default
value → validate
+// - This flow is better tested with Playwright where the full component
lifecycle is available
+//
+// The core validation logic is still covered - this guards against
regressions where
+// the "Please choose a valid value" error fails to appear when default value
is enabled.
+test('validates the default value', async () => {
+ defaultRender();
+ // Wait for the default value checkbox to appear
+ const defaultValueCheckbox = await screen.findByRole('checkbox', {
+ name: DEFAULT_VALUE_REGEX,
});
+ // Verify default value error is NOT present before enabling checkbox
+ expect(screen.queryByText(/choose.*valid value/i)).not.toBeInTheDocument();
+ // Enable default value checkbox without setting a value
+ await userEvent.click(defaultValueCheckbox);
+ // Try to save - should show validation error
+ await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
+ // Verify validation error appears (actual message is "Please choose a valid
value")
expect(
- await screen.findByText(DEFAULT_VALUE_REQUIRED_REGEX),
+ await screen.findByText(/choose.*valid value/i, {}, { timeout: 3000 }),
).toBeInTheDocument();
Review Comment:
The test checks for a validation error with the regex `/choose.*valid
value/i`, but the actual error message format should be verified. If the error
message changes in the future (e.g., from "Please choose a valid value" to
something else), this test might fail or pass incorrectly. Consider using a
more specific matcher or extracting the error message to a constant like the
other regex patterns in the file (e.g., DEFAULT_VALUE_REQUIRED_REGEX).
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -327,68 +346,58 @@ test('validates the column', async () => {
).toBeInTheDocument();
});
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('validates the default value', async () => {
- defaultRender(noTemporalColumnsState());
- expect(await screen.findByText('birth_names')).toBeInTheDocument();
- await userEvent.type(screen.getByRole('combobox'), `Column A{Enter}`);
- await userEvent.click(getCheckbox(DEFAULT_VALUE_REGEX));
- await waitFor(() => {
- expect(
- screen.queryByText(FILL_REQUIRED_FIELDS_REGEX),
- ).not.toBeInTheDocument();
+// This test validates the "default value" field validation.
+//
+// LIMITATION: Does not exercise the full dataset/column selection flow.
+// With createNewOnOpen: true, the modal renders in a state where form fields
+// are visible but selecting dataset/column through async selects requires
+// complex setup that proved unreliable in this unit test environment.
+//
+// What this test covers:
+// - Default value checkbox can be enabled
+// - Validation error appears when default value is enabled without a value
+//
+// What would require integration/E2E testing:
+// - Full flow: open modal → select dataset → select column → enable default
value → validate
+// - This flow is better tested with Playwright where the full component
lifecycle is available
+//
+// The core validation logic is still covered - this guards against
regressions where
+// the "Please choose a valid value" error fails to appear when default value
is enabled.
+test('validates the default value', async () => {
+ defaultRender();
+ // Wait for the default value checkbox to appear
+ const defaultValueCheckbox = await screen.findByRole('checkbox', {
+ name: DEFAULT_VALUE_REGEX,
});
+ // Verify default value error is NOT present before enabling checkbox
+ expect(screen.queryByText(/choose.*valid value/i)).not.toBeInTheDocument();
+ // Enable default value checkbox without setting a value
+ await userEvent.click(defaultValueCheckbox);
+ // Try to save - should show validation error
+ await userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
+ // Verify validation error appears (actual message is "Please choose a valid
value")
expect(
- await screen.findByText(DEFAULT_VALUE_REQUIRED_REGEX),
+ await screen.findByText(/choose.*valid value/i, {}, { timeout: 3000 }),
Review Comment:
The test timeout is set to 50 seconds (50000ms), but the findByText call
within has a timeout of only 3 seconds (3000ms). If the validation error takes
longer than 3 seconds to appear, the test will fail even though there are 47
seconds remaining in the test timeout. Consider increasing the findByText
timeout to be closer to the test timeout, or using the default timeout which
should respect the test's overall timeout.
```suggestion
await screen.findByText(/choose.*valid value/i, {}, { timeout: 45000 }),
```
--
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]
