bito-code-review[bot] commented on code in PR #36996: URL: https://github.com/apache/superset/pull/36996#discussion_r2688897140
########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.test.tsx: ########## @@ -0,0 +1,259 @@ +/** + * 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 { render } from 'spec/helpers/testing-library'; +import { Provider } from 'react-redux'; +import { Store } from 'redux'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { FilterBarOrientation } from 'src/dashboard/types'; +import FilterControls from './FilterControls'; + +const mockStore = configureStore([thunk]); + +const createMockFilter = (id: string, name: string) => ({ + id, + name, + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'country' } }], + defaultDataMask: {}, + controlValues: {}, + cascadeParentIds: [], + scope: { + rootPath: ['ROOT_ID'], + excluded: [] as string[], + }, + isInstant: true, + allowsMultipleValues: true, + isRequired: false, +}); + +const getDefaultState = (orientation: FilterBarOrientation) => ({ + dashboardInfo: { + id: 1, + filterBarOrientation: orientation, + }, + dashboardLayout: { + present: { + ROOT_ID: { + type: 'ROOT', + id: 'ROOT_ID', + children: ['TABS-1'], + }, + 'TABS-1': { + type: 'TABS', + id: 'TABS-1', + children: ['TAB-1', 'TAB-2'], + }, + 'TAB-1': { + type: 'TAB', + id: 'TAB-1', + children: ['CHART-1'], + }, + 'TAB-2': { + type: 'TAB', + id: 'TAB-2', + children: ['CHART-2'], + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { chartId: 1 }, + }, + 'CHART-2': { + type: 'CHART', + id: 'CHART-2', + meta: { chartId: 2 }, + }, + }, + }, + charts: { + 1: { id: 1, formData: {} }, + 2: { id: 2, formData: {} }, + }, + dataMask: {}, + nativeFilters: { + filters: { + 'filter-1': createMockFilter('filter-1', 'Country Filter'), + 'filter-2': createMockFilter('filter-2', 'Region Filter'), + 'filter-3': createMockFilter('filter-3', 'City Filter'), + }, + filterSets: {}, + }, + dashboardState: { + directPathToChild: [], + activeTabs: ['TAB-1'], + chartCustomizationItems: [], + }, + sliceEntities: { + slices: { + 1: { + slice_id: 1, + slice_name: 'Chart 1', + form_data: {}, + }, + 2: { + slice_id: 2, + slice_name: 'Chart 2', + form_data: {}, + }, + }, + }, + datasources: {}, +}); + +function setup(overrideState: any = {}, props: any = {}) { + const state = { + ...getDefaultState(FilterBarOrientation.Vertical), + ...overrideState, + }; + const store = mockStore(state) as Store; + + return render( + <Provider store={store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + {...props} + /> + </Provider>, + ); +} + +test('FilterControls should mark out-of-scope filters as not overflowed in vertical mode', () => { + const stateWithVertical = getDefaultState(FilterBarOrientation.Vertical); + + // Set up filters where filter-3 is out of scope (not on active tab) + stateWithVertical.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], // Exclude from active tab + }; + + const { container } = setup(stateWithVertical); + + // In vertical mode, out-of-scope filters should be in the collapse panel + // not in the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render without errors and properly calculate + // that out-of-scope filters are not "overflowed" in vertical mode +}); + +test('FilterControls should mark out-of-scope filters as overflowed in horizontal mode', () => { + const stateWithHorizontal = getDefaultState(FilterBarOrientation.Horizontal); + + // Set up filters where filter-3 is out of scope + stateWithHorizontal.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + const { container } = setup(stateWithHorizontal); + + // In horizontal mode, out-of-scope filters go to the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render and treat out-of-scope filters as overflowed +}); + +test('FilterControls overflowedByIndex calculation respects filter bar orientation', () => { + // Test vertical orientation + const verticalState = getDefaultState(FilterBarOrientation.Vertical); + verticalState.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + const { rerender, container } = setup(verticalState); + expect(container).toBeInTheDocument(); + + // Test horizontal orientation with same filter setup + const horizontalState = getDefaultState(FilterBarOrientation.Horizontal); + horizontalState.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + rerender( + <Provider store={mockStore(horizontalState) as Store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + /> + </Provider>, + ); + + // Component should handle orientation change without errors + expect(container).toBeInTheDocument(); +}); + +test('FilterControls should correctly pass isOverflowing prop to filter controls', () => { + const state = getDefaultState(FilterBarOrientation.Vertical); + + // Create a mix of in-scope and out-of-scope filters + state.nativeFilters.filters['filter-1'].scope = { + rootPath: ['ROOT_ID'], + excluded: [], + }; + + state.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], // Out of scope + }; + + const { container } = setup(state); + + // Verify the component renders correctly + // The filter controls should receive the correct overflow status + // based on their position (in-scope vs out-of-scope) and orientation + expect(container).toBeInTheDocument(); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete Test Assertion</b></div> <div id="fix"> The test 'FilterControls should correctly pass isOverflowing prop to filter controls' only checks that the component renders, but does not verify the `isOverflowing` prop passing as described. The component passes `overflowedByIndex[index]` to `filterControlFactory` (line 559 in FilterControls.tsx), but the test does not assert this, potentially missing prop-passing bugs. </div> </div> <small><i>Code Review Run #20a962</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.test.tsx: ########## @@ -0,0 +1,259 @@ +/** + * 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 { render } from 'spec/helpers/testing-library'; +import { Provider } from 'react-redux'; +import { Store } from 'redux'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { FilterBarOrientation } from 'src/dashboard/types'; +import FilterControls from './FilterControls'; + +const mockStore = configureStore([thunk]); + +const createMockFilter = (id: string, name: string) => ({ + id, + name, + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'country' } }], + defaultDataMask: {}, + controlValues: {}, + cascadeParentIds: [], + scope: { + rootPath: ['ROOT_ID'], + excluded: [] as string[], + }, + isInstant: true, + allowsMultipleValues: true, + isRequired: false, +}); + +const getDefaultState = (orientation: FilterBarOrientation) => ({ + dashboardInfo: { + id: 1, + filterBarOrientation: orientation, + }, + dashboardLayout: { + present: { + ROOT_ID: { + type: 'ROOT', + id: 'ROOT_ID', + children: ['TABS-1'], + }, + 'TABS-1': { + type: 'TABS', + id: 'TABS-1', + children: ['TAB-1', 'TAB-2'], + }, + 'TAB-1': { + type: 'TAB', + id: 'TAB-1', + children: ['CHART-1'], + }, + 'TAB-2': { + type: 'TAB', + id: 'TAB-2', + children: ['CHART-2'], + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { chartId: 1 }, + }, + 'CHART-2': { + type: 'CHART', + id: 'CHART-2', + meta: { chartId: 2 }, + }, + }, + }, + charts: { + 1: { id: 1, formData: {} }, + 2: { id: 2, formData: {} }, + }, + dataMask: {}, + nativeFilters: { + filters: { + 'filter-1': createMockFilter('filter-1', 'Country Filter'), + 'filter-2': createMockFilter('filter-2', 'Region Filter'), + 'filter-3': createMockFilter('filter-3', 'City Filter'), + }, + filterSets: {}, + }, + dashboardState: { + directPathToChild: [], + activeTabs: ['TAB-1'], + chartCustomizationItems: [], + }, + sliceEntities: { + slices: { + 1: { + slice_id: 1, + slice_name: 'Chart 1', + form_data: {}, + }, + 2: { + slice_id: 2, + slice_name: 'Chart 2', + form_data: {}, + }, + }, + }, + datasources: {}, +}); + +function setup(overrideState: any = {}, props: any = {}) { + const state = { + ...getDefaultState(FilterBarOrientation.Vertical), + ...overrideState, + }; + const store = mockStore(state) as Store; + + return render( + <Provider store={store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + {...props} + /> + </Provider>, + ); +} + +test('FilterControls should mark out-of-scope filters as not overflowed in vertical mode', () => { + const stateWithVertical = getDefaultState(FilterBarOrientation.Vertical); + + // Set up filters where filter-3 is out of scope (not on active tab) + stateWithVertical.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], // Exclude from active tab + }; + + const { container } = setup(stateWithVertical); + + // In vertical mode, out-of-scope filters should be in the collapse panel + // not in the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render without errors and properly calculate + // that out-of-scope filters are not "overflowed" in vertical mode +}); + +test('FilterControls should mark out-of-scope filters as overflowed in horizontal mode', () => { + const stateWithHorizontal = getDefaultState(FilterBarOrientation.Horizontal); + + // Set up filters where filter-3 is out of scope + stateWithHorizontal.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + const { container } = setup(stateWithHorizontal); + + // In horizontal mode, out-of-scope filters go to the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render and treat out-of-scope filters as overflowed +}); + +test('FilterControls overflowedByIndex calculation respects filter bar orientation', () => { + // Test vertical orientation + const verticalState = getDefaultState(FilterBarOrientation.Vertical); + verticalState.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + const { rerender, container } = setup(verticalState); + expect(container).toBeInTheDocument(); + + // Test horizontal orientation with same filter setup + const horizontalState = getDefaultState(FilterBarOrientation.Horizontal); + horizontalState.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + rerender( + <Provider store={mockStore(horizontalState) as Store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + /> + </Provider>, + ); + + // Component should handle orientation change without errors + expect(container).toBeInTheDocument(); +}); + +test('FilterControls should correctly pass isOverflowing prop to filter controls', () => { + const state = getDefaultState(FilterBarOrientation.Vertical); + + // Create a mix of in-scope and out-of-scope filters + state.nativeFilters.filters['filter-1'].scope = { + rootPath: ['ROOT_ID'], + excluded: [], + }; + + state.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], // Out of scope + }; + + const { container } = setup(state); + + // Verify the component renders correctly + // The filter controls should receive the correct overflow status + // based on their position (in-scope vs out-of-scope) and orientation + expect(container).toBeInTheDocument(); +}); + +test('FilterControls should handle empty filters list', () => { + const state = getDefaultState(FilterBarOrientation.Vertical); + state.nativeFilters.filters = {} as any; + + const { container } = setup(state); + expect(container).toBeInTheDocument(); +}); + +test('FilterControls overflowedByIndex updates when filters change scope', () => { + const state = getDefaultState(FilterBarOrientation.Vertical); + + const { container, rerender } = setup(state); + expect(container).toBeInTheDocument(); + + // Change filter scope + const updatedState = getDefaultState(FilterBarOrientation.Vertical); + updatedState.nativeFilters.filters['filter-1'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + rerender( + <Provider store={mockStore(updatedState) as Store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + /> + </Provider>, + ); + + // Component should update overflow status when filters change + expect(container).toBeInTheDocument(); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete Test Assertion</b></div> <div id="fix"> The test 'FilterControls overflowedByIndex updates when filters change scope' only checks that the component renders, but does not verify the `overflowedByIndex` update as described. The component recalculates `overflowedByIndex` on filter changes (lines 518-542 in FilterControls.tsx), but the test does not assert this, potentially missing update bugs. </div> </div> <small><i>Code Review Run #20a962</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.test.tsx: ########## @@ -0,0 +1,259 @@ +/** + * 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 { render } from 'spec/helpers/testing-library'; +import { Provider } from 'react-redux'; +import { Store } from 'redux'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { FilterBarOrientation } from 'src/dashboard/types'; +import FilterControls from './FilterControls'; + +const mockStore = configureStore([thunk]); + +const createMockFilter = (id: string, name: string) => ({ + id, + name, + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'country' } }], + defaultDataMask: {}, + controlValues: {}, + cascadeParentIds: [], + scope: { + rootPath: ['ROOT_ID'], + excluded: [] as string[], + }, + isInstant: true, + allowsMultipleValues: true, + isRequired: false, +}); + +const getDefaultState = (orientation: FilterBarOrientation) => ({ + dashboardInfo: { + id: 1, + filterBarOrientation: orientation, + }, + dashboardLayout: { + present: { + ROOT_ID: { + type: 'ROOT', + id: 'ROOT_ID', + children: ['TABS-1'], + }, + 'TABS-1': { + type: 'TABS', + id: 'TABS-1', + children: ['TAB-1', 'TAB-2'], + }, + 'TAB-1': { + type: 'TAB', + id: 'TAB-1', + children: ['CHART-1'], + }, + 'TAB-2': { + type: 'TAB', + id: 'TAB-2', + children: ['CHART-2'], + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { chartId: 1 }, + }, + 'CHART-2': { + type: 'CHART', + id: 'CHART-2', + meta: { chartId: 2 }, + }, + }, + }, + charts: { + 1: { id: 1, formData: {} }, + 2: { id: 2, formData: {} }, + }, + dataMask: {}, + nativeFilters: { + filters: { + 'filter-1': createMockFilter('filter-1', 'Country Filter'), + 'filter-2': createMockFilter('filter-2', 'Region Filter'), + 'filter-3': createMockFilter('filter-3', 'City Filter'), + }, + filterSets: {}, + }, + dashboardState: { + directPathToChild: [], + activeTabs: ['TAB-1'], + chartCustomizationItems: [], + }, + sliceEntities: { + slices: { + 1: { + slice_id: 1, + slice_name: 'Chart 1', + form_data: {}, + }, + 2: { + slice_id: 2, + slice_name: 'Chart 2', + form_data: {}, + }, + }, + }, + datasources: {}, +}); + +function setup(overrideState: any = {}, props: any = {}) { + const state = { + ...getDefaultState(FilterBarOrientation.Vertical), + ...overrideState, + }; + const store = mockStore(state) as Store; + + return render( + <Provider store={store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + {...props} + /> + </Provider>, + ); +} + +test('FilterControls should mark out-of-scope filters as not overflowed in vertical mode', () => { + const stateWithVertical = getDefaultState(FilterBarOrientation.Vertical); + + // Set up filters where filter-3 is out of scope (not on active tab) + stateWithVertical.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], // Exclude from active tab + }; + + const { container } = setup(stateWithVertical); + + // In vertical mode, out-of-scope filters should be in the collapse panel + // not in the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render without errors and properly calculate + // that out-of-scope filters are not "overflowed" in vertical mode +}); + +test('FilterControls should mark out-of-scope filters as overflowed in horizontal mode', () => { + const stateWithHorizontal = getDefaultState(FilterBarOrientation.Horizontal); + + // Set up filters where filter-3 is out of scope + stateWithHorizontal.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + const { container } = setup(stateWithHorizontal); + + // In horizontal mode, out-of-scope filters go to the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render and treat out-of-scope filters as overflowed +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete Test Assertion</b></div> <div id="fix"> The test 'FilterControls should mark out-of-scope filters as overflowed in horizontal mode' only checks that the component renders, but does not verify the overflow logic as described. The component's `overflowedByIndex` calculation (lines 518-542 in FilterControls.tsx) determines overflow based on orientation and scope, but the test does not assert this behavior, potentially missing bugs in overflow handling. </div> </div> <small><i>Code Review Run #20a962</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.test.tsx: ########## @@ -0,0 +1,259 @@ +/** + * 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 { render } from 'spec/helpers/testing-library'; +import { Provider } from 'react-redux'; +import { Store } from 'redux'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { FilterBarOrientation } from 'src/dashboard/types'; +import FilterControls from './FilterControls'; + +const mockStore = configureStore([thunk]); + +const createMockFilter = (id: string, name: string) => ({ + id, + name, + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'country' } }], + defaultDataMask: {}, + controlValues: {}, + cascadeParentIds: [], + scope: { + rootPath: ['ROOT_ID'], + excluded: [] as string[], + }, + isInstant: true, + allowsMultipleValues: true, + isRequired: false, +}); + +const getDefaultState = (orientation: FilterBarOrientation) => ({ + dashboardInfo: { + id: 1, + filterBarOrientation: orientation, + }, + dashboardLayout: { + present: { + ROOT_ID: { + type: 'ROOT', + id: 'ROOT_ID', + children: ['TABS-1'], + }, + 'TABS-1': { + type: 'TABS', + id: 'TABS-1', + children: ['TAB-1', 'TAB-2'], + }, + 'TAB-1': { + type: 'TAB', + id: 'TAB-1', + children: ['CHART-1'], + }, + 'TAB-2': { + type: 'TAB', + id: 'TAB-2', + children: ['CHART-2'], + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { chartId: 1 }, + }, + 'CHART-2': { + type: 'CHART', + id: 'CHART-2', + meta: { chartId: 2 }, + }, + }, + }, + charts: { + 1: { id: 1, formData: {} }, + 2: { id: 2, formData: {} }, + }, + dataMask: {}, + nativeFilters: { + filters: { + 'filter-1': createMockFilter('filter-1', 'Country Filter'), + 'filter-2': createMockFilter('filter-2', 'Region Filter'), + 'filter-3': createMockFilter('filter-3', 'City Filter'), + }, + filterSets: {}, + }, + dashboardState: { + directPathToChild: [], + activeTabs: ['TAB-1'], + chartCustomizationItems: [], + }, + sliceEntities: { + slices: { + 1: { + slice_id: 1, + slice_name: 'Chart 1', + form_data: {}, + }, + 2: { + slice_id: 2, + slice_name: 'Chart 2', + form_data: {}, + }, + }, + }, + datasources: {}, +}); + +function setup(overrideState: any = {}, props: any = {}) { + const state = { + ...getDefaultState(FilterBarOrientation.Vertical), + ...overrideState, + }; + const store = mockStore(state) as Store; + + return render( + <Provider store={store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + {...props} + /> + </Provider>, + ); +} + +test('FilterControls should mark out-of-scope filters as not overflowed in vertical mode', () => { + const stateWithVertical = getDefaultState(FilterBarOrientation.Vertical); + + // Set up filters where filter-3 is out of scope (not on active tab) + stateWithVertical.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'], // Exclude from active tab + }; + + const { container } = setup(stateWithVertical); + + // In vertical mode, out-of-scope filters should be in the collapse panel + // not in the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render without errors and properly calculate + // that out-of-scope filters are not "overflowed" in vertical mode +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test: Weak assertions</b></div> <div id="fix"> Test assertions are too weak - they only check that the component renders but don't verify the overflow behavior claimed in test names. Consider checking rendered components or props. </div> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/31a2775/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run #ab7b15</i></small> </div><div> <div id="suggestion"> <div id="issue"><b>Incomplete Test Assertion</b></div> <div id="fix"> The test 'FilterControls should mark out-of-scope filters as not overflowed in vertical mode' only checks that the component renders, but does not verify the overflow logic as described. The component's `overflowedByIndex` calculation (lines 518-542 in FilterControls.tsx) determines overflow based on orientation and scope, but the test does not assert this behavior, potentially missing bugs in overflow handling. </div> </div> <small><i>Code Review Run #20a962</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.test.tsx: ########## @@ -0,0 +1,259 @@ +/** + * 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 { render } from 'spec/helpers/testing-library'; +import { Provider } from 'react-redux'; +import { Store } from 'redux'; +import configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { FilterBarOrientation } from 'src/dashboard/types'; +import FilterControls from './FilterControls'; + +const mockStore = configureStore([thunk]); + +const createMockFilter = (id: string, name: string) => ({ + id, + name, + filterType: 'filter_select', + targets: [{ datasetId: 1, column: { name: 'country' } }], + defaultDataMask: {}, + controlValues: {}, + cascadeParentIds: [], + scope: { + rootPath: ['ROOT_ID'], + excluded: [] as string[], + }, + isInstant: true, + allowsMultipleValues: true, + isRequired: false, +}); + +const getDefaultState = (orientation: FilterBarOrientation) => ({ + dashboardInfo: { + id: 1, + filterBarOrientation: orientation, + }, + dashboardLayout: { + present: { + ROOT_ID: { + type: 'ROOT', + id: 'ROOT_ID', + children: ['TABS-1'], + }, + 'TABS-1': { + type: 'TABS', + id: 'TABS-1', + children: ['TAB-1', 'TAB-2'], + }, + 'TAB-1': { + type: 'TAB', + id: 'TAB-1', + children: ['CHART-1'], + }, + 'TAB-2': { + type: 'TAB', + id: 'TAB-2', + children: ['CHART-2'], + }, + 'CHART-1': { + type: 'CHART', + id: 'CHART-1', + meta: { chartId: 1 }, + }, + 'CHART-2': { + type: 'CHART', + id: 'CHART-2', + meta: { chartId: 2 }, + }, + }, + }, + charts: { + 1: { id: 1, formData: {} }, + 2: { id: 2, formData: {} }, + }, + dataMask: {}, + nativeFilters: { + filters: { + 'filter-1': createMockFilter('filter-1', 'Country Filter'), + 'filter-2': createMockFilter('filter-2', 'Region Filter'), + 'filter-3': createMockFilter('filter-3', 'City Filter'), + }, + filterSets: {}, + }, + dashboardState: { + directPathToChild: [], + activeTabs: ['TAB-1'], + chartCustomizationItems: [], + }, + sliceEntities: { + slices: { + 1: { + slice_id: 1, + slice_name: 'Chart 1', + form_data: {}, + }, + 2: { + slice_id: 2, + slice_name: 'Chart 2', + form_data: {}, + }, + }, + }, + datasources: {}, +}); + +function setup(overrideState: any = {}, props: any = {}) { + const state = { + ...getDefaultState(FilterBarOrientation.Vertical), + ...overrideState, + }; + const store = mockStore(state) as Store; + + return render( + <Provider store={store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + {...props} + /> + </Provider>, + ); +} + +test('FilterControls should mark out-of-scope filters as not overflowed in vertical mode', () => { + const stateWithVertical = getDefaultState(FilterBarOrientation.Vertical); + + // Set up filters where filter-3 is out of scope (not on active tab) + stateWithVertical.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], // Exclude from active tab + }; + + const { container } = setup(stateWithVertical); + + // In vertical mode, out-of-scope filters should be in the collapse panel + // not in the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render without errors and properly calculate + // that out-of-scope filters are not "overflowed" in vertical mode +}); + +test('FilterControls should mark out-of-scope filters as overflowed in horizontal mode', () => { + const stateWithHorizontal = getDefaultState(FilterBarOrientation.Horizontal); + + // Set up filters where filter-3 is out of scope + stateWithHorizontal.nativeFilters.filters['filter-3'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + const { container } = setup(stateWithHorizontal); + + // In horizontal mode, out-of-scope filters go to the overflow dropdown + expect(container).toBeInTheDocument(); + + // The component should render and treat out-of-scope filters as overflowed +}); + +test('FilterControls overflowedByIndex calculation respects filter bar orientation', () => { + // Test vertical orientation + const verticalState = getDefaultState(FilterBarOrientation.Vertical); + verticalState.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + const { rerender, container } = setup(verticalState); + expect(container).toBeInTheDocument(); + + // Test horizontal orientation with same filter setup + const horizontalState = getDefaultState(FilterBarOrientation.Horizontal); + horizontalState.nativeFilters.filters['filter-2'].scope = { + rootPath: ['ROOT_ID'], + excluded: ['TAB-1'] as string[], + }; + + rerender( + <Provider store={mockStore(horizontalState) as Store}> + <FilterControls + dataMaskSelected={{}} + onFilterSelectionChange={jest.fn()} + /> + </Provider>, + ); + + // Component should handle orientation change without errors + expect(container).toBeInTheDocument(); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete Test Assertion</b></div> <div id="fix"> The test 'FilterControls overflowedByIndex calculation respects filter bar orientation' only checks that the component renders, but does not verify the overflow calculation as described. The component's `overflowedByIndex` (lines 518-542 in FilterControls.tsx) depends on orientation, but the test does not assert this, potentially missing orientation-related bugs. </div> </div> <small><i>Code Review Run #20a962</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
