bito-code-review[bot] commented on code in PR #40449: URL: https://github.com/apache/superset/pull/40449#discussion_r3338097303
########## superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx: ########## @@ -0,0 +1,143 @@ +/** + * 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, screen, fireEvent } from 'spec/helpers/testing-library'; +import { DrillDownBreadcrumb } from './DrillDownBreadcrumb'; +import { DrillDownLevel } from './types'; + +const hierarchy = ['country', 'region', 'city']; + +const drillStack: DrillDownLevel[] = [ + { + column: 'country', + filters: [{ col: 'country', op: '==', val: 'USA' }], + label: 'USA', + }, + { + column: 'region', + filters: [{ col: 'region', op: '==', val: 'Texas' }], + label: 'Texas', + }, +]; + +test('breadcrumb returns null when drillStack is empty and no selectedLeaf', () => { + const { container } = render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={[]} + onJumpTo={jest.fn()} + />, + ); + + expect(container.firstChild).toBeNull(); +}); + +test('breadcrumb renders hierarchy[0] and drill labels', () => { + render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={drillStack} + onJumpTo={jest.fn()} + />, + ); + + // The root column name should be rendered + expect(screen.getByText('country')).toBeInTheDocument(); + // Drill labels should be rendered + expect(screen.getByText('USA')).toBeInTheDocument(); + expect(screen.getByText('Texas')).toBeInTheDocument(); +}); + +test('clicking a breadcrumb segment calls onJumpTo with correct depth', () => { + const onJumpTo = jest.fn(); + + render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={drillStack} + onJumpTo={onJumpTo} + />, + ); + + // Click the root segment (hierarchy[0]) — should call onJumpTo(0) + fireEvent.click(screen.getByText('country')); + expect(onJumpTo).toHaveBeenCalledWith(0); + + // Click the first drill label 'USA' — should call onJumpTo(1) + fireEvent.click(screen.getByText('USA')); + expect(onJumpTo).toHaveBeenCalledWith(1); +}); + +test('keyboard Enter on breadcrumb calls onJumpTo', () => { + const onJumpTo = jest.fn(); + + render( + <DrillDownBreadcrumb + hierarchy={hierarchy} + drillStack={drillStack} + onJumpTo={onJumpTo} + />, + ); + + // Press Enter on the root segment + fireEvent.keyDown(screen.getByText('country'), { key: 'Enter' }); + expect(onJumpTo).toHaveBeenCalledWith(0); + + // Press Enter on the 'USA' segment + fireEvent.keyDown(screen.getByText('USA'), { key: 'Enter' }); + expect(onJumpTo).toHaveBeenCalledWith(1); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete keyboard accessibility coverage</b></div> <div id="fix"> The keyboard test only covers Enter key but the component also handles Space key (DrillDownBreadcrumb.tsx:79,104). Add Space key coverage for accessibility compliance. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx +++ b/superset-frontend/src/components/Chart/DrillDown/DrillDownBreadcrumb.test.tsx @@ -96,9 +96,15 @@ test('keyboard Enter on breadcrumb calls onJumpTo', () => { // Press Enter on the root segment fireEvent.keyDown(screen.getByText('country'), { key: 'Enter' }); expect(onJumpTo).toHaveBeenCalledWith(0); + onJumpTo.mockClear(); // Press Enter on the 'USA' segment fireEvent.keyDown(screen.getByText('USA'), { key: 'Enter' }); expect(onJumpTo).toHaveBeenCalledWith(1); + + // Press Space on the root segment - should also trigger onJumpTo + fireEvent.keyDown(screen.getByText('country'), { key: ' ' }); + expect(onJumpTo).toHaveBeenCalledWith(0); }); ``` </div> </details> </div> <small><i>Code Review Run #88ec4d</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/components/Chart/DrillDown/DrillDownHost.test.tsx: ########## @@ -0,0 +1,161 @@ +/** + * 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 { ReactElement } from 'react'; +import { render as rtlRender, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ThemeProvider, supersetTheme } from '@apache-superset/core/theme'; +import { QueryFormData } from '@superset-ui/core'; +import { DrillDownHost } from './DrillDownHost'; +import type { ChartRendererProps } from '../ChartRenderer'; + +function render(ui: ReactElement) { + return rtlRender(ui, { + wrapper: ({ children }) => ( + <ThemeProvider theme={supersetTheme}>{children}</ThemeProvider> + ), + }); +} + +jest.mock('src/components/Chart/chartAction', () => ({ + getChartDataRequest: jest.fn(() => + Promise.resolve({ response: {}, json: { result: [{ data: [] }] } }), + ), + handleChartDataResponse: jest.fn(() => + Promise.resolve([{ data: [{ region: 'Texas' }] }]), + ), +})); + +jest.mock('src/explore/exploreUtils', () => ({ + getQuerySettings: jest.fn(() => [false]), +})); + +jest.mock('src/utils/simpleFilterToAdhoc', () => ({ + simpleFilterToAdhoc: jest.fn(filter => ({ + expressionType: 'SIMPLE', + clause: 'WHERE', + operator: filter.op, + subject: filter.col, + comparator: filter.val, + })), +})); + +// A simple mock ChartRenderer component that renders its props for inspection +function MockChartRenderer(props: ChartRendererProps & { onDrillDown?: unknown }) { + return ( + <div data-test="mock-chart-renderer"> + <span data-test="has-on-drill-down"> + {props.onDrillDown ? 'yes' : 'no'} + </span> + <span data-test="form-data-x-axis"> + {JSON.stringify((props.formData as Record<string, unknown>).x_axis)} + </span> + </div> + ); +} + +const baseFormData: QueryFormData = { + datasource: '1__table', + viz_type: 'echarts_timeseries_bar', + slice_id: 42, + x_axis: 'country', + groupby: [], + adhoc_filters: [], +}; + +const baseRendererProps: ChartRendererProps = { + formData: baseFormData, + vizType: 'echarts_timeseries_bar', + chartId: 1, + height: 400, + width: 600, + queriesResponse: [{ data: [{ country: 'USA' }] }], + actions: {} as ChartRendererProps['actions'], +}; + +test('DrillDownHost passes through to ChartRenderer when no hierarchy', () => { + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + />, + ); + + // Should render the chart + expect(screen.getByTestId('mock-chart-renderer')).toBeInTheDocument(); + // No drill-down host wrapper when no hierarchy + expect(screen.queryByTestId('drill-down-host')).not.toBeInTheDocument(); + // onDrillDown should not be provided + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('no'); +}); + +test('DrillDownHost shows breadcrumb wrapper when hierarchy exists', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + // Should render the drill-down host wrapper + expect(screen.getByTestId('drill-down-host')).toBeInTheDocument(); + // Chart should still render + expect(screen.getByTestId('mock-chart-renderer')).toBeInTheDocument(); +}); + +test('onDrillDown is provided when hierarchy exists', () => { + const formDataWithHierarchy: QueryFormData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + }; + + render( + <DrillDownHost + ChartRendererComponent={MockChartRenderer as any} + {...baseRendererProps} + formData={formDataWithHierarchy} + />, + ); + + expect(screen.getByTestId('has-on-drill-down')).toHaveTextContent('yes'); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete test assertions</b></div> <div id="fix"> Per rule [6262], tests should verify actual business logic, not just rendering. Tests at lines 91-142 only check that elements exist or props are present (`toBeInTheDocument()`, `toHaveTextContent('yes')`). They don't verify that drilling into a level correctly updates formData or that effectiveQueriesResponse is computed properly. </div> </div> <small><i>Code Review Run #88ec4d</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/components/Chart/DrillDown/useDrillDownState.test.ts: ########## @@ -0,0 +1,321 @@ +/** + * 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 { act, renderHook, waitFor } from '@testing-library/react'; +import { QueryFormData } from '@superset-ui/core'; +import { useDrillDownState } from './useDrillDownState'; + +jest.mock('src/components/Chart/chartAction', () => ({ + getChartDataRequest: jest.fn(() => + Promise.resolve({ response: {}, json: { result: [{ data: [] }] } }), + ), + handleChartDataResponse: jest.fn(() => + Promise.resolve([{ data: [{ col1: 'val1' }] }]), + ), +})); + +jest.mock('src/explore/exploreUtils', () => ({ + getQuerySettings: jest.fn(() => [false]), +})); + +jest.mock('src/utils/simpleFilterToAdhoc', () => ({ + simpleFilterToAdhoc: jest.fn(filter => ({ + expressionType: 'SIMPLE', + clause: 'WHERE', + operator: filter.op, + subject: filter.col, + comparator: filter.val, + })), +})); + +const baseFormData: QueryFormData = { + datasource: '1__table', + viz_type: 'echarts_timeseries_bar', + slice_id: 42, + x_axis: 'country', + groupby: [], + adhoc_filters: [], +}; + +test('hierarchy is empty when x_axis is a single string with no drilldown_hierarchy', () => { + const { result } = renderHook(() => + useDrillDownState({ + formData: baseFormData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + expect(result.current.hierarchy).toEqual([]); + expect(result.current.hasHierarchy).toBe(false); +}); + +test('hierarchy is built from x_axis array with 2+ items', () => { + const formData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + expect(result.current.hierarchy).toEqual(['country', 'region', 'city']); + expect(result.current.hasHierarchy).toBe(true); +}); + +test('hierarchy is built from drilldown_hierarchy field', () => { + const formData = { + ...baseFormData, + x_axis: 'country', + drilldown_hierarchy: ['country', 'region', 'city'], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + expect(result.current.hierarchy).toEqual(['country', 'region', 'city']); + expect(result.current.hasHierarchy).toBe(true); +}); + +test('drillDown() pushes a level onto the stack', async () => { + const formData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + act(() => { + result.current.drillDown( + [{ col: 'country', op: '==', val: 'USA' }], + 'USA', + ); + }); + + expect(result.current.drillStack).toHaveLength(1); + expect(result.current.drillStack[0]).toEqual({ + column: 'country', + filters: [{ col: 'country', op: '==', val: 'USA' }], + label: 'USA', + }); + expect(result.current.isDrilling).toBe(true); +}); + +test('drillDown() at last level sets selectedLeaf instead of pushing', () => { + const formData = { + ...baseFormData, + x_axis: ['country', 'region'], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + // Drill to depth 1 (only one more level available: 'region') + act(() => { + result.current.drillDown( + [{ col: 'country', op: '==', val: 'USA' }], + 'USA', + ); + }); + + expect(result.current.drillStack).toHaveLength(1); + + // Now we're at the last level; next drill should set selectedLeaf + act(() => { + result.current.drillDown( + [{ col: 'region', op: '==', val: 'Texas' }], + 'Texas', + ); + }); + + expect(result.current.drillStack).toHaveLength(1); + expect(result.current.selectedLeaf).toBe('Texas'); +}); + +test('resetTo(0) clears the stack', () => { + const formData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + act(() => { + result.current.drillDown( + [{ col: 'country', op: '==', val: 'USA' }], + 'USA', + ); + }); + + expect(result.current.drillStack).toHaveLength(1); + + act(() => { + result.current.resetTo(0); + }); + + expect(result.current.drillStack).toHaveLength(0); + expect(result.current.isDrilling).toBe(false); + expect(result.current.selectedLeaf).toBeUndefined(); +}); + +test('effectiveFormData swaps x_axis when drilling', () => { + const formData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + groupby: [], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + act(() => { + result.current.drillDown( + [{ col: 'country', op: '==', val: 'USA' }], + 'USA', + ); + }); + + // After drilling one level, the effective x_axis should be 'region' + const effective = result.current.effectiveFormData as Record<string, unknown>; + expect(effective.x_axis).toBe('region'); +}); + +test('effectiveFormData adds adhoc_filters from drill stack', () => { + const formData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + groupby: [], + adhoc_filters: [{ expressionType: 'SIMPLE' as const, clause: 'WHERE' as const, subject: 'year', operator: '==' as const, comparator: '2023' }], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + act(() => { + result.current.drillDown( + [{ col: 'country', op: '==', val: 'USA' }], + 'USA', + ); + }); + + const effective = result.current.effectiveFormData as Record<string, unknown>; + const adhocFilters = effective.adhoc_filters as unknown[]; + + // Should contain the original filter plus the drill filter + expect(adhocFilters).toHaveLength(2); + // The first should be the original filter + expect(adhocFilters[0]).toEqual( + expect.objectContaining({ subject: 'year' }), + ); + // The second should be generated from the drill filter via simpleFilterToAdhoc + expect(adhocFilters[1]).toEqual( + expect.objectContaining({ subject: 'country', operator: '==', comparator: 'USA' }), + ); +}); + +test('effectiveFormData swaps groupby when chart uses groupby', () => { + const formData = { + ...baseFormData, + x_axis: undefined, + groupby: ['country'], + drilldown_hierarchy: ['country', 'region', 'city'], + adhoc_filters: [], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [] }], + }), + ); + + act(() => { + result.current.drillDown( + [{ col: 'country', op: '==', val: 'USA' }], + 'USA', + ); + }); + + const effective = result.current.effectiveFormData as Record<string, unknown>; + expect(effective.groupby).toEqual(['region']); +}); + +test('fetches data when drilling and returns it as effectiveQueriesResponse', async () => { + const formData = { + ...baseFormData, + x_axis: ['country', 'region', 'city'], + groupby: [], + adhoc_filters: [], + }; + + const { result } = renderHook(() => + useDrillDownState({ + formData, + baseQueriesResponse: [{ data: [{ country: 'USA' }] }], + }), + ); + + // Before drilling, effectiveQueriesResponse is the base data + expect(result.current.effectiveQueriesResponse).toEqual([ + { data: [{ country: 'USA' }] }, + ]); + + act(() => { + result.current.drillDown( + [{ col: 'country', op: '==', val: 'USA' }], + 'USA', + ); + }); + + await waitFor(() => { + expect(result.current.isLoading).toBe(false); + }); + + expect(result.current.effectiveQueriesResponse).toEqual([ + { data: [{ col1: 'val1' }] }, + ]); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Add tests for uncovered paths</b></div> <div id="fix"> Coverage gap: `reset()` (useDrillDownState.ts:258-261), `error` state on fetch failure, `hasMoreLevels` property, single-item `x_axis` array edge case, and formData-change reset are all untested. Add tests to cover these code paths per rule [6262]. </div> </div> <small><i>Code Review Run #88ec4d</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]
