sadpandajoe commented on code in PR #36012:
URL: https://github.com/apache/superset/pull/36012#discussion_r2765424007
##########
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 (see PROJECT.md Investigation section).
Review Comment:
Already removed in previous commit.
##########
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));
Review Comment:
Won't fix - tried this exact change previously and it broke 4 existing
tests. Current patterns work correctly.
##########
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:
Won't fix - `restore()` was renamed to `removeRoutes()` in fetch-mock v12
(PR #36662).
##########
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:
Fixed - clarified comment to reference `id` constant.
##########
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:
Won't fix - 3s timeout is intentional (expected max for element to appear).
50s is safety margin for entire test, not individual assertions.
##########
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:
Fixed - extracted `DEFAULT_VALUE_INVALID_REGEX` constant.
--
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]
