codeant-ai-for-open-source[bot] commented on code in PR #37298: URL: https://github.com/apache/superset/pull/37298#discussion_r2747349259
########## superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx: ########## @@ -0,0 +1,241 @@ +/** + * 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 type { ReactChild } from 'react'; +import fetchMock from 'fetch-mock'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; + +import TableExploreTree from '.'; + +jest.mock( + 'react-virtualized-auto-sizer', + () => + ({ children }: { children: (params: { height: number }) => ReactChild }) => + children({ height: 500 }), Review Comment: **Suggestion:** The AutoSizer mock only provides `height` to the child render function but many consumers expect both `width` and `height`; if the real component destructures `width` this will be undefined and can cause layout issues or runtime errors. Mock the same shape the real AutoSizer provides by passing both `width` and `height`. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ TableExploreTree test rendering may differ from real layout. - ⚠️ SQLLab treeview layout assertions could be flaky. - ⚠️ Real AutoSizer-related regressions may be invisible in tests. ``` </details> ```suggestion ({ children }: { children: (params: { width: number; height: number }) => ReactChild }) => children({ width: 500, height: 500 }), ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the tests in this file: execute Jest for superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx. The test file sets up a mock for AutoSizer at lines 27-32 (jest.mock(...) returning children({ height: 500 })). 2. The render flow starts at renderComponent() (lines 97-101) which calls render(<TableExploreTree ... />). TableExploreTree will be mounted inside the test with the mocked AutoSizer in place (mock replaces the real module). 3. If TableExploreTree (the component under test) expects both width and height from AutoSizer (common pattern), its internal code will receive params.width === undefined because the mock only supplies height. This can cause layout calculations or style logic that destructures width to use undefined, producing unexpected layout or runtime errors during rendering. 4. Observed behaviour when this occurs: tests may render differently, assertions about visible nodes (tree layout) can fail, or the component may throw if it assumes numeric width. If the component does not need width, the tests still pass — this mock omission is a subtle mismatch with the real AutoSizer API and may mask regressions. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx **Line:** 30:31 **Comment:** *Possible Bug: The AutoSizer mock only provides `height` to the child render function but many consumers expect both `width` and `height`; if the real component destructures `width` this will be undefined and can cause layout issues or runtime errors. Mock the same shape the real AutoSizer provides by passing both `width` and `height`. 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/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx: ########## @@ -0,0 +1,241 @@ +/** + * 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 type { ReactChild } from 'react'; +import fetchMock from 'fetch-mock'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; + +import TableExploreTree from '.'; + +jest.mock( + 'react-virtualized-auto-sizer', + () => + ({ children }: { children: (params: { height: number }) => ReactChild }) => + children({ height: 500 }), +); + +const mockedQueryEditorId = defaultQueryEditor.id; +const mockedDatabase = { + id: 1, + database_name: 'main', + backend: 'mysql', +}; + +const mockSchemas = ['public', 'information_schema', 'test_schema']; +const mockTables = [ + { label: 'users', value: 'users', type: 'table' }, + { label: 'orders', value: 'orders', type: 'table' }, + { label: 'user_view', value: 'user_view', type: 'view' }, +]; +const mockColumns = [ + { name: 'id', type: 'INTEGER', keys: [{ type: 'pk' }] }, + { name: 'name', type: 'VARCHAR(255)' }, + { name: 'created_at', type: 'TIMESTAMP' }, +]; + +beforeEach(() => { + fetchMock.get('glob:*/api/v1/database/1/schemas/?*', { + count: mockSchemas.length, + result: mockSchemas, + }); + fetchMock.get('glob:*/api/v1/database/1/tables/*', { + count: mockTables.length, + result: mockTables, + }); + fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', { + status: 200, + body: { + columns: mockColumns, + }, + }); + fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', { + status: 200, + body: {}, Review Comment: **Suggestion:** The generic `table_metadata/*` mock is registered before the more specific `table_metadata/extra/*` mock, causing the generic route to match and shadow the specific one; register the specific `extra/*` route first (or use a more specific pattern) so the intended response for the extra endpoint is returned. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Tests relying on extra metadata receive wrong payload. - ⚠️ TableExploreTree behavior under extra metadata affected. - ⚠️ Mock mismatch can hide API contract regressions. ``` </details> ```suggestion // register the more specific `extra/*` route before the generic `table_metadata/*` fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', { status: 200, body: {}, }); fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', { status: 200, body: { columns: mockColumns, }, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the test file superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx. The beforeEach at line 53 registers fetch-mock routes starting at line 54. 2. The generic route for table metadata is registered at line 62: fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', ...) returning columns (lines 62-67). 3. A more specific route for extra metadata is registered afterwards at lines 68-71: fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', ...) returning {}. 4. When the component under test issues a request to /api/v1/database/1/table_metadata/extra/<...>, fetch-mock matches routes in registration order, so the earlier generic 'table_metadata/*' (registered at line 62) will match first and be used, causing the extra/* request to receive the generic response (columns) instead of the intended {}. 5. Reproduce by running the test that triggers the extra metadata path (e.g., expand actions that cause TableExploreTree to fetch extra metadata) and observe the response body not matching expectations. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx **Line:** 62:70 **Comment:** *Logic Error: The generic `table_metadata/*` mock is registered before the more specific `table_metadata/extra/*` mock, causing the generic route to match and shadow the specific one; register the specific `extra/*` route first (or use a more specific pattern) so the intended response for the extra endpoint is returned. 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/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx: ########## @@ -0,0 +1,241 @@ +/** + * 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 type { ReactChild } from 'react'; +import fetchMock from 'fetch-mock'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; + +import TableExploreTree from '.'; + +jest.mock( + 'react-virtualized-auto-sizer', + () => + ({ children }: { children: (params: { height: number }) => ReactChild }) => + children({ height: 500 }), +); + +const mockedQueryEditorId = defaultQueryEditor.id; +const mockedDatabase = { + id: 1, + database_name: 'main', + backend: 'mysql', +}; + +const mockSchemas = ['public', 'information_schema', 'test_schema']; +const mockTables = [ + { label: 'users', value: 'users', type: 'table' }, + { label: 'orders', value: 'orders', type: 'table' }, + { label: 'user_view', value: 'user_view', type: 'view' }, +]; +const mockColumns = [ + { name: 'id', type: 'INTEGER', keys: [{ type: 'pk' }] }, + { name: 'name', type: 'VARCHAR(255)' }, + { name: 'created_at', type: 'TIMESTAMP' }, +]; + +beforeEach(() => { + fetchMock.get('glob:*/api/v1/database/1/schemas/?*', { + count: mockSchemas.length, + result: mockSchemas, + }); + fetchMock.get('glob:*/api/v1/database/1/tables/*', { + count: mockTables.length, + result: mockTables, + }); + fetchMock.get('glob:*/api/v1/database/1/table_metadata/*', { + status: 200, + body: { + columns: mockColumns, + }, + }); + fetchMock.get('glob:*/api/v1/database/1/table_metadata/extra/*', { + status: 200, + body: {}, + }); +}); + +afterEach(() => { + jest.clearAllMocks(); + fetchMock.clearHistory(); Review Comment: **Suggestion:** The test teardown calls only `fetchMock.clearHistory()` which leaves registered routes in place across tests; this can accumulate duplicate routes or cause tests in other files to see stale mocks. Restore/reset fetch-mock in afterEach to fully remove registered handlers and avoid cross-test interference. [resource leak] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Other tests observe stale network mocks. - ❌ Test suite flakiness and false failures. - ⚠️ CI runs may become unstable intermittently. ``` </details> ```suggestion // fully restore/reset fetch-mock so routes do not leak between tests // `restore` removes registered mocks; `resetHistory` clears call history fetchMock.restore(); fetchMock.resetHistory(); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the full Jest suite including this file. The afterEach at lines 74-77 currently only calls jest.clearAllMocks() and fetchMock.clearHistory(), leaving registered fetch-mock routes in place. 2. Another test file (for example, packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx) runs after this test file and performs network interactions; because routes remain registered, those tests may match the mocks defined here and receive unexpected mocked responses. 3. Observe cross-test interference: unrelated tests see network calls intercepted by this file's mocks, causing flaky failures or unexpected pass/fail behavior across the suite. 4. Fix reproduction: change afterEach to call fetchMock.restore() and fetchMock.resetHistory() so registered handlers are removed and call history cleared, preventing leakage when running the test suite. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/TableExploreTree.test.tsx **Line:** 76:76 **Comment:** *Resource Leak: The test teardown calls only `fetchMock.clearHistory()` which leaves registered routes in place across tests; this can accumulate duplicate routes or cause tests in other files to see stale mocks. Restore/reset fetch-mock in afterEach to fully remove registered handlers and avoid cross-test interference. 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]
