codeant-ai-for-open-source[bot] commented on code in PR #38762:
URL: https://github.com/apache/superset/pull/38762#discussion_r2993801243
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts:
##########
@@ -157,244 +161,26 @@ afterEach(() => {
document.body.removeChild(container);
});
-test('sets up mouseover and mouseout handlers on countries', () => {
+test('sets up only contextmenu and click handlers on countries', () => {
WorldMap(container, baseProps);
expect(mockSvg.selectAll).toHaveBeenCalledWith('.datamaps-subunit');
const onCalls = mockSvg.on.mock.calls;
- // Find mouseover and mouseout handler registrations (namespaced events)
+ const hasContextMenu = onCalls.some(call => call[0] === 'contextmenu');
+ const hasClick = onCalls.some(call => call[0] === 'click');
+
+ expect(hasContextMenu).toBe(true);
+ expect(hasClick).toBe(true);
+
+ // Ensure removed handlers are NOT present
const hasMouseover = onCalls.some(
call => call[0] === 'mouseover.fillPreserve',
);
const hasMouseout = onCalls.some(call => call[0] ===
'mouseout.fillPreserve');
- 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 as any, 'select').mockReturnValue(mockD3Selection as any);
-
- // Capture the mouseover handler (namespaced event)
- mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) =>
{
- if (event === 'mouseover.fillPreserve') {
- 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 mockD3Selection;
- }
- return mockElement.getAttribute(attrName);
- }),
- 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 as any, 'select').mockReturnValue(mockD3Selection as any);
-
- // Capture the mouseout handler (namespaced event)
- mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) =>
{
- if (event === 'mouseout.fillPreserve') {
- 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 mockD3Selection;
- }
- return mockElement.getAttribute(attrName);
- }),
- 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 as any, 'select').mockReturnValue(mockD3Selection as any);
-
- // Capture the mouseout handler (namespaced event)
- mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) =>
{
- if (event === 'mouseout.fillPreserve') {
- 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 as any, 'select').mockReturnValue(mockD3Selection as any);
-
- // Capture namespaced event handlers
- mockSvg.on.mockImplementation((event: string, handler: MouseEventHandler) =>
{
- if (event === 'mouseover.fillPreserve') {
- mouseoverHandler = handler;
- }
- if (event === 'mouseout.fillPreserve') {
- 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,
- );
- 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(fillChangeCalls.length).toBe(0);
- expect(fillStyleChangeCalls.length).toBe(0);
+ expect(hasMouseover).toBe(false);
+ expect(hasMouseout).toBe(false);
Review Comment:
**Suggestion:** The test asserting that `mouseover.fillPreserve` and
`mouseout.fillPreserve` handlers are not registered on `.datamaps-subunit`
contradicts the actual `WorldMap` implementation, which still attaches these
handlers, so this expectation will consistently fail even when the chart
behaves correctly. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ WorldMap hover-handler test fails despite correct implementation.
- ⚠️ Frontend Jest suite for world map plugin unreliable.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the frontend Jest tests that include
`superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts`
(this file
is a standard Jest test file under
`plugins/legacy-plugin-chart-world-map/test`).
2. In `WorldMap.test.ts:15-24`, the test `sets up only contextmenu and click
handlers on
countries` calls `WorldMap(container, baseProps)` (line 16), importing the
implementation
from `../src/WorldMap` (import at line 21).
3. Inside
`superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.ts:311-324`,
the
Datamap `done` callback executes
`datamap.svg.selectAll('.datamaps-subunit').on('contextmenu',
handleContextMenu).on('click', handleClick).on('mouseover.fillPreserve',
...).on('mouseout.fillPreserve', ...)`, as shown by Grep matches at lines
311 and 324, so
`mockSvg.on` is invoked with both `'mouseover.fillPreserve'` and
`'mouseout.fillPreserve'`.
4. Back in `WorldMap.test.ts:27-34`, the test computes `hasMouseover` and
`hasMouseout` by
checking `onCalls` for those exact event names, then asserts
`expect(hasMouseover).toBe(false); expect(hasMouseout).toBe(false);`, which
fails because
the implementation correctly registers these handlers and both flags are
`true`.
```
</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:** 176:183
**Comment:**
*Logic Error: The test asserting that `mouseover.fillPreserve` and
`mouseout.fillPreserve` handlers are not registered on `.datamaps-subunit`
contradicts the actual `WorldMap` implementation, which still attaches these
handlers, so this expectation will consistently fail even when the chart
behaves correctly.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=d3d9391c9a1ebc88a340f75d09ecd02799c89bcc4831be66dfc24b6d06010b98&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=d3d9391c9a1ebc88a340f75d09ecd02799c89bcc4831be66dfc24b6d06010b98&reaction=dislike'>👎</a>
--
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]
