codeant-ai-for-open-source[bot] commented on code in PR #37902: URL: https://github.com/apache/superset/pull/37902#discussion_r2873391871
########## superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.test.tsx: ########## @@ -0,0 +1,265 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + cleanup, + render, + screen, + userEvent, +} from 'spec/helpers/testing-library'; +import FilterScopeSelector from './FilterScopeSelector'; +import type { DashboardLayout } from 'src/dashboard/types'; + +// --- Mock child components --- + +jest.mock('./FilterFieldTree', () => ({ + __esModule: true, + default: (props: Record<string, unknown>) => ( + <div data-test="filter-field-tree"> + FilterFieldTree (checked={String(props.checked)}) + </div> + ), +})); + +jest.mock('./FilterScopeTree', () => ({ + __esModule: true, + default: (props: Record<string, unknown>) => ( + <div data-test="filter-scope-tree"> + FilterScopeTree (checked={String(props.checked)}) + </div> + ), +})); + +// --- Mock utility functions --- + +jest.mock('src/dashboard/util/getFilterFieldNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => [ + { + value: 'ALL_FILTERS_ROOT', + label: 'All filters', + children: [ + { + value: 1, + label: 'Filter A', + children: [ + { value: '1_column_b', label: 'Filter B' }, + { value: '1_column_c', label: 'Filter C' }, + ], + }, + ], + }, + ]), +})); + +jest.mock('src/dashboard/util/getFilterScopeNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => [ + { + value: 'ROOT_ID', + label: 'All charts', + children: [{ value: 2, label: 'Chart A' }], + }, + ]), +})); + +jest.mock('src/dashboard/util/getFilterScopeParentNodes', () => ({ + __esModule: true, + default: jest.fn(() => ['ROOT_ID']), +})); + +jest.mock('src/dashboard/util/buildFilterScopeTreeEntry', () => ({ + __esModule: true, + default: jest.fn(() => ({})), +})); + +jest.mock('src/dashboard/util/getKeyForFilterScopeTree', () => ({ + __esModule: true, + default: jest.fn(() => '1_column_b'), +})); + +jest.mock('src/dashboard/util/getSelectedChartIdForFilterScopeTree', () => ({ + __esModule: true, + default: jest.fn(() => 1), +})); + +jest.mock('src/dashboard/util/getFilterScopeFromNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => ({ scope: ['ROOT_ID'], immune: [] })), +})); + +jest.mock('src/dashboard/util/getRevertedFilterScope', () => ({ + __esModule: true, + default: jest.fn(() => ({})), +})); + +jest.mock('src/dashboard/util/activeDashboardFilters', () => ({ + getChartIdsInFilterScope: jest.fn(() => [2, 3]), +})); + +afterEach(() => { + cleanup(); + jest.clearAllMocks(); +}); + +const mockDashboardFilters = { + 1: { + chartId: 1, + componentId: 'component-1', + filterName: 'Filter A', + datasourceId: 'ds-1', + directPathToFilter: ['ROOT_ID', 'GRID', 'CHART_1'], + isDateFilter: false, + isInstantFilter: false, + columns: { column_b: undefined, column_c: undefined }, + labels: { column_b: 'Filter B', column_c: 'Filter C' }, + scopes: { + column_b: { immune: [], scope: ['ROOT_ID'] }, + column_c: { immune: [], scope: ['ROOT_ID'] }, + }, + }, +}; + +const mockLayout: DashboardLayout = { + ROOT_ID: { children: ['GRID'], id: 'ROOT_ID', type: 'ROOT' }, + GRID: { + children: ['CHART_1', 'CHART_2'], + id: 'GRID', + type: 'GRID', + parents: ['ROOT_ID'], + }, + CHART_1: { + meta: { chartId: 1, sliceName: 'Chart 1' }, + children: [], + id: 'CHART_1', + type: 'CHART', + parents: ['ROOT_ID', 'GRID'], + }, + CHART_2: { + meta: { chartId: 2, sliceName: 'Chart 2' }, + children: [], + id: 'CHART_2', + type: 'CHART', + parents: ['ROOT_ID', 'GRID'], + }, +} as unknown as DashboardLayout; + +const defaultProps = { + dashboardFilters: mockDashboardFilters, + layout: mockLayout, + updateDashboardFiltersScope: jest.fn(), + setUnsavedChanges: jest.fn(), + onCloseModal: jest.fn(), +}; + +test('renders the header, filter field panel, and scope panel', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + expect(screen.getByText('Configure filter scopes')).toBeInTheDocument(); + expect(screen.getByTestId('filter-field-tree')).toBeInTheDocument(); + expect(screen.getByTestId('filter-scope-tree')).toBeInTheDocument(); +}); + +test('renders the search input with correct placeholder', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + const searchInput = screen.getByPlaceholderText('Search...'); + expect(searchInput).toBeInTheDocument(); + expect(searchInput).toHaveAttribute('type', 'text'); +}); + +test('renders Close and Save buttons when filters exist', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument(); +}); + +test('renders only Close button and a warning when no filters exist', () => { + render(<FilterScopeSelector {...defaultProps} dashboardFilters={{}} />, { + useRedux: true, + }); + + expect( + screen.getByText('There are no filters in this dashboard.'), + ).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + expect( + screen.queryByRole('button', { name: 'Save' }), + ).not.toBeInTheDocument(); +}); + +test('does not render FilterFieldTree or FilterScopeTree when no filters exist', () => { + render(<FilterScopeSelector {...defaultProps} dashboardFilters={{}} />, { + useRedux: true, + }); + + expect(screen.queryByTestId('filter-field-tree')).not.toBeInTheDocument(); + expect(screen.queryByTestId('filter-scope-tree')).not.toBeInTheDocument(); +}); + +test('calls onCloseModal when Close button is clicked', () => { + const onCloseModal = jest.fn(); + render( + <FilterScopeSelector {...defaultProps} onCloseModal={onCloseModal} />, + { useRedux: true }, + ); + + userEvent.click(screen.getByRole('button', { name: 'Close' })); Review Comment: **Suggestion:** Missing await on async userEvent.click call. In modern React Testing Library (v14+), userEvent methods return promises and must be awaited to ensure the click action completes before assertions run. Without await, the test may complete before the event is processed, causing flaky or false-passing tests. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Flaky test failures in CI/CD pipeline for dashboard filter scope selector. - ⚠️ Unreliable test coverage for close button interaction in filter configuration modal. ``` </details> ```suggestion await userEvent.click(screen.getByRole('button', { name: 'Close' })); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Test runner executes `FilterScopeSelector.test.tsx` via Jest test command. 2. Test `calls onCloseModal when Close button is clicked` at line 215 renders the FilterScopeSelector component with mocked `onCloseModal` callback. 3. Test executes `userEvent.click()` at line 223 without await, which returns a Promise that is not awaited. 4. Test execution continues immediately to line 225 assertion `expect(onCloseModal).toHaveBeenCalledTimes(1)` before the user event promise resolves and React finishes state updates. 5. In React Testing Library v14+, this causes a race condition where the assertion may execute before the click event propagates and `onCloseModal` is called, resulting in intermittent test failures depending on event loop timing. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.test.tsx **Line:** 223:223 **Comment:** *Possible Bug: Missing await on async userEvent.click call. In modern React Testing Library (v14+), userEvent methods return promises and must be awaited to ensure the click action completes before assertions run. Without await, the test may complete before the event is processed, causing flaky or false-passing tests. 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%2F37902&comment_hash=1f7252b8caae273aa0a0e01594293dce11a62422b77a23284332af692572535c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=1f7252b8caae273aa0a0e01594293dce11a62422b77a23284332af692572535c&reaction=dislike'>👎</a> ########## superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.test.tsx: ########## @@ -0,0 +1,265 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + cleanup, + render, + screen, + userEvent, +} from 'spec/helpers/testing-library'; +import FilterScopeSelector from './FilterScopeSelector'; +import type { DashboardLayout } from 'src/dashboard/types'; + +// --- Mock child components --- + +jest.mock('./FilterFieldTree', () => ({ + __esModule: true, + default: (props: Record<string, unknown>) => ( + <div data-test="filter-field-tree"> + FilterFieldTree (checked={String(props.checked)}) + </div> + ), +})); + +jest.mock('./FilterScopeTree', () => ({ + __esModule: true, + default: (props: Record<string, unknown>) => ( + <div data-test="filter-scope-tree"> + FilterScopeTree (checked={String(props.checked)}) + </div> + ), +})); + +// --- Mock utility functions --- + +jest.mock('src/dashboard/util/getFilterFieldNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => [ + { + value: 'ALL_FILTERS_ROOT', + label: 'All filters', + children: [ + { + value: 1, + label: 'Filter A', + children: [ + { value: '1_column_b', label: 'Filter B' }, + { value: '1_column_c', label: 'Filter C' }, + ], + }, + ], + }, + ]), +})); + +jest.mock('src/dashboard/util/getFilterScopeNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => [ + { + value: 'ROOT_ID', + label: 'All charts', + children: [{ value: 2, label: 'Chart A' }], + }, + ]), +})); + +jest.mock('src/dashboard/util/getFilterScopeParentNodes', () => ({ + __esModule: true, + default: jest.fn(() => ['ROOT_ID']), +})); + +jest.mock('src/dashboard/util/buildFilterScopeTreeEntry', () => ({ + __esModule: true, + default: jest.fn(() => ({})), +})); + +jest.mock('src/dashboard/util/getKeyForFilterScopeTree', () => ({ + __esModule: true, + default: jest.fn(() => '1_column_b'), +})); + +jest.mock('src/dashboard/util/getSelectedChartIdForFilterScopeTree', () => ({ + __esModule: true, + default: jest.fn(() => 1), +})); + +jest.mock('src/dashboard/util/getFilterScopeFromNodesTree', () => ({ + __esModule: true, + default: jest.fn(() => ({ scope: ['ROOT_ID'], immune: [] })), +})); + +jest.mock('src/dashboard/util/getRevertedFilterScope', () => ({ + __esModule: true, + default: jest.fn(() => ({})), +})); + +jest.mock('src/dashboard/util/activeDashboardFilters', () => ({ + getChartIdsInFilterScope: jest.fn(() => [2, 3]), +})); + +afterEach(() => { + cleanup(); + jest.clearAllMocks(); +}); + +const mockDashboardFilters = { + 1: { + chartId: 1, + componentId: 'component-1', + filterName: 'Filter A', + datasourceId: 'ds-1', + directPathToFilter: ['ROOT_ID', 'GRID', 'CHART_1'], + isDateFilter: false, + isInstantFilter: false, + columns: { column_b: undefined, column_c: undefined }, + labels: { column_b: 'Filter B', column_c: 'Filter C' }, + scopes: { + column_b: { immune: [], scope: ['ROOT_ID'] }, + column_c: { immune: [], scope: ['ROOT_ID'] }, + }, + }, +}; + +const mockLayout: DashboardLayout = { + ROOT_ID: { children: ['GRID'], id: 'ROOT_ID', type: 'ROOT' }, + GRID: { + children: ['CHART_1', 'CHART_2'], + id: 'GRID', + type: 'GRID', + parents: ['ROOT_ID'], + }, + CHART_1: { + meta: { chartId: 1, sliceName: 'Chart 1' }, + children: [], + id: 'CHART_1', + type: 'CHART', + parents: ['ROOT_ID', 'GRID'], + }, + CHART_2: { + meta: { chartId: 2, sliceName: 'Chart 2' }, + children: [], + id: 'CHART_2', + type: 'CHART', + parents: ['ROOT_ID', 'GRID'], + }, +} as unknown as DashboardLayout; + +const defaultProps = { + dashboardFilters: mockDashboardFilters, + layout: mockLayout, + updateDashboardFiltersScope: jest.fn(), + setUnsavedChanges: jest.fn(), + onCloseModal: jest.fn(), +}; + +test('renders the header, filter field panel, and scope panel', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + expect(screen.getByText('Configure filter scopes')).toBeInTheDocument(); + expect(screen.getByTestId('filter-field-tree')).toBeInTheDocument(); + expect(screen.getByTestId('filter-scope-tree')).toBeInTheDocument(); +}); + +test('renders the search input with correct placeholder', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + const searchInput = screen.getByPlaceholderText('Search...'); + expect(searchInput).toBeInTheDocument(); + expect(searchInput).toHaveAttribute('type', 'text'); +}); + +test('renders Close and Save buttons when filters exist', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument(); +}); + +test('renders only Close button and a warning when no filters exist', () => { + render(<FilterScopeSelector {...defaultProps} dashboardFilters={{}} />, { + useRedux: true, + }); + + expect( + screen.getByText('There are no filters in this dashboard.'), + ).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + expect( + screen.queryByRole('button', { name: 'Save' }), + ).not.toBeInTheDocument(); +}); + +test('does not render FilterFieldTree or FilterScopeTree when no filters exist', () => { + render(<FilterScopeSelector {...defaultProps} dashboardFilters={{}} />, { + useRedux: true, + }); + + expect(screen.queryByTestId('filter-field-tree')).not.toBeInTheDocument(); + expect(screen.queryByTestId('filter-scope-tree')).not.toBeInTheDocument(); +}); + +test('calls onCloseModal when Close button is clicked', () => { + const onCloseModal = jest.fn(); + render( + <FilterScopeSelector {...defaultProps} onCloseModal={onCloseModal} />, + { useRedux: true }, + ); + + userEvent.click(screen.getByRole('button', { name: 'Close' })); + + expect(onCloseModal).toHaveBeenCalledTimes(1); +}); + +test('calls updateDashboardFiltersScope, setUnsavedChanges, and onCloseModal when Save is clicked', () => { + const updateDashboardFiltersScope = jest.fn(); + const setUnsavedChanges = jest.fn(); + const onCloseModal = jest.fn(); + + render( + <FilterScopeSelector + {...defaultProps} + updateDashboardFiltersScope={updateDashboardFiltersScope} + setUnsavedChanges={setUnsavedChanges} + onCloseModal={onCloseModal} + />, + { useRedux: true }, + ); + + userEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(updateDashboardFiltersScope).toHaveBeenCalledTimes(1); + expect(setUnsavedChanges).toHaveBeenCalledWith(true); + expect(onCloseModal).toHaveBeenCalledTimes(1); +}); + +test('renders the editing filters name section with "Editing 1 filter:" label', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + expect(screen.getByText('Editing 1 filter:')).toBeInTheDocument(); + // The active filter label should appear (column_b maps to "Filter B") + expect(screen.getByText('Filter B')).toBeInTheDocument(); +}); + +test('updates search text when typing in the search input', () => { + render(<FilterScopeSelector {...defaultProps} />, { useRedux: true }); + + const searchInput = screen.getByPlaceholderText('Search...'); + userEvent.type(searchInput, 'Chart'); Review Comment: **Suggestion:** Missing await on async userEvent.type call. In modern React Testing Library (v14+), userEvent.type returns a promise and must be awaited to ensure all keystrokes are processed before assertions run. Without await, the test may assert on the input value before typing completes. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Flaky test failures for search input functionality in filter scope selector. - ⚠️ False negatives in test suite when typing simulation doesn't complete before assertions. ``` </details> ```suggestion await userEvent.type(searchInput, 'Chart'); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Test runner executes test `updates search text when typing in the search input` starting at line 259 in `FilterScopeSelector.test.tsx`. 2. Test renders FilterScopeSelector and queries the search input element at line 261. 3. Test executes `userEvent.type(searchInput, 'Chart')` at line 262 without await, returning a Promise that fires keystroke events asynchronously. 4. Test continues immediately to line 263 assertion `expect(searchInput).toHaveValue('Chart')` before all keystrokes are processed. 5. In React Testing Library v14+, `userEvent.type` returns a Promise that must be awaited to ensure all input events complete; without await, the test may assert on empty or partial input value causing flaky failures. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.test.tsx **Line:** 262:262 **Comment:** *Possible Bug: Missing await on async userEvent.type call. In modern React Testing Library (v14+), userEvent.type returns a promise and must be awaited to ensure all keystrokes are processed before assertions run. Without await, the test may assert on the input value before typing completes. 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%2F37902&comment_hash=b4fb2d9e5e0524a89c33b680e1cc17bc6b6de3f00801b7f6b854a49778417cb8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37902&comment_hash=b4fb2d9e5e0524a89c33b680e1cc17bc6b6de3f00801b7f6b854a49778417cb8&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]
