bito-code-review[bot] commented on code in PR #35525: URL: https://github.com/apache/superset/pull/35525#discussion_r2658864736
########## superset-frontend/plugins/legacy-plugin-chart-map-box/test/MapBox.test.tsx: ########## @@ -0,0 +1,466 @@ +/** + * 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, waitFor } from 'spec/helpers/testing-library'; +import MapBox, { + Clusterer, + DEFAULT_MAX_ZOOM, + DEFAULT_POINT_RADIUS, +} from '../src/MapBox'; +import { type Location } from '../src/ScatterPlotGlowOverlay'; + +type BBox = [number, number, number, number]; + +const mockMap = { + on: jest.fn(), + off: jest.fn(), + project: jest.fn((coords: [number, number]) => ({ + x: coords[0], + y: coords[1], + })), + getCanvas: jest.fn(() => ({ + style: {}, + })), + getContainer: jest.fn(() => document.createElement('div')), +}; + +const MockMap = ({ children, onMove, onIdle }: any) => { + const { useEffect } = require('react'); + useEffect(() => { + onIdle?.(); + }, [onIdle]); + return ( + <div + data-test="mapbox-mock" + onClick={() => + onMove?.({ viewState: { latitude: 0, longitude: 0, zoom: 5 } }) + } + > + {children} + </div> + ); +}; + +jest.mock('react-map-gl/mapbox', () => ({ + __esModule: true, + default: (props: any) => <MockMap {...props} />, + useMap: () => ({ current: mockMap }), +})); + +jest.mock('supercluster'); + +const mockGetClusters = jest.fn<Location[], [BBox, number]>(() => []); +const mockClusterer: Clusterer = { + getClusters: mockGetClusters, +}; + +const defaultProps = { + bounds: [ + [-122.5, 37.5], + [-122.0, 38.0], + ] as [[number, number], [number, number]], + clusterer: mockClusterer, + mapboxApiKey: 'test-api-key', + width: 800, + height: 600, +}; + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('renders without crashing', () => { + const { getByTestId } = render(<MapBox {...defaultProps} />); + expect(getByTestId('mapbox-mock')).toBeInTheDocument(); +}); + +test('initializes viewport from bounds', () => { + const { container } = render(<MapBox {...defaultProps} />); + expect( + container.querySelector('[data-test="mapbox-mock"]'), + ).toBeInTheDocument(); +}); + +test('renders with default width and height when not provided', () => { + const props = { + ...defaultProps, + width: undefined, + height: undefined, + }; + const { container } = render(<MapBox {...props} />); + expect( + container.querySelector('[data-test="mapbox-mock"]'), + ).toBeInTheDocument(); +}); + +test('calls clusterer.getClusters with correct bbox and zoom', () => { + render(<MapBox {...defaultProps} />); + + expect(mockGetClusters).toHaveBeenCalled(); + const [bbox, zoom] = mockGetClusters.mock.calls[0]; + + expect(Array.isArray(bbox)).toBe(true); + expect(bbox.length).toBe(4); + expect(typeof zoom).toBe('number'); +}); + +test('handles viewport change events', async () => { + const onViewportChange = jest.fn(); + const { getByTestId } = render( + <MapBox {...defaultProps} onViewportChange={onViewportChange} />, + ); + + const mapElement = getByTestId('mapbox-mock'); + mapElement.click(); + + await waitFor(() => { + expect(onViewportChange).toHaveBeenCalled(); + }); +}); + +test('updates state when viewport changes', async () => { + const { getByTestId } = render(<MapBox {...defaultProps} />); + + const mapElement = getByTestId('mapbox-mock'); + mapElement.click(); + + await waitFor(() => { + expect(getByTestId('mapbox-mock')).toBeInTheDocument(); + }); +}); + +test('uses DEFAULT_POINT_RADIUS when pointRadius not provided', () => { + const { container } = render(<MapBox {...defaultProps} />); + expect( + container.querySelector('[data-test="mapbox-mock"]'), + ).toBeInTheDocument(); + expect(DEFAULT_POINT_RADIUS).toBe(60); +}); + +test('uses provided pointRadius value', () => { + const { container } = render(<MapBox {...defaultProps} pointRadius={100} />); + expect( + container.querySelector('[data-test="mapbox-mock"]'), + ).toBeInTheDocument(); +}); + +test('passes rgb prop to ScatterPlotGlowOverlay', () => { + const rgb: [number, number, number, number] = [255, 128, 64, 255]; + const { container } = render(<MapBox {...defaultProps} rgb={rgb} />); + expect( + container.querySelector('[data-test="mapbox-mock"]'), + ).toBeInTheDocument(); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete prop passing verification</b></div> <div id="fix"> Tests like 'passes rgb prop to ScatterPlotGlowOverlay' only check that the component renders but don't verify that props are actually passed to child components. This reduces test coverage; consider mocking ScatterPlotGlowOverlay to assert prop forwarding. </div> </div> <small><i>Code Review Run #fabf38</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/plugins/legacy-plugin-chart-map-box/test/ScatterPlotGlowOverlay.test.tsx: ########## @@ -0,0 +1,586 @@ +/** + * 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 ScatterPlotGlowOverlay, { + AggregationType, +} from '../src/ScatterPlotGlowOverlay'; + +jest.mock('react-map-gl/mapbox', () => ({ + useMap: () => ({ + current: { + project: (coords: [number, number]) => ({ + x: coords[0] * 10, + y: coords[1] * 10, + }), + on: jest.fn(), + off: jest.fn(), + }, + }), +})); + +const mockLocation = { + geometry: { + coordinates: [10, 20] as [number, number], + type: 'Point', + }, + properties: {}, +}; + +const mockClusterLocation = { + geometry: { + coordinates: [10, 20] as [number, number], + type: 'Point', + }, + properties: { + cluster: true, + point_count: 10, + sum: 100, + squaredSum: 1200, + min: 5, + max: 20, + }, +}; + +test('renders without crashing', () => { + const { container } = render( + <ScatterPlotGlowOverlay locations={[]} rgb={[255, 0, 0, 255]} />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders canvas element', () => { + const { container } = render( + <ScatterPlotGlowOverlay + locations={[mockLocation]} + rgb={[255, 128, 64, 255]} + />, + ); + + const canvas = container.querySelector('canvas'); + expect(canvas).toBeInTheDocument(); + expect(canvas?.tagName).toBe('CANVAS'); +}); + +test('renders with default props', () => { + const { container } = render(<ScatterPlotGlowOverlay locations={[]} />); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders with empty locations array', () => { + const { container } = render( + <ScatterPlotGlowOverlay locations={[]} rgb={[255, 0, 0, 255]} />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders with single location', () => { + const { container } = render( + <ScatterPlotGlowOverlay + locations={[mockLocation]} + rgb={[255, 0, 0, 255]} + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders with multiple locations', () => { + const locations = [mockLocation, { ...mockLocation }]; + const { container } = render( + <ScatterPlotGlowOverlay locations={locations} rgb={[255, 0, 0, 255]} />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders with cluster location', () => { + const { container } = render( + <ScatterPlotGlowOverlay + locations={[mockClusterLocation]} + rgb={[255, 0, 0, 255]} + aggregation="sum" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('computes cluster label as point_count when no aggregation specified', () => { + const { container } = render( + <ScatterPlotGlowOverlay + locations={[mockClusterLocation]} + rgb={[255, 0, 0, 255]} + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('computes cluster label using sum aggregation', () => { + const location = { + ...mockClusterLocation, + properties: { + ...mockClusterLocation.properties, + sum: 150, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + aggregation="sum" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('computes cluster label using min aggregation', () => { + const location = { + ...mockClusterLocation, + properties: { + ...mockClusterLocation.properties, + min: 5, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + aggregation="min" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('computes cluster label using max aggregation', () => { + const location = { + ...mockClusterLocation, + properties: { + ...mockClusterLocation.properties, + max: 20, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + aggregation="max" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('computes cluster label using mean aggregation', () => { + const location = { + ...mockClusterLocation, + properties: { + cluster: true, + point_count: 10, + sum: 100, + squaredSum: 1200, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + aggregation="mean" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('computes cluster label using variance aggregation', () => { + const location = { + ...mockClusterLocation, + properties: { + cluster: true, + point_count: 10, + sum: 100, + squaredSum: 1200, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + aggregation="var" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('computes cluster label using stdev aggregation', () => { + const location = { + ...mockClusterLocation, + properties: { + cluster: true, + point_count: 10, + sum: 100, + squaredSum: 1200, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + aggregation="stdev" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('uses white text color on dark background (low luminance)', () => { + const darkRgb: [number, number, number, number] = [255, 10, 10, 10]; + const { container } = render( + <ScatterPlotGlowOverlay + locations={[mockClusterLocation]} + rgb={darkRgb} + aggregation="sum" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('uses black text color on light background (high luminance)', () => { + const lightRgb: [number, number, number, number] = [255, 240, 240, 240]; + const { container } = render( + <ScatterPlotGlowOverlay + locations={[mockClusterLocation]} + rgb={lightRgb} + aggregation="sum" + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders point with custom radius', () => { + const location = { + ...mockLocation, + properties: { + radius: 50, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + dotRadius={100} + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders point with metric label', () => { + const location = { + ...mockLocation, + properties: { + metric: 42.5, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('renders point with null radius (uses default)', () => { + const location = { + ...mockLocation, + properties: { + radius: null, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('handles kilometers pointRadiusUnit', () => { + const location = { + ...mockLocation, + properties: { + radius: 10, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + pointRadiusUnit="Kilometers" + zoom={10} + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('handles miles pointRadiusUnit', () => { + const location = { + ...mockLocation, + properties: { + radius: 10, + }, + }; + + const { container } = render( + <ScatterPlotGlowOverlay + locations={[location]} + rgb={[255, 0, 0, 255]} + pointRadiusUnit="Miles" + zoom={10} + />, + ); + + expect(container.querySelector('canvas')).toBeInTheDocument(); +}); + +test('uses custom lngLatAccessor', () => { + const customAccessor = () => [50, 60] as [number, number]; + const { container } = render( + <ScatterPlotGlowOverlay + locations={[mockLocation]} + rgb={[255, 0, 0, 255]} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Insufficient Test Coverage</b></div> <div id="fix"> The test suite only verifies that a canvas element is rendered but does not test the core drawing functionality of the ScatterPlotGlowOverlay component. This means bugs in coordinate projection, clustering logic, or canvas rendering could go undetected. Consider adding assertions that verify the canvas context is manipulated correctly or using canvas snapshots for visual regression testing. </div> </div> <small><i>Code Review Run #fabf38</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]
