codeant-ai-for-open-source[bot] commented on code in PR #37716: URL: https://github.com/apache/superset/pull/37716#discussion_r2775079384
########## superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts: ########## @@ -0,0 +1,329 @@ +/** + * 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 d3 from 'd3'; +import WorldMap from '../src/WorldMap'; + +type MouseEventHandler = (this: HTMLElement) => void; + +interface MockD3Selection { + attr: jest.Mock; + style: jest.Mock; + classed: jest.Mock; + selectAll: jest.Mock; +} + +// Mock Datamap +const mockBubbles = jest.fn(); +const mockUpdateChoropleth = jest.fn(); +const mockSvg = { + selectAll: jest.fn().mockReturnThis(), + on: jest.fn().mockReturnThis(), + attr: jest.fn().mockReturnThis(), + style: jest.fn().mockReturnThis(), +}; + +jest.mock('datamaps/dist/datamaps.all.min', () => { + return jest.fn().mockImplementation(config => { + // Call the done callback immediately to simulate Datamap initialization + if (config.done) { + config.done({ + svg: mockSvg, + }); + } + return { + bubbles: mockBubbles, + updateChoropleth: mockUpdateChoropleth, + svg: mockSvg, + }; + }); +}); + +let container: HTMLElement; +const mockFormatter = jest.fn(val => String(val)); + +const baseProps = { + data: [ + { country: 'USA', name: 'United States', m1: 100, m2: 200, code: 'US' }, + { country: 'CAN', name: 'Canada', m1: 50, m2: 100, code: 'CA' }, + ], + width: 600, + height: 400, + maxBubbleSize: 25, + showBubbles: false, + linearColorScheme: 'schemeRdYlBu', + color: '#61B0B7', + colorBy: 'country', + colorScheme: 'supersetColors', + sliceId: 123, + theme: { + colorBorder: '#e0e0e0', + colorSplit: '#333', + colorIcon: '#000', + colorTextSecondary: '#666', + }, + countryFieldtype: 'code', + entity: 'country', + onContextMenu: jest.fn(), + setDataMask: jest.fn(), + inContextMenu: false, + filterState: { selectedValues: [] }, + emitCrossFilters: false, + formatter: mockFormatter, +}; + +beforeEach(() => { + jest.clearAllMocks(); + container = document.createElement('div'); + document.body.appendChild(container); +}); + +afterEach(() => { + document.body.removeChild(container); +}); + +test('sets up mouseover and mouseout handlers on countries', () => { + WorldMap(container, baseProps); + + expect(mockSvg.selectAll).toHaveBeenCalledWith('.datamaps-subunit'); + const onCalls = mockSvg.on.mock.calls; + + // Find mouseover and mouseout handler registrations + const hasMouseover = onCalls.some(call => call[0] === 'mouseover'); + const hasMouseout = onCalls.some(call => call[0] === 'mouseout'); + + expect(hasMouseover).toBe(true); + expect(hasMouseout).toBe(true); +}); + +test('stores original fill color on mouseover', () => { + // Create a mock DOM element with d3 selection capabilities + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit USA'); + mockElement.style.fill = 'rgb(100, 150, 200)'; + container.appendChild(mockElement); + + let mouseoverHandler: MouseEventHandler | null = null; + + // Mock d3.select to return the mock element + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string) => { + if (value !== undefined) { + mockElement.setAttribute(attrName, value); + } else { + return mockElement.getAttribute(attrName); + } + return mockD3Selection; + }), + style: jest.fn((styleName: string, value?: string) => { + if (value !== undefined) { + mockElement.style[styleName as any] = value; + } else { + return mockElement.style[styleName as any]; + } + return mockD3Selection; + }), + classed: jest.fn().mockReturnThis(), + selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }), + }; + + jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); + + // Capture the mouseover handler + mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) => { + if (event === 'mouseover') { + mouseoverHandler = handler; + } + return mockSvg; + }); + + WorldMap(container, baseProps); + + // Simulate mouseover + if (mouseoverHandler) { + (mouseoverHandler as MouseEventHandler).call(mockElement); + } + + // Verify that data-original-fill attribute was set + expect(mockD3Selection.attr).toHaveBeenCalledWith( + 'data-original-fill', + expect.any(String), + ); +}); + +test('restores original fill color on mouseout for country with data', () => { + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit USA'); + mockElement.style.fill = 'rgb(100, 150, 200)'; + mockElement.setAttribute('data-original-fill', 'rgb(100, 150, 200)'); + container.appendChild(mockElement); + + let mouseoutHandler: MouseEventHandler | null = null; + + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string | null) => { + if (value !== undefined) { + if (value === null) { + mockElement.removeAttribute(attrName); + } else { + mockElement.setAttribute(attrName, value); + } + } + return mockElement.getAttribute(attrName) || mockD3Selection; Review Comment: **Suggestion:** In the "restores original fill color on mouseout for country with data" test, the mocked `attr` function returns the attribute string when setting a non-null value instead of the selection, breaking D3's chainable API and potentially causing runtime errors if the production code chains `.attr(...).style(...)` on the selection. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ WorldMap unit tests may throw TypeError on chained attr->style. - ⚠️ Test misrepresents d3 selection chaining semantics. - ⚠️ Can cause false negatives in hover/mouseout tests. ``` </details> ```suggestion // When used as a setter, mimic D3 by returning the selection for chaining return mockD3Selection; } // Getter: return the attribute value return mockElement.getAttribute(attrName); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the test file superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts and locate the test "restores original fill color on mouseout for country with data" (starts around line 170 in the file). The test defines a mock selection object including the `attr` mock at lines 180-189. 2. Note the test stubs d3.select to return this mock selection at the line: jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); (line ~200 in the test). The test then installs the mouseout handler capture via mockSvg.on.mockImplementation at lines ~203-208. 3. The test simulates the mouseout handler by calling (mouseoutHandler as MouseEventHandler).call(mockElement); (line ~214-215). Production WorldMap code (imported at ../src/WorldMap) will execute the mouseout handler using the mocked selection returned by d3.select. 4. Because the current mocked `attr` implementation (lines 180-189) returns mockElement.getAttribute(...) (a string) when used as a setter (it uses the getter return fallback), if the mouseout handler in WorldMap performs D3-style chaining like d3.select(this).attr(...).style(...), the .attr(...) call will return a string instead of the mock selection. This would cause the subsequent chained .style(...) call to throw at runtime and the test to fail (TypeError) or prevent mockD3Selection.style from being called. Replacing the mock `attr` setter to return mockD3Selection (as in the improved code) preserves chaining and matches D3 behavior. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts **Line:** 187:188 **Comment:** *Possible Bug: In the "restores original fill color on mouseout for country with data" test, the mocked `attr` function returns the attribute string when setting a non-null value instead of the selection, breaking D3's chainable API and potentially causing runtime errors if the production code chains `.attr(...).style(...)` on the selection. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts: ########## @@ -0,0 +1,329 @@ +/** + * 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 d3 from 'd3'; +import WorldMap from '../src/WorldMap'; + +type MouseEventHandler = (this: HTMLElement) => void; + +interface MockD3Selection { + attr: jest.Mock; + style: jest.Mock; + classed: jest.Mock; + selectAll: jest.Mock; +} + +// Mock Datamap +const mockBubbles = jest.fn(); +const mockUpdateChoropleth = jest.fn(); +const mockSvg = { + selectAll: jest.fn().mockReturnThis(), + on: jest.fn().mockReturnThis(), + attr: jest.fn().mockReturnThis(), + style: jest.fn().mockReturnThis(), +}; + +jest.mock('datamaps/dist/datamaps.all.min', () => { + return jest.fn().mockImplementation(config => { + // Call the done callback immediately to simulate Datamap initialization + if (config.done) { + config.done({ + svg: mockSvg, + }); + } + return { + bubbles: mockBubbles, + updateChoropleth: mockUpdateChoropleth, + svg: mockSvg, + }; + }); +}); + +let container: HTMLElement; +const mockFormatter = jest.fn(val => String(val)); + +const baseProps = { + data: [ + { country: 'USA', name: 'United States', m1: 100, m2: 200, code: 'US' }, + { country: 'CAN', name: 'Canada', m1: 50, m2: 100, code: 'CA' }, + ], + width: 600, + height: 400, + maxBubbleSize: 25, + showBubbles: false, + linearColorScheme: 'schemeRdYlBu', + color: '#61B0B7', + colorBy: 'country', + colorScheme: 'supersetColors', + sliceId: 123, + theme: { + colorBorder: '#e0e0e0', + colorSplit: '#333', + colorIcon: '#000', + colorTextSecondary: '#666', + }, + countryFieldtype: 'code', + entity: 'country', + onContextMenu: jest.fn(), + setDataMask: jest.fn(), + inContextMenu: false, + filterState: { selectedValues: [] }, + emitCrossFilters: false, + formatter: mockFormatter, +}; + +beforeEach(() => { + jest.clearAllMocks(); + container = document.createElement('div'); + document.body.appendChild(container); +}); + +afterEach(() => { + document.body.removeChild(container); +}); + +test('sets up mouseover and mouseout handlers on countries', () => { + WorldMap(container, baseProps); + + expect(mockSvg.selectAll).toHaveBeenCalledWith('.datamaps-subunit'); + const onCalls = mockSvg.on.mock.calls; + + // Find mouseover and mouseout handler registrations + const hasMouseover = onCalls.some(call => call[0] === 'mouseover'); + const hasMouseout = onCalls.some(call => call[0] === 'mouseout'); + + expect(hasMouseover).toBe(true); + expect(hasMouseout).toBe(true); +}); + +test('stores original fill color on mouseover', () => { + // Create a mock DOM element with d3 selection capabilities + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit USA'); + mockElement.style.fill = 'rgb(100, 150, 200)'; + container.appendChild(mockElement); + + let mouseoverHandler: MouseEventHandler | null = null; + + // Mock d3.select to return the mock element + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string) => { + if (value !== undefined) { + mockElement.setAttribute(attrName, value); + } else { + return mockElement.getAttribute(attrName); + } + return mockD3Selection; + }), + style: jest.fn((styleName: string, value?: string) => { + if (value !== undefined) { + mockElement.style[styleName as any] = value; + } else { + return mockElement.style[styleName as any]; + } + return mockD3Selection; + }), + classed: jest.fn().mockReturnThis(), + selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }), + }; + + jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); + + // Capture the mouseover handler + mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) => { + if (event === 'mouseover') { + mouseoverHandler = handler; + } + return mockSvg; + }); + + WorldMap(container, baseProps); + + // Simulate mouseover + if (mouseoverHandler) { + (mouseoverHandler as MouseEventHandler).call(mockElement); + } + + // Verify that data-original-fill attribute was set + expect(mockD3Selection.attr).toHaveBeenCalledWith( + 'data-original-fill', + expect.any(String), + ); +}); + +test('restores original fill color on mouseout for country with data', () => { + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit USA'); + mockElement.style.fill = 'rgb(100, 150, 200)'; + mockElement.setAttribute('data-original-fill', 'rgb(100, 150, 200)'); + container.appendChild(mockElement); + + let mouseoutHandler: MouseEventHandler | null = null; + + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string | null) => { + if (value !== undefined) { + if (value === null) { + mockElement.removeAttribute(attrName); + } else { + mockElement.setAttribute(attrName, value); + } + } + return mockElement.getAttribute(attrName) || mockD3Selection; + }), + style: jest.fn((styleName: string, value?: string) => { + if (value !== undefined) { + mockElement.style[styleName as any] = value; + } + return mockElement.style[styleName as any] || mockD3Selection; + }), + classed: jest.fn().mockReturnThis(), + selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }), + }; + + jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); + + // Capture the mouseout handler + mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) => { + if (event === 'mouseout') { + mouseoutHandler = handler; + } + return mockSvg; + }); + + WorldMap(container, baseProps); + + // Simulate mouseout + if (mouseoutHandler) { + (mouseoutHandler as MouseEventHandler).call(mockElement); + } + + // Verify that original fill was restored + expect(mockD3Selection.style).toHaveBeenCalledWith( + 'fill', + 'rgb(100, 150, 200)', + ); + expect(mockD3Selection.attr).toHaveBeenCalledWith('data-original-fill', null); +}); + +test('restores default fill color on mouseout for country with no data', () => { + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit XXX'); + mockElement.style.fill = '#e0e0e0'; // Default border color + mockElement.setAttribute('data-original-fill', '#e0e0e0'); + container.appendChild(mockElement); + + let mouseoutHandler: MouseEventHandler | null = null; + + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string | null) => { + if (value !== undefined) { + if (value === null) { + mockElement.removeAttribute(attrName); + } else { + mockElement.setAttribute(attrName, value); + } + } + return mockElement.getAttribute(attrName) || mockD3Selection; Review Comment: **Suggestion:** In the "restores default fill color on mouseout for country with no data" test, the mocked `attr` implementation has the same issue of returning the attribute string on set instead of the selection, which can break D3-style chaining and make the test diverge from real behavior. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ No-data mouseout unit test can throw TypeError. - ⚠️ Test may miss style restoration behavior. - ⚠️ Reduces confidence in hover state fixes. ``` </details> ```suggestion // When setting an attribute, return the selection to allow chaining return mockD3Selection; } // Getter: return the attribute value return mockElement.getAttribute(attrName); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts and find the test "restores default fill color on mouseout for country with no data" (starts around line 225). The test defines a mock selection with `attr` at lines 235-244. 2. The test stubs d3.select to return this mock selection at the line: jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); (immediately before the mouseout handler capture around line ~255). 3. The test captures the mouseout handler via mockSvg.on.mockImplementation((event, handler) => { if (event === 'mouseout') mouseoutHandler = handler; }); (around lines ~257-262) and later simulates it with (mouseoutHandler as MouseEventHandler).call(mockElement); (around line ~268). 4. If the production mouseout handler uses chaining (d3.select(this).attr(...).style(...)), the current mock's attr returns a string (via mockElement.getAttribute(...) fallback) instead of the selection when used as a setter, breaking chaining and causing a runtime error or preventing style calls. Updating the mock to return the selection when used as a setter (improved code) recreates D3 semantics and avoids such failures. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts **Line:** 242:243 **Comment:** *Possible Bug: In the "restores default fill color on mouseout for country with no data" test, the mocked `attr` implementation has the same issue of returning the attribute string on set instead of the selection, which can break D3-style chaining and make the test diverge from real behavior. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts: ########## @@ -0,0 +1,329 @@ +/** + * 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 d3 from 'd3'; +import WorldMap from '../src/WorldMap'; + +type MouseEventHandler = (this: HTMLElement) => void; + +interface MockD3Selection { + attr: jest.Mock; + style: jest.Mock; + classed: jest.Mock; + selectAll: jest.Mock; +} + +// Mock Datamap +const mockBubbles = jest.fn(); +const mockUpdateChoropleth = jest.fn(); +const mockSvg = { + selectAll: jest.fn().mockReturnThis(), + on: jest.fn().mockReturnThis(), + attr: jest.fn().mockReturnThis(), + style: jest.fn().mockReturnThis(), +}; + +jest.mock('datamaps/dist/datamaps.all.min', () => { + return jest.fn().mockImplementation(config => { + // Call the done callback immediately to simulate Datamap initialization + if (config.done) { + config.done({ + svg: mockSvg, + }); + } + return { + bubbles: mockBubbles, + updateChoropleth: mockUpdateChoropleth, + svg: mockSvg, + }; + }); +}); + +let container: HTMLElement; +const mockFormatter = jest.fn(val => String(val)); + +const baseProps = { + data: [ + { country: 'USA', name: 'United States', m1: 100, m2: 200, code: 'US' }, + { country: 'CAN', name: 'Canada', m1: 50, m2: 100, code: 'CA' }, + ], + width: 600, + height: 400, + maxBubbleSize: 25, + showBubbles: false, + linearColorScheme: 'schemeRdYlBu', + color: '#61B0B7', + colorBy: 'country', + colorScheme: 'supersetColors', + sliceId: 123, + theme: { + colorBorder: '#e0e0e0', + colorSplit: '#333', + colorIcon: '#000', + colorTextSecondary: '#666', + }, + countryFieldtype: 'code', + entity: 'country', + onContextMenu: jest.fn(), + setDataMask: jest.fn(), + inContextMenu: false, + filterState: { selectedValues: [] }, + emitCrossFilters: false, + formatter: mockFormatter, +}; + +beforeEach(() => { + jest.clearAllMocks(); + container = document.createElement('div'); + document.body.appendChild(container); +}); + +afterEach(() => { + document.body.removeChild(container); +}); + +test('sets up mouseover and mouseout handlers on countries', () => { + WorldMap(container, baseProps); + + expect(mockSvg.selectAll).toHaveBeenCalledWith('.datamaps-subunit'); + const onCalls = mockSvg.on.mock.calls; + + // Find mouseover and mouseout handler registrations + const hasMouseover = onCalls.some(call => call[0] === 'mouseover'); + const hasMouseout = onCalls.some(call => call[0] === 'mouseout'); + + expect(hasMouseover).toBe(true); + expect(hasMouseout).toBe(true); +}); + +test('stores original fill color on mouseover', () => { + // Create a mock DOM element with d3 selection capabilities + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit USA'); + mockElement.style.fill = 'rgb(100, 150, 200)'; + container.appendChild(mockElement); + + let mouseoverHandler: MouseEventHandler | null = null; + + // Mock d3.select to return the mock element + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string) => { + if (value !== undefined) { + mockElement.setAttribute(attrName, value); + } else { + return mockElement.getAttribute(attrName); + } + return mockD3Selection; + }), + style: jest.fn((styleName: string, value?: string) => { + if (value !== undefined) { + mockElement.style[styleName as any] = value; + } else { + return mockElement.style[styleName as any]; + } + return mockD3Selection; + }), + classed: jest.fn().mockReturnThis(), + selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }), + }; + + jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); + + // Capture the mouseover handler + mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) => { + if (event === 'mouseover') { + mouseoverHandler = handler; + } + return mockSvg; + }); + + WorldMap(container, baseProps); + + // Simulate mouseover + if (mouseoverHandler) { + (mouseoverHandler as MouseEventHandler).call(mockElement); + } + + // Verify that data-original-fill attribute was set + expect(mockD3Selection.attr).toHaveBeenCalledWith( + 'data-original-fill', + expect.any(String), + ); +}); + +test('restores original fill color on mouseout for country with data', () => { + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit USA'); + mockElement.style.fill = 'rgb(100, 150, 200)'; + mockElement.setAttribute('data-original-fill', 'rgb(100, 150, 200)'); + container.appendChild(mockElement); + + let mouseoutHandler: MouseEventHandler | null = null; + + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string | null) => { + if (value !== undefined) { + if (value === null) { + mockElement.removeAttribute(attrName); + } else { + mockElement.setAttribute(attrName, value); + } + } + return mockElement.getAttribute(attrName) || mockD3Selection; + }), + style: jest.fn((styleName: string, value?: string) => { + if (value !== undefined) { + mockElement.style[styleName as any] = value; + } + return mockElement.style[styleName as any] || mockD3Selection; + }), + classed: jest.fn().mockReturnThis(), + selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }), + }; + + jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); + + // Capture the mouseout handler + mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) => { + if (event === 'mouseout') { + mouseoutHandler = handler; + } + return mockSvg; + }); + + WorldMap(container, baseProps); + + // Simulate mouseout + if (mouseoutHandler) { + (mouseoutHandler as MouseEventHandler).call(mockElement); + } + + // Verify that original fill was restored + expect(mockD3Selection.style).toHaveBeenCalledWith( + 'fill', + 'rgb(100, 150, 200)', + ); + expect(mockD3Selection.attr).toHaveBeenCalledWith('data-original-fill', null); +}); + +test('restores default fill color on mouseout for country with no data', () => { + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit XXX'); + mockElement.style.fill = '#e0e0e0'; // Default border color + mockElement.setAttribute('data-original-fill', '#e0e0e0'); + container.appendChild(mockElement); + + let mouseoutHandler: MouseEventHandler | null = null; + + const mockD3Selection: MockD3Selection = { + attr: jest.fn((attrName: string, value?: string | null) => { + if (value !== undefined) { + if (value === null) { + mockElement.removeAttribute(attrName); + } else { + mockElement.setAttribute(attrName, value); + } + } + return mockElement.getAttribute(attrName) || mockD3Selection; + }), + style: jest.fn((styleName: string, value?: string) => { + if (value !== undefined) { + mockElement.style[styleName as any] = value; + } + return mockElement.style[styleName as any] || mockD3Selection; + }), + classed: jest.fn().mockReturnThis(), + selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }), + }; + + jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); + + mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) => { + if (event === 'mouseout') { + mouseoutHandler = handler; + } + return mockSvg; + }); + + WorldMap(container, baseProps); + + // Simulate mouseout + if (mouseoutHandler) { + (mouseoutHandler as MouseEventHandler).call(mockElement); + } + + // Verify that default fill was restored (no-data color) + expect(mockD3Selection.style).toHaveBeenCalledWith('fill', '#e0e0e0'); + expect(mockD3Selection.attr).toHaveBeenCalledWith('data-original-fill', null); +}); + +test('does not handle mouse events when inContextMenu is true', () => { + const propsWithContextMenu = { + ...baseProps, + inContextMenu: true, + }; + + const mockElement = document.createElement('path'); + mockElement.setAttribute('class', 'datamaps-subunit USA'); + mockElement.style.fill = 'rgb(100, 150, 200)'; + container.appendChild(mockElement); + + let mouseoverHandler: MouseEventHandler | null = null; + let mouseoutHandler: MouseEventHandler | null = null; + + const mockD3Selection: MockD3Selection = { + attr: jest.fn(() => mockD3Selection), + style: jest.fn(() => mockD3Selection), + classed: jest.fn().mockReturnThis(), + selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }), + }; + + jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any); + + mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) => { + if (event === 'mouseover') { + mouseoverHandler = handler; + } + if (event === 'mouseout') { + mouseoutHandler = handler; + } + return mockSvg; + }); + + WorldMap(container, propsWithContextMenu); + + // Simulate mouseover and mouseout + if (mouseoverHandler) { + (mouseoverHandler as MouseEventHandler).call(mockElement); + } + if (mouseoutHandler) { + (mouseoutHandler as MouseEventHandler).call(mockElement); + } + + // When inContextMenu is true, handlers should exit early without modifying anything + // We verify this by checking that attr and style weren't called to change fill + const attrCalls = mockD3Selection.attr.mock.calls; + const fillChangeCalls = attrCalls.filter( + (call: [string, unknown]) => + call[0] === 'data-original-fill' && call[1] !== undefined, + ); + + // The handlers should return early, so no state changes + expect(fillChangeCalls.length).toBe(0); Review Comment: **Suggestion:** The "does not handle mouse events when inContextMenu is true" test only asserts that `data-original-fill` is not set, but never verifies that `style('fill', ...)` was not called, so the test can pass even if the chart still changes the visual fill color while in the context menu. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ inContextMenu test may miss visual fill changes. - ⚠️ Hover suppression bug could slip into releases. - ⚠️ Reduces test coverage for context-menu behavior. ``` </details> ```suggestion const fillAttrChangeCalls = attrCalls.filter( (call: [string, unknown]) => call[0] === 'data-original-fill' && call[1] !== undefined, ); const styleCalls = mockD3Selection.style.mock.calls; const fillStyleChangeCalls = styleCalls.filter( (call: [string, unknown]) => call[0] === 'fill' && call[1] !== undefined, ); // The handlers should return early, so no state changes expect(fillAttrChangeCalls.length).toBe(0); expect(fillStyleChangeCalls.length).toBe(0); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the test "does not handle mouse events when inContextMenu is true" in superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts. The test captures mouseover/mouseout handlers (mockSvg.on.mockImplementation around lines ~299-307) and simulates both handlers at lines ~312-317. 2. After simulation the test currently inspects only mockD3Selection.attr.mock.calls (line 321) and filters for data-original-fill changes (lines 322-325), asserting that none were called (line 328). 3. However, WorldMap could change a country's visual fill via selection.style('fill', ...) rather than or in addition to setting the data-original-fill attribute. The current test does not inspect mockD3Selection.style calls, so a change to the visual fill could occur and the test would still pass. 4. Reproduce by running jest for this test file. If the WorldMap mouse handlers modify style('fill', ...) while inContextMenu=true, the current test will not detect it. Adding the improved assertions (inspecting mockD3Selection.style.mock.calls for 'fill' changes as shown) will catch such regressions and ensure handlers truly do nothing. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts **Line:** 322:328 **Comment:** *Logic Error: The "does not handle mouse events when inContextMenu is true" test only asserts that `data-original-fill` is not set, but never verifies that `style('fill', ...)` was not called, so the test can pass even if the chart still changes the visual fill color while in the context menu. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- 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]
