Copilot commented on code in PR #36323: URL: https://github.com/apache/superset/pull/36323#discussion_r2578247803
########## superset-frontend/plugins/legacy-plugin-chart-country-map/test/CountryMap.test.tsx: ########## @@ -0,0 +1,130 @@ +/** + * 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 '@testing-library/jest-dom'; +import { render, fireEvent } from '@testing-library/react'; +import d3 from 'd3'; +import ReactCountryMap from '../src/ReactCountryMap'; + +jest.spyOn(d3, 'json'); + +type Projection = ((...args: unknown[]) => void) & { + scale: () => Projection; + center: () => Projection; + translate: () => Projection; +}; + +type PathFn = (() => string) & { + projection: jest.Mock; + bounds: jest.Mock<[[number, number], [number, number]]>; + centroid: jest.Mock<[number, number]>; +}; + +const mockPath: PathFn = jest.fn(() => 'M10 10 L20 20') as unknown as PathFn; +mockPath.projection = jest.fn(); +mockPath.bounds = jest.fn(() => [ + [0, 0], + [100, 100], +]); +mockPath.centroid = jest.fn(() => [50, 50]); + +jest.spyOn(d3.geo, 'path').mockImplementation(() => mockPath); + +// Mock d3.geo.mercator +jest.spyOn(d3.geo, 'mercator').mockImplementation(() => { + const proj = (() => {}) as Projection; + proj.scale = () => proj; + proj.center = () => proj; + proj.translate = () => proj; + return proj; +}); + +// Mock d3.mouse +jest.spyOn(d3, 'mouse').mockReturnValue([100, 50]); + +const mockMapData = { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + properties: { ISO: 'CAN', NAME_1: 'Canada' }, + geometry: {}, + }, + ], +}; + +type D3JsonCallback = (error: Error | null, data: unknown) => void; + +describe('CountryMap (legacy d3)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('renders a map after d3.json loads data', async () => { + d3.json.mockImplementation((_url: string, cb: D3JsonCallback) => + cb(null, mockMapData), + ); + + render( + <ReactCountryMap + width={500} + height={300} + data={[{ country_id: 'CAN', metric: 100 }]} + country="canada" + linearColorScheme="bnbColors" + colorScheme="" + numberFormat=".2f" + />, + ); + + expect(d3.json).toHaveBeenCalledTimes(1); + + const region = await document.querySelector('path.region'); + expect(region).not.toBeNull(); + }); + + it('shows tooltip on mouseenter/mousemove/mouseout', async () => { + d3.json.mockImplementation((_url: string, cb: D3JsonCallback) => + cb(null, mockMapData), + ); + + render( + <ReactCountryMap + width={500} + height={300} + data={[{ country_id: 'CAN', metric: 100 }]} + country="canada" + linearColorScheme="bnbColors" + colorScheme="" + />, + ); + + const region = await document.querySelector('path.region'); + expect(region).not.toBeNull(); + + const popup = document.querySelector('.hover-popup'); + expect(popup).not.toBeNull(); + + fireEvent.mouseEnter(region!); Review Comment: [nitpick] The test verifies the tooltip displays on `mouseEnter` and hides on `mouseOut`, but uses inconsistent casing. The actual event handlers are bound to `mouseenter` and `mouseout` (lowercase) in CountryMap.js line 209-211. While React's `fireEvent` API normalizes these, it's better practice to use the standard DOM event names for clarity: `fireEvent.mouseEnter` should be `fireEvent.mouseOver` or match the lowercase convention used in the implementation. ```suggestion fireEvent.mouseOver(region!); ``` ########## superset-frontend/package-lock.json: ########## @@ -6407,7 +6407,6 @@ "version": "29.7.0", "resolved": "https://registry.npmjs.org/@jest/expect-utils/-/expect-utils-29.7.0.tgz";, "integrity": "sha512-GlsNBWiFQFCVi9QVSx7f5AgMeLxe9YCCs5PuP2O2LdjDAA8Jh9eX7lA1Jq/xdXw3Wb3hyvlFNfZIfcRetSzYcA==", - "dev": true, "license": "MIT", Review Comment: The removal of `"dev": true` markers from Jest and testing-related packages (like `@jest/expect-utils`, `@jest/schemas`, `@jest/types`, etc.) in package-lock.json is a side effect of incorrectly adding testing libraries to `peerDependencies` in the plugin's package.json. These packages should remain as dev dependencies. Once the test dependencies are removed from `peerDependencies` (as noted in another comment), regenerate package-lock.json to restore the proper `"dev": true` markers. ########## superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js: ########## @@ -176,17 +141,34 @@ function CountryMap(element, props) { c = d3.rgb(c).darker().toString(); } d3.select(this).style('fill', c); - selectAndDisplayNameOfRegion(d); + // Display information popup const result = data.filter( region => region.country_id === d.properties.ISO, ); - updateMetrics(result); + + const position = d3.mouse(svg.node()); + hoverPopup + .style('display', 'block') + .style('top', `${position[1] + 30}px`) + .style('left', `${position[0]}px`) + .html( + `<div> + <strong>${getNameOfRegion(d)}</strong><br> + ${result.length > 0 ? format(result[0].metric) : ''} + </div>`, Review Comment: [nitpick] The HTML template string contains unnecessary whitespace and line breaks that will be rendered in the tooltip. Consider using a template without the formatting whitespace: ```javascript .html( `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ? format(result[0].metric) : ''}</div>`, ); ``` ```suggestion `<div><strong>${getNameOfRegion(d)}</strong><br>${result.length > 0 ? format(result[0].metric) : ''}</div>`, ``` ########## superset-frontend/plugins/legacy-plugin-chart-country-map/test/CountryMap.test.tsx: ########## @@ -0,0 +1,130 @@ +/** + * 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 '@testing-library/jest-dom'; +import { render, fireEvent } from '@testing-library/react'; +import d3 from 'd3'; +import ReactCountryMap from '../src/ReactCountryMap'; + +jest.spyOn(d3, 'json'); + +type Projection = ((...args: unknown[]) => void) & { + scale: () => Projection; + center: () => Projection; + translate: () => Projection; +}; + +type PathFn = (() => string) & { + projection: jest.Mock; + bounds: jest.Mock<[[number, number], [number, number]]>; + centroid: jest.Mock<[number, number]>; +}; + +const mockPath: PathFn = jest.fn(() => 'M10 10 L20 20') as unknown as PathFn; +mockPath.projection = jest.fn(); +mockPath.bounds = jest.fn(() => [ + [0, 0], + [100, 100], +]); +mockPath.centroid = jest.fn(() => [50, 50]); + +jest.spyOn(d3.geo, 'path').mockImplementation(() => mockPath); + +// Mock d3.geo.mercator +jest.spyOn(d3.geo, 'mercator').mockImplementation(() => { + const proj = (() => {}) as Projection; + proj.scale = () => proj; + proj.center = () => proj; + proj.translate = () => proj; + return proj; +}); + +// Mock d3.mouse +jest.spyOn(d3, 'mouse').mockReturnValue([100, 50]); + +const mockMapData = { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + properties: { ISO: 'CAN', NAME_1: 'Canada' }, + geometry: {}, + }, + ], +}; + +type D3JsonCallback = (error: Error | null, data: unknown) => void; + +describe('CountryMap (legacy d3)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('renders a map after d3.json loads data', async () => { + d3.json.mockImplementation((_url: string, cb: D3JsonCallback) => + cb(null, mockMapData), + ); + + render( + <ReactCountryMap + width={500} + height={300} + data={[{ country_id: 'CAN', metric: 100 }]} + country="canada" + linearColorScheme="bnbColors" + colorScheme="" + numberFormat=".2f" + />, + ); + + expect(d3.json).toHaveBeenCalledTimes(1); + + const region = await document.querySelector('path.region'); + expect(region).not.toBeNull(); + }); + + it('shows tooltip on mouseenter/mousemove/mouseout', async () => { + d3.json.mockImplementation((_url: string, cb: D3JsonCallback) => + cb(null, mockMapData), + ); + + render( + <ReactCountryMap + width={500} + height={300} + data={[{ country_id: 'CAN', metric: 100 }]} + country="canada" + linearColorScheme="bnbColors" + colorScheme="" + />, + ); + + const region = await document.querySelector('path.region'); Review Comment: The `await` keyword is used with `document.querySelector()` which returns a value synchronously, not a Promise. This is incorrect usage and the `await` keyword should be removed. ########## superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js: ########## @@ -176,17 +141,34 @@ function CountryMap(element, props) { c = d3.rgb(c).darker().toString(); } d3.select(this).style('fill', c); - selectAndDisplayNameOfRegion(d); + // Display information popup const result = data.filter( region => region.country_id === d.properties.ISO, ); - updateMetrics(result); + + const position = d3.mouse(svg.node()); + hoverPopup + .style('display', 'block') + .style('top', `${position[1] + 30}px`) + .style('left', `${position[0]}px`) + .html( + `<div> + <strong>${getNameOfRegion(d)}</strong><br> + ${result.length > 0 ? format(result[0].metric) : ''} Review Comment: [nitpick] When `result` is empty or `result[0].metric` is `0`, the tooltip will display an empty line after the region name. Consider providing a fallback message like "No data" or conditionally hiding the `<br>` tag when there's no metric to display: ```javascript .html( `<div> <strong>${getNameOfRegion(d)}</strong>${result.length > 0 ? `<br>${format(result[0].metric)}` : ''} </div>`, ); ``` ```suggestion <strong>${getNameOfRegion(d)}</strong>${ result.length > 0 && result[0].metric != null ? `<br>${format(result[0].metric)}` : `<br><em>No data</em>` } ``` ########## superset-frontend/plugins/legacy-plugin-chart-country-map/test/CountryMap.test.tsx: ########## @@ -0,0 +1,130 @@ +/** + * 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 '@testing-library/jest-dom'; +import { render, fireEvent } from '@testing-library/react'; +import d3 from 'd3'; +import ReactCountryMap from '../src/ReactCountryMap'; + +jest.spyOn(d3, 'json'); + +type Projection = ((...args: unknown[]) => void) & { + scale: () => Projection; + center: () => Projection; + translate: () => Projection; +}; + +type PathFn = (() => string) & { + projection: jest.Mock; + bounds: jest.Mock<[[number, number], [number, number]]>; + centroid: jest.Mock<[number, number]>; +}; + +const mockPath: PathFn = jest.fn(() => 'M10 10 L20 20') as unknown as PathFn; +mockPath.projection = jest.fn(); +mockPath.bounds = jest.fn(() => [ + [0, 0], + [100, 100], +]); +mockPath.centroid = jest.fn(() => [50, 50]); + +jest.spyOn(d3.geo, 'path').mockImplementation(() => mockPath); + +// Mock d3.geo.mercator +jest.spyOn(d3.geo, 'mercator').mockImplementation(() => { + const proj = (() => {}) as Projection; + proj.scale = () => proj; + proj.center = () => proj; + proj.translate = () => proj; + return proj; +}); + +// Mock d3.mouse +jest.spyOn(d3, 'mouse').mockReturnValue([100, 50]); + +const mockMapData = { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + properties: { ISO: 'CAN', NAME_1: 'Canada' }, + geometry: {}, + }, + ], +}; + +type D3JsonCallback = (error: Error | null, data: unknown) => void; + +describe('CountryMap (legacy d3)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('renders a map after d3.json loads data', async () => { + d3.json.mockImplementation((_url: string, cb: D3JsonCallback) => + cb(null, mockMapData), + ); + + render( + <ReactCountryMap + width={500} + height={300} + data={[{ country_id: 'CAN', metric: 100 }]} + country="canada" + linearColorScheme="bnbColors" + colorScheme="" + numberFormat=".2f" + />, + ); + + expect(d3.json).toHaveBeenCalledTimes(1); + + const region = await document.querySelector('path.region'); Review Comment: The `await` keyword is used with `document.querySelector()` which returns a value synchronously, not a Promise. This is incorrect usage and the `await` keyword should be removed. The same issue exists on line 118. ########## superset-frontend/plugins/legacy-plugin-chart-country-map/package.json: ########## @@ -31,9 +31,12 @@ "prop-types": "^15.8.1" }, "peerDependencies": { + "@apache-superset/core": "*", "@superset-ui/chart-controls": "*", "@superset-ui/core": "*", - "@apache-superset/core": "*", - "react": "^17.0.2" + "react": "^17.0.2", + "@testing-library/jest-dom": "*", + "@types/jest": "*", + "@testing-library/react": "^12.1.5" Review Comment: Testing dependencies (`@testing-library/jest-dom`, `@types/jest`, `@testing-library/react`) should not be added to `peerDependencies`. These are development dependencies that should only be declared at the workspace/root level, not as peer dependencies of a publishable package. Peer dependencies are runtime dependencies that the consuming application is expected to provide. Other plugins in this repository (e.g., `plugin-chart-echarts`, `legacy-plugin-chart-world-map`) don't include test dependencies in their `peerDependencies`, following the standard convention. Remove these test-related entries from `peerDependencies`. ```suggestion "react": "^17.0.2" ``` ########## superset-frontend/plugins/legacy-plugin-chart-country-map/test/CountryMap.test.tsx: ########## @@ -0,0 +1,130 @@ +/** + * 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 '@testing-library/jest-dom'; +import { render, fireEvent } from '@testing-library/react'; +import d3 from 'd3'; +import ReactCountryMap from '../src/ReactCountryMap'; + +jest.spyOn(d3, 'json'); + +type Projection = ((...args: unknown[]) => void) & { + scale: () => Projection; + center: () => Projection; + translate: () => Projection; +}; + +type PathFn = (() => string) & { + projection: jest.Mock; + bounds: jest.Mock<[[number, number], [number, number]]>; + centroid: jest.Mock<[number, number]>; +}; + +const mockPath: PathFn = jest.fn(() => 'M10 10 L20 20') as unknown as PathFn; +mockPath.projection = jest.fn(); +mockPath.bounds = jest.fn(() => [ + [0, 0], + [100, 100], +]); +mockPath.centroid = jest.fn(() => [50, 50]); + +jest.spyOn(d3.geo, 'path').mockImplementation(() => mockPath); + +// Mock d3.geo.mercator +jest.spyOn(d3.geo, 'mercator').mockImplementation(() => { + const proj = (() => {}) as Projection; + proj.scale = () => proj; + proj.center = () => proj; + proj.translate = () => proj; + return proj; +}); + +// Mock d3.mouse +jest.spyOn(d3, 'mouse').mockReturnValue([100, 50]); + +const mockMapData = { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + properties: { ISO: 'CAN', NAME_1: 'Canada' }, + geometry: {}, + }, + ], +}; + +type D3JsonCallback = (error: Error | null, data: unknown) => void; + +describe('CountryMap (legacy d3)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('renders a map after d3.json loads data', async () => { + d3.json.mockImplementation((_url: string, cb: D3JsonCallback) => + cb(null, mockMapData), + ); + + render( + <ReactCountryMap + width={500} + height={300} + data={[{ country_id: 'CAN', metric: 100 }]} + country="canada" + linearColorScheme="bnbColors" + colorScheme="" + numberFormat=".2f" + />, + ); + + expect(d3.json).toHaveBeenCalledTimes(1); + + const region = await document.querySelector('path.region'); + expect(region).not.toBeNull(); + }); + + it('shows tooltip on mouseenter/mousemove/mouseout', async () => { + d3.json.mockImplementation((_url: string, cb: D3JsonCallback) => + cb(null, mockMapData), + ); + + render( + <ReactCountryMap + width={500} + height={300} + data={[{ country_id: 'CAN', metric: 100 }]} + country="canada" + linearColorScheme="bnbColors" + colorScheme="" + />, + ); + + const region = await document.querySelector('path.region'); + expect(region).not.toBeNull(); + + const popup = document.querySelector('.hover-popup'); + expect(popup).not.toBeNull(); + + fireEvent.mouseEnter(region!); + expect(popup!).toHaveStyle({ display: 'block' }); + + fireEvent.mouseOut(region!); + expect(popup!).toHaveStyle({ display: 'none' }); + }); Review Comment: The test doesn't verify that `mousemove` events update the tooltip position. The test only checks `mouseenter` and `mouseout`, but the code change added a new `mousemove` handler (line 210 in CountryMap.js) that should also be tested to ensure the tooltip follows the cursor as intended. -- 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]
