bito-code-review[bot] commented on code in PR #44850: URL: https://github.com/apache/superset/pull/44850#discussion_r4150882449
########## superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.test.tsx: ########## @@ -0,0 +1,148 @@ +/** + * 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, userEvent } from 'spec/helpers/testing-library'; +import LayerConfigsControl from './LayerConfigsControl'; +import { LayerConf, WmsLayerConf } from './types'; + +const wms = ( + title: string, + url: string, + overrides: Partial<WmsLayerConf> = {}, +): LayerConf => ({ + type: 'WMS', Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Non-discriminating fixture assertion</b></div> <div id="fix"> The `wms` fixture helper hardcodes `layersParam: 'roads'` for every layer, so the edit test's `layersParam: 'roads'` assertion on `saved[1]` (the 'Rivers' layer) cannot detect a layersParam mix-up between layers and reads as a typo. Parameterize `layersParam` in the helper and give 'Rivers' a distinct value so the in-place replacement is fully discriminated. </div> </div> <small><i>Code Review Run #3bde65</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/explore/components/controls/LayerConfigsControl/LayerConfigsControl.test.tsx: ########## @@ -0,0 +1,148 @@ +/** + * 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, userEvent } from 'spec/helpers/testing-library'; +import LayerConfigsControl from './LayerConfigsControl'; +import { LayerConf, WmsLayerConf } from './types'; + +const wms = ( + title: string, + url: string, + overrides: Partial<WmsLayerConf> = {}, +): LayerConf => ({ + type: 'WMS', + version: '1.3.0', + title, + url, + layersParam: 'roads', + ...overrides, +}); + +const existing = [wms('Roads', 'https://maps.example.com/wms')]; + +const setup = (value?: LayerConf[]) => { + const onChange = jest.fn(); + render( + <LayerConfigsControl + name="layer_configs" + label="Layers" + value={value} + onChange={onChange} + />, + ); + return { onChange }; +}; + +test('renders the label and the add button', () => { + setup([]); + expect(screen.getByText('Layers')).toBeInTheDocument(); + expect( + screen.getByRole('button', { name: /Click to add new layer/ }), + ).toBeInTheDocument(); +}); + +test('lists each configured layer with its type and title', () => { + setup([...existing, wms('Rivers', 'https://x.example.com')]); + expect(screen.getAllByRole('button', { name: 'WMS' })).toHaveLength(2); + expect(screen.getByRole('button', { name: 'Roads' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Rivers' })).toBeInTheDocument(); +}); + +test('removing a layer emits the list without it', async () => { + const { onChange } = setup([ + wms('Roads', 'https://a.example.com'), + wms('Rivers', 'https://b.example.com'), + ]); + const [removeRoads] = screen.getAllByRole('button', { name: /close/i }); + await userEvent.click(removeRoads); + expect(onChange).toHaveBeenCalledWith([ + wms('Rivers', 'https://b.example.com'), + ]); +}); + +test('adding a layer opens the form and prepends the saved layer', async () => { + const { onChange } = setup(existing); + await userEvent.click( + screen.getByRole('button', { name: /Click to add new layer/ }), + ); + expect(await screen.findByText('Add Layer')).toBeInTheDocument(); + + await userEvent.type( + screen.getByPlaceholderText('Insert Layer URL'), + 'https://new.example.com/wms', + ); + await userEvent.type( + screen.getByPlaceholderText('Insert Layer title'), + 'Parcels', + ); + await userEvent.type(screen.getByPlaceholderText('Layer Name'), 'parcels'); + await userEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith([ + { + type: 'WMS', + version: '1.3.0', + title: 'Parcels', + url: 'https://new.example.com/wms', + layersParam: 'parcels', + attribution: undefined, + }, + ...existing, + ]); +}); + +test('editing a layer prefills the form and replaces the layer in place', async () => { + // Rivers differs from the defaults so the save must carry its own fields. + const { onChange } = setup([ + wms('Roads', 'https://a.example.com'), + wms('Rivers', 'https://b.example.com', { + version: '1.1.1', + layersParam: 'rivers', + }), + ]); + await userEvent.click(screen.getByRole('button', { name: 'Rivers' })); + const title = await screen.findByPlaceholderText('Insert Layer title'); + expect(title).toHaveValue('Rivers'); + + await userEvent.clear(title); + await userEvent.type(title, 'Lakes'); + await userEvent.click(screen.getByRole('button', { name: 'Save' })); + + expect(onChange).toHaveBeenCalledTimes(1); + const saved = onChange.mock.calls[0][0]; + expect(saved).toHaveLength(2); + expect(saved[0].title).toBe('Roads'); + expect(saved[1]).toMatchObject({ + type: 'WMS', + title: 'Lakes', + url: 'https://b.example.com', + version: '1.1.1', + layersParam: 'rivers', + }); +}); + Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing coverage: reorder, WFS/XYZ, undefined</b></div> <div id="fix"> The new suite covers WMS add/edit/remove/close but not the component's other behaviors: drag reorder (`onMoveLayer`/`handleDrop`), WFS and XYZ layer types, and `value={undefined}` (the `setup` helper's optional param is never exercised). Adding cases for these would match the repo's comprehensive-coverage standard and guard the reorder path, which currently has no test. </div> </div> <small><i>Code Review Run #3bde65</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/explore/components/controls/XAxisSortControl.test.tsx: ########## @@ -0,0 +1,56 @@ +/** + * 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 } from 'spec/helpers/testing-library'; +import XAxisSortControl from './XAxisSortControl'; + +const choices: [string, string][] = [ + ['metric_a', 'Metric A'], + ['metric_b', 'Metric B'], +]; + +const setup = (overrides = {}) => { + const onChange = jest.fn(); + const utils = render( + <XAxisSortControl + name="x_axis_sort" + choices={choices} + value="metric_a" + shouldReset={false} + onChange={onChange} + {...overrides} + />, + ); + return { onChange, ...utils }; +}; + +test('renders the selected sort option', () => { + setup(); + expect(screen.getByText('Metric A')).toBeInTheDocument(); +}); + +test('does not reset the value when shouldReset is false', () => { + const { onChange } = setup(); + expect(onChange).not.toHaveBeenCalled(); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing state assertion in test</b></div> <div id="fix"> The test name promises the value is not reset, but the body only asserts `onChange` was not called. Per BITO.md rule 6262, assertions should validate the behavior the name describes — add a check that 'Metric A' is still rendered so a regression clearing the value without firing `onChange` fails here. </div> </div> <small><i>Code Review Run #3bde65</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/explore/components/controls/XAxisSortControl.test.tsx: ########## @@ -0,0 +1,56 @@ +/** + * 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 } from 'spec/helpers/testing-library'; +import XAxisSortControl from './XAxisSortControl'; + +const choices: [string, string][] = [ + ['metric_a', 'Metric A'], + ['metric_b', 'Metric B'], +]; + +const setup = (overrides = {}) => { + const onChange = jest.fn(); + const utils = render( + <XAxisSortControl + name="x_axis_sort" + choices={choices} + value="metric_a" + shouldReset={false} + onChange={onChange} + {...overrides} + />, + ); + return { onChange, ...utils }; +}; + +test('renders the selected sort option', () => { + setup(); + expect(screen.getByText('Metric A')).toBeInTheDocument(); +}); + +test('does not reset the value when shouldReset is false', () => { + const { onChange } = setup(); + expect(onChange).not.toHaveBeenCalled(); +}); + +test('clears the value and notifies the parent when shouldReset is true', () => { + const { onChange } = setup({ shouldReset: true }); + expect(onChange).toHaveBeenCalledWith(undefined); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Assertion order in reset test</b></div> <div id="fix"> This test asserts `onChange` before checking the rendered value. If `XAxisSortControl` regressed in its `setValue(null)` state update, only the second assertion catches it, and a first-assertion failure reads as a mock mismatch. Assert the cleared UI first, then the `onChange(undefined)` side effect. </div> </div> <small><i>Code Review Run #3bde65</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/explore/components/controls/XAxisSortControl.test.tsx: ########## @@ -0,0 +1,56 @@ +/** + * 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 } from 'spec/helpers/testing-library'; +import XAxisSortControl from './XAxisSortControl'; + +const choices: [string, string][] = [ + ['metric_a', 'Metric A'], + ['metric_b', 'Metric B'], +]; + +const setup = (overrides = {}) => { Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Untyped test helper overrides</b></div> <div id="fix"> `overrides = {}` leaves the helper's parameter untyped, so overrides get no compile-time checking against `XAxisSortControlProps` (e.g. `shouldReset: 'yes'` would compile). dev-standard.mdc requires proper TypeScript types — annotate as `Partial<Parameters<typeof XAxisSortControl>[0]>`. </div> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/aab34ec/.cursor/rules/dev-standard.mdc#L16";>dev-standard.mdc:16</a> </li> </ul> </details> <small><i>Code Review Run #3bde65</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/explore/components/controls/ViewQueryModalFooter.test.tsx: ########## @@ -0,0 +1,107 @@ +/** + * 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 { SupersetClient } from '@superset-ui/core'; +import { + fireEvent, + render, + screen, + userEvent, +} from 'spec/helpers/testing-library'; +import ViewQueryModalFooter from './ViewQueryModalFooter'; + +const datasource = { id: '7', type: 'table', sql: 'SELECT 1' }; + +const setup = () => { + const closeModal = jest.fn(); + const changeDatasource = jest.fn(); + render( + <ViewQueryModalFooter + closeModal={closeModal} + changeDatasource={changeDatasource} + datasource={datasource} + />, + { useRouter: true }, + ); + return { closeModal, changeDatasource }; +}; + +beforeEach(() => { + window.history.replaceState(null, '', '/explore/'); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +test('renders the footer actions', () => { + setup(); + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); + expect( + screen.getByRole('button', { name: 'Save as Dataset' }), + ).toBeInTheDocument(); + expect( + screen.getByRole('button', { name: 'Open in SQL Lab' }), + ).toBeInTheDocument(); +}); + +test('Open in SQL Lab navigates in-app with the requested query', async () => { + const postForm = jest + .spyOn(SupersetClient, 'postForm') + .mockResolvedValue(undefined); + setup(); + await userEvent.click( + screen.getByRole('button', { name: 'Open in SQL Lab' }), + ); + expect(window.location.pathname).toBe('/sqllab'); + expect(window.history.state.state).toEqual({ Review Comment: <div> <div id="suggestion"> <div id="issue"><b>History v5 state shape mismatch</b></div> <div id="fix"> `window.history.state.state` reads the DOM History state, but `history.push` from `useHistory` ([email protected], per package-lock) stores location state under `usr`, not `state`. This assertion can only pass if something else wrote `state`; it verifies the wrong thing. Assert on the router location state instead. (https://github.com/remix-run/history/blob/v5.3.0/docs/getting-started.md) </div> </div> <small><i>Code Review Run #3bde65</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]
