codeant-ai-for-open-source[bot] commented on code in PR #37221: URL: https://github.com/apache/superset/pull/37221#discussion_r2715398897
########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx: ########## @@ -0,0 +1,515 @@ +/** + * 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, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { supersetTheme, ThemeProvider } from '@apache-superset/core/ui'; +import { Provider } from 'react-redux'; +import { configureStore } from '@reduxjs/toolkit'; +import { SupersetClient } from '@superset-ui/core'; +import DeckMulti from './Multi'; +import * as fitViewportModule from '../utils/fitViewport'; + +// Mock DeckGLContainer +jest.mock('../DeckGLContainer', () => ({ + DeckGLContainerStyledWrapper: ({ viewport, layers }: any) => ( + <div + data-test="deckgl-container" + data-viewport={JSON.stringify(viewport)} Review Comment: **Suggestion:** The mocked DeckGL container sets a custom attribute `data-test` but tests query `data-testid` via `getByTestId`, so the test will fail to find the mocked element; also JSON.stringify on an undefined `viewport` can produce the string "undefined" which JSON.parse will throw on. Change the mock to expose `data-testid` and safely stringify the viewport (fallback to an empty object). [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ DeckMulti unit tests fail to find mock container. - ❌ CI job fails running this test file. - ⚠️ Parsing viewport is already guarded; minor. ``` </details> ```suggestion data-testid="deckgl-container" data-viewport={JSON.stringify(viewport ?? {})} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the test suite for this file: execute Jest for superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx. 2. The test at line 204-213 ('should use adjusted viewport when autozoom is enabled') calls screen.getByTestId('deckgl-container') (see line ~205). The mock renders a div with attribute data-test="deckgl-container" (file Multi.test.tsx:28-39), not data-testid. 3. getByTestId will throw / fail to find the element because the mocked element does not expose data-testid; the assertion that reads the viewport (lines ~206-213) will therefore not run and the test fails with an error from getByTestId. 4. Note: JSON.stringify(viewport) when viewport is undefined results in the attribute being omitted; tests already guard with getAttribute('data-viewport') || '{}' so JSON.parse is safe. The concrete failing issue is the attribute name mismatch. ``` </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-preset-chart-deckgl/src/Multi/Multi.test.tsx **Line:** 32:33 **Comment:** *Logic Error: The mocked DeckGL container sets a custom attribute `data-test` but tests query `data-testid` via `getByTestId`, so the test will fail to find the mocked element; also JSON.stringify on an undefined `viewport` can produce the string "undefined" which JSON.parse will throw on. Change the mock to expose `data-testid` and safely stringify the viewport (fallback to an empty object). 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-preset-chart-deckgl/src/Multi/Multi.test.tsx: ########## @@ -0,0 +1,515 @@ +/** + * 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, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { supersetTheme, ThemeProvider } from '@apache-superset/core/ui'; +import { Provider } from 'react-redux'; +import { configureStore } from '@reduxjs/toolkit'; +import { SupersetClient } from '@superset-ui/core'; +import DeckMulti from './Multi'; +import * as fitViewportModule from '../utils/fitViewport'; + +// Mock DeckGLContainer +jest.mock('../DeckGLContainer', () => ({ + DeckGLContainerStyledWrapper: ({ viewport, layers }: any) => ( + <div + data-test="deckgl-container" + data-viewport={JSON.stringify(viewport)} + data-layers-count={layers?.length || 0} + > + DeckGL Container Mock + </div> + ), +})); + +// Mock SupersetClient +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + SupersetClient: { + get: jest.fn(), + }, +})); + +const mockStore = configureStore({ + reducer: { + dataMask: () => ({}), + }, +}); + +const baseMockProps = { + formData: { + datasource: 'test_datasource', + viz_type: 'deck_multi', + deck_slices: [1, 2], + autozoom: false, + mapbox_style: 'mapbox://styles/mapbox/light-v9', + }, + payload: { + data: { + slices: [ + { + slice_id: 1, + form_data: { + viz_type: 'deck_scatter', + datasource: 'test_datasource', + }, + }, + { + slice_id: 2, + form_data: { + viz_type: 'deck_polygon', + datasource: 'test_datasource', + }, + }, + ], + features: { + deck_scatter: [{ position: [0, 0] }], + deck_polygon: [ + { + polygon: [ + [1, 1], + [2, 2], + ], + }, + ], + deck_path: [], + deck_grid: [], + deck_contour: [], + deck_heatmap: [], + deck_hex: [], + deck_arc: [], + deck_geojson: [], + deck_screengrid: [], + }, + mapboxApiKey: 'test-key', + }, + }, + setControlValue: jest.fn(), + viewport: { longitude: 0, latitude: 0, zoom: 1 }, + onAddFilter: jest.fn(), + height: 600, + width: 800, + datasource: { id: 1, type: 'table' }, + onSelect: jest.fn(), +}; + +const renderWithProviders = (component: React.ReactElement) => + render( + <Provider store={mockStore}> + <ThemeProvider theme={supersetTheme}>{component}</ThemeProvider> + </Provider>, + ); + +describe('DeckMulti Autozoom Functionality', () => { + beforeEach(() => { + jest.clearAllMocks(); + (SupersetClient.get as jest.Mock).mockResolvedValue({ + json: { + data: { + features: [], + }, + }, Review Comment: **Suggestion:** The `SupersetClient.get` mock resolves to an object where `json` is a plain object, but real code likely calls `await response.json()` (a function). This will cause "response.json is not a function" at runtime; make `json` a function that returns the expected payload (mockResolvedValue of a function that returns the data). [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ DeckMulti tests throw TypeError during component mount. - ❌ CI test runs fail for this spec file. - ⚠️ Mocked HTTP responses behave unlike real fetch responses. ``` </details> ```suggestion json: jest.fn().mockResolvedValue({ data: { features: [], }, }), ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The test file sets up a mock in the top-level beforeEach at superset-frontend/plugins/legacy-preset-chart-deckgl/src/Multi/Multi.test.tsx:120-129. The mock resolves SupersetClient.get to an object where `json` is a plain object. 2. Rendering the component under test (renderWithProviders(<DeckMulti ... />)) at e.g. Multi.test.tsx:142 triggers DeckMulti (imported at Multi.test.tsx:25). 3. DeckMulti's data-loading code (in ./Multi, invoked on mount) follows the fetch pattern and calls await response.json() (standard usage); when the mock response is used, response.json is NOT a function and the component code will throw: TypeError: response.json is not a function. 4. The failing call occurs during render in the tests (examples: renders at lines 142, 166, 202, 234, 278, 327, 352, 388, 424, 456 in this test file), causing test 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-preset-chart-deckgl/src/Multi/Multi.test.tsx **Line:** 123:127 **Comment:** *Possible Bug: The `SupersetClient.get` mock resolves to an object where `json` is a plain object, but real code likely calls `await response.json()` (a function). This will cause "response.json is not a function" at runtime; make `json` a function that returns the expected payload (mockResolvedValue of a function that returns the data). 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]
