bito-code-review[bot] commented on code in PR #36644: URL: https://github.com/apache/superset/pull/36644#discussion_r2670086956
########## superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts: ########## @@ -0,0 +1,320 @@ +/** + * 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 configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { renderHook, act } from '@testing-library/react-hooks'; +import { createWrapper } from 'spec/helpers/testing-library'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; +import * as localStorageHelpers from 'src/utils/localStorageHelpers'; + +import useDatabaseSelector from './useDatabaseSelector'; + +const middlewares = [thunk]; +const mockStore = configureStore(middlewares); + +const mockDatabase = { + id: 1, + database_name: 'main', + backend: 'mysql', +}; + +const mockDatabases = { + [mockDatabase.id]: mockDatabase, +}; + +const createInitialState = (overrides = {}) => ({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + databases: mockDatabases, + ...overrides, + }, +}); + +beforeEach(() => { + jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(null); + jest.spyOn(localStorageHelpers, 'setItem').mockImplementation(() => {}); +}); + +afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); +}); + +test('returns initial values from query editor', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + expect(result.current.catalog).toBe(defaultQueryEditor.catalog); + expect(result.current.schema).toBe(defaultQueryEditor.schema); + expect(typeof result.current.onDbChange).toBe('function'); + expect(typeof result.current.onCatalogChange).toBe('function'); + expect(typeof result.current.onSchemaChange).toBe('function'); + expect(typeof result.current.getDbList).toBe('function'); + expect(typeof result.current.handleError).toBe('function'); +}); + +test('returns database when dbId exists in store', () => { + const store = mockStore( + createInitialState({ + unsavedQueryEditor: { + id: defaultQueryEditor.id, + dbId: mockDatabase.id, + }, + }), + ); + + const { result, rerender } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Trigger effect by rerendering + rerender(); + + expect(result.current.db).toEqual(mockDatabase); +}); + +test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onDbChange({ id: 2 }); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SETDB', + dbId: 2, + }), + ); +}); + +test('dispatches queryEditorSetCatalog action on onCatalogChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onCatalogChange('new_catalog'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_CATALOG', + }), + ); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete dispatch assertion</b></div> <div id="fix"> Test checks type but not payload.catalog. Add expect.objectContaining({ catalog: 'new_catalog' }). </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts +++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts @@ -149,6 +149,7 @@ test('dispatches queryEditorSetCatalog action on onCatalogChange', () => { expect(actions).toContainEqual( expect.objectContaining({ type: 'QUERY_EDITOR_SET_CATALOG', + catalog: 'new_catalog', }), ); }); ``` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/c689c41/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run #09d82e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/components/ViewExtension/ViewExtension.test.tsx: ########## @@ -0,0 +1,202 @@ +/** + * 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 { ReactElement } from 'react'; +import { render, screen } from 'spec/helpers/testing-library'; +import type { contributions, core } from '@apache-superset/core'; +import ExtensionsManager from 'src/extensions/ExtensionsManager'; +import { ExtensionsProvider } from 'src/extensions/ExtensionsContext'; +import ViewExtension from 'src/components/ViewExtension'; + +function createMockView( + id: string, + overrides: Partial<contributions.ViewContribution> = {}, +): contributions.ViewContribution { + return { + id, + name: `${id} View`, + ...overrides, + }; +} + +function createMockExtension( + options: Partial<core.Extension> & { + views?: Record<string, contributions.ViewContribution[]>; + } = {}, +): core.Extension { + const { + id = 'test-extension', + name = 'Test Extension', + views = {}, + } = options; + + return { + id, + name, + description: 'A test extension', + version: '1.0.0', + dependencies: [], + remoteEntry: '', + exposedModules: [], + extensionDependencies: [], + contributions: { + commands: [], + menus: {}, + views, + }, + activate: jest.fn(), + deactivate: jest.fn(), + }; +} + +function setupActivatedExtension( + manager: ExtensionsManager, + extension: core.Extension, +) { + const context = { disposables: [] }; + (manager as any).contextIndex.set(extension.id, context); + (manager as any).extensionContributions.set(extension.id, { + commands: extension.contributions.commands, + menus: extension.contributions.menus, + views: extension.contributions.views, + }); +} + +async function createActivatedExtension( + manager: ExtensionsManager, + extensionOptions: Parameters<typeof createMockExtension>[0] = {}, +): Promise<core.Extension> { + const mockExtension = createMockExtension(extensionOptions); + await manager.initializeExtension(mockExtension); + setupActivatedExtension(manager, mockExtension); + return mockExtension; +} + +const TEST_VIEW_ID = 'test.view'; + +const renderWithExtensionsProvider = (ui: ReactElement) => { + return render(ui, { wrapper: ExtensionsProvider as any }); +}; + +beforeEach(() => { + (ExtensionsManager as any).instance = undefined; +}); + +afterEach(() => { + (ExtensionsManager as any).instance = undefined; +}); + +test('renders nothing when no view contributions exist', () => { + const { container } = renderWithExtensionsProvider( + <ViewExtension viewId={TEST_VIEW_ID} />, + ); + + expect(container.firstChild?.childNodes.length ?? 0).toBe(0); +}); + +test('renders placeholder for unregistered view provider', async () => { + const manager = ExtensionsManager.getInstance(); + + await createActivatedExtension(manager, { + views: { + [TEST_VIEW_ID]: [createMockView('test-view-1')], + }, + }); + + renderWithExtensionsProvider(<ViewExtension viewId={TEST_VIEW_ID} />); + + expect(screen.getByText(/test-view-1/)).toBeInTheDocument(); +}); + +test('renders multiple view placeholders for multiple contributions', async () => { + const manager = ExtensionsManager.getInstance(); + + await createActivatedExtension(manager, { + views: { + [TEST_VIEW_ID]: [ + createMockView('test-view-1'), + createMockView('test-view-2'), + ], + }, + }); + + renderWithExtensionsProvider(<ViewExtension viewId={TEST_VIEW_ID} />); + + expect(screen.getByText(/test-view-1/)).toBeInTheDocument(); + expect(screen.getByText(/test-view-2/)).toBeInTheDocument(); +}); + +test('renders nothing for viewId with no matching contributions', () => { + const { container } = renderWithExtensionsProvider( + <ViewExtension viewId="nonexistent.view" />, + ); + + expect(container.firstChild?.childNodes.length ?? 0).toBe(0); +}); + +test('handles multiple extensions with views for same viewId', async () => { + const manager = ExtensionsManager.getInstance(); + + await createActivatedExtension(manager, { + id: 'extension-1', + views: { + [TEST_VIEW_ID]: [createMockView('ext1-view')], + }, + }); + + await createActivatedExtension(manager, { + id: 'extension-2', + views: { + [TEST_VIEW_ID]: [createMockView('ext2-view')], + }, + }); + + renderWithExtensionsProvider(<ViewExtension viewId={TEST_VIEW_ID} />); + + expect(screen.getByText(/ext1-view/)).toBeInTheDocument(); + expect(screen.getByText(/ext2-view/)).toBeInTheDocument(); +}); + +test('renders views for different viewIds independently', async () => { + const manager = ExtensionsManager.getInstance(); + const VIEW_ID_A = 'view.a'; + const VIEW_ID_B = 'view.b'; + + await createActivatedExtension(manager, { + views: { + [VIEW_ID_A]: [createMockView('view-a-component')], + [VIEW_ID_B]: [createMockView('view-b-component')], + }, + }); + + const { rerender } = renderWithExtensionsProvider( + <ViewExtension viewId={VIEW_ID_A} />, + ); + + expect(screen.getByText(/view-a-component/)).toBeInTheDocument(); + expect(screen.queryByText(/view-b-component/)).not.toBeInTheDocument(); + + rerender( + <ExtensionsProvider> + <ViewExtension viewId={VIEW_ID_B} /> + </ExtensionsProvider>, + ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test Logic Bug</b></div> <div id="fix"> The rerender in this test incorrectly wraps the component with ExtensionsProvider again, leading to double wrapping since the initial render already uses this wrapper. This could cause unexpected behavior in context or state. Change to rerender(<ViewExtension viewId={VIEW_ID_B} />) instead. </div> </div> <small><i>Code Review Run #09d82e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts: ########## @@ -0,0 +1,320 @@ +/** + * 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 configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { renderHook, act } from '@testing-library/react-hooks'; +import { createWrapper } from 'spec/helpers/testing-library'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; +import * as localStorageHelpers from 'src/utils/localStorageHelpers'; + +import useDatabaseSelector from './useDatabaseSelector'; + +const middlewares = [thunk]; +const mockStore = configureStore(middlewares); + +const mockDatabase = { + id: 1, + database_name: 'main', + backend: 'mysql', +}; + +const mockDatabases = { + [mockDatabase.id]: mockDatabase, +}; + +const createInitialState = (overrides = {}) => ({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + databases: mockDatabases, + ...overrides, + }, +}); + +beforeEach(() => { + jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(null); + jest.spyOn(localStorageHelpers, 'setItem').mockImplementation(() => {}); +}); + +afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); +}); + +test('returns initial values from query editor', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + expect(result.current.catalog).toBe(defaultQueryEditor.catalog); + expect(result.current.schema).toBe(defaultQueryEditor.schema); + expect(typeof result.current.onDbChange).toBe('function'); + expect(typeof result.current.onCatalogChange).toBe('function'); + expect(typeof result.current.onSchemaChange).toBe('function'); + expect(typeof result.current.getDbList).toBe('function'); + expect(typeof result.current.handleError).toBe('function'); +}); + +test('returns database when dbId exists in store', () => { + const store = mockStore( + createInitialState({ + unsavedQueryEditor: { + id: defaultQueryEditor.id, + dbId: mockDatabase.id, + }, + }), + ); + + const { result, rerender } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Trigger effect by rerendering + rerender(); + + expect(result.current.db).toEqual(mockDatabase); +}); + +test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onDbChange({ id: 2 }); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SETDB', + dbId: 2, + }), + ); +}); + +test('dispatches queryEditorSetCatalog action on onCatalogChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onCatalogChange('new_catalog'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_CATALOG', + }), + ); +}); + +test('dispatches queryEditorSetSchema action on onSchemaChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onSchemaChange('new_schema'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_SCHEMA', + }), + ); +}); + +test('dispatches setDatabases action on getDbList', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + const newDatabase = { + id: 3, + database_name: 'test_db', + backend: 'postgresql', + }; + + act(() => { + result.current.getDbList(newDatabase as any); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'SET_DATABASES', + }), + ); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Type mismatch in getDbList test</b></div> <div id="fix"> Test passes single object to getDbList, but hook expects DatabaseObject[]. Change to [newDatabase] and assert payload.databases. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts +++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts @@ -192,7 +192,7 @@ test('dispatches setDatabases action on getDbList', () => { const newDatabase = { id: 3, database_name: 'test_db', backend: 'postgresql', }; act(() => { - result.current.getDbList(newDatabase as any); + result.current.getDbList([newDatabase]); }); const actions = store.getActions(); expect(actions).toContainEqual( expect.objectContaining({ type: 'SET_DATABASES', + databases: [newDatabase], }), ); }); ``` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/c689c41/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run #09d82e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/SqlLab/components/SqlEditor/SqlEditor.test.tsx: ########## @@ -321,7 +321,7 @@ describe('SqlEditor', () => { const defaultQueryLimit = 101; const updatedProps = { ...mockedProps, defaultQueryLimit }; const { findByText } = setup(updatedProps, store); - fireEvent.click(await findByText('LIMIT:')); + fireEvent.click(await findByText('Limit')); expect(await findByText('10 000')).toBeInTheDocument(); }); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test assertion mismatch</b></div> <div id="fix"> The change to findByText('Limit') correctly updates the test to match the component's button text, but the related assertion on line 326 expects '10 000' while the component renders '10 000 ' (with trailing space) in the dropdown options. This could cause the test to fail after opening the dropdown. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion fireEvent.click(await findByText('Limit')); expect(await findByText('10 000 ')).toBeInTheDocument(); }); ```` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/c689c41/.cursor/rules/dev-standard.mdc#L97";>dev-standard.mdc:97</a> </li> </ul> </details> <small><i>Code Review Run #09d82e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts: ########## @@ -0,0 +1,320 @@ +/** + * 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 configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { renderHook, act } from '@testing-library/react-hooks'; +import { createWrapper } from 'spec/helpers/testing-library'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; +import * as localStorageHelpers from 'src/utils/localStorageHelpers'; + +import useDatabaseSelector from './useDatabaseSelector'; + +const middlewares = [thunk]; +const mockStore = configureStore(middlewares); + +const mockDatabase = { + id: 1, + database_name: 'main', + backend: 'mysql', +}; + +const mockDatabases = { + [mockDatabase.id]: mockDatabase, +}; + +const createInitialState = (overrides = {}) => ({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + databases: mockDatabases, + ...overrides, + }, +}); + +beforeEach(() => { + jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(null); + jest.spyOn(localStorageHelpers, 'setItem').mockImplementation(() => {}); +}); + +afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); +}); + +test('returns initial values from query editor', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + expect(result.current.catalog).toBe(defaultQueryEditor.catalog); + expect(result.current.schema).toBe(defaultQueryEditor.schema); + expect(typeof result.current.onDbChange).toBe('function'); + expect(typeof result.current.onCatalogChange).toBe('function'); + expect(typeof result.current.onSchemaChange).toBe('function'); + expect(typeof result.current.getDbList).toBe('function'); + expect(typeof result.current.handleError).toBe('function'); +}); + +test('returns database when dbId exists in store', () => { + const store = mockStore( + createInitialState({ + unsavedQueryEditor: { + id: defaultQueryEditor.id, + dbId: mockDatabase.id, + }, + }), + ); + + const { result, rerender } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Trigger effect by rerendering + rerender(); + + expect(result.current.db).toEqual(mockDatabase); +}); + +test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onDbChange({ id: 2 }); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SETDB', + dbId: 2, + }), + ); +}); + +test('dispatches queryEditorSetCatalog action on onCatalogChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onCatalogChange('new_catalog'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_CATALOG', + }), + ); +}); + +test('dispatches queryEditorSetSchema action on onSchemaChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onSchemaChange('new_schema'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_SCHEMA', + }), + ); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete dispatch assertion</b></div> <div id="fix"> Test checks type but not payload.schema. Add expect.objectContaining({ schema: 'new_schema' }). </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts +++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts @@ -173,6 +173,7 @@ test('dispatches queryEditorSetSchema action on onSchemaChange', () => { expect(actions).toContainEqual( expect.objectContaining({ type: 'QUERY_EDITOR_SET_SCHEMA', + schema: 'new_schema', }), ); }); ``` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/c689c41/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run #09d82e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts: ########## @@ -0,0 +1,320 @@ +/** + * 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 configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { renderHook, act } from '@testing-library/react-hooks'; +import { createWrapper } from 'spec/helpers/testing-library'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; +import * as localStorageHelpers from 'src/utils/localStorageHelpers'; + +import useDatabaseSelector from './useDatabaseSelector'; + +const middlewares = [thunk]; +const mockStore = configureStore(middlewares); + +const mockDatabase = { + id: 1, + database_name: 'main', + backend: 'mysql', +}; + +const mockDatabases = { + [mockDatabase.id]: mockDatabase, +}; + +const createInitialState = (overrides = {}) => ({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + databases: mockDatabases, + ...overrides, + }, +}); + +beforeEach(() => { + jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(null); + jest.spyOn(localStorageHelpers, 'setItem').mockImplementation(() => {}); +}); + +afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); +}); + +test('returns initial values from query editor', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + expect(result.current.catalog).toBe(defaultQueryEditor.catalog); + expect(result.current.schema).toBe(defaultQueryEditor.schema); + expect(typeof result.current.onDbChange).toBe('function'); + expect(typeof result.current.onCatalogChange).toBe('function'); + expect(typeof result.current.onSchemaChange).toBe('function'); + expect(typeof result.current.getDbList).toBe('function'); + expect(typeof result.current.handleError).toBe('function'); +}); + +test('returns database when dbId exists in store', () => { + const store = mockStore( + createInitialState({ + unsavedQueryEditor: { + id: defaultQueryEditor.id, + dbId: mockDatabase.id, + }, + }), + ); + + const { result, rerender } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Trigger effect by rerendering + rerender(); + + expect(result.current.db).toEqual(mockDatabase); +}); + +test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onDbChange({ id: 2 }); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SETDB', + dbId: 2, + }), + ); +}); + +test('dispatches queryEditorSetCatalog action on onCatalogChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onCatalogChange('new_catalog'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_CATALOG', + }), + ); +}); + +test('dispatches queryEditorSetSchema action on onSchemaChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onSchemaChange('new_schema'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_SCHEMA', + }), + ); +}); + +test('dispatches setDatabases action on getDbList', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + const newDatabase = { + id: 3, + database_name: 'test_db', + backend: 'postgresql', + }; + + act(() => { + result.current.getDbList(newDatabase as any); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'SET_DATABASES', + }), + ); +}); + +test('dispatches addDangerToast action on handleError', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.handleError('Test error message'); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'ADD_TOAST', + payload: expect.objectContaining({ + toastType: 'DANGER_TOAST', + text: 'Test error message', + }), + }), + ); +}); + +test('reads database from localStorage when URL has db param', () => { + const localStorageDb = { + id: 5, + database_name: 'local_storage_db', + backend: 'sqlite', + }; + + jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(localStorageDb); + + const originalLocation = window.location; + Object.defineProperty(window, 'location', { + value: { search: '?db=true' }, + writable: true, + }); + + const store = mockStore(createInitialState()); + const { result, rerender } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + rerender(); + + expect(result.current.db).toEqual(localStorageDb); + expect(localStorageHelpers.setItem).toHaveBeenCalledWith( + localStorageHelpers.LocalStorageKeys.Database, + null, + ); + + Object.defineProperty(window, 'location', { + value: originalLocation, + writable: true, + }); +}); + +test('returns null db when dbId does not exist in databases', () => { + const store = mockStore( + createInitialState({ + databases: {}, + }), + ); + + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + expect(result.current.db).toBeNull(); +}); + +test('handles null catalog change', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onCatalogChange(null); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SET_CATALOG', + }), + ); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete dispatch assertion</b></div> <div id="fix"> Test checks type but not payload.catalog === null. Add to expect.objectContaining. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts +++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts @@ -315,6 +315,7 @@ test('handles null catalog change', () => { expect(actions).toContainEqual( expect.objectContaining({ type: 'QUERY_EDITOR_SET_CATALOG', + catalog: null, }), ); }); ``` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/c689c41/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run #09d82e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts: ########## @@ -0,0 +1,320 @@ +/** + * 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 configureStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import { renderHook, act } from '@testing-library/react-hooks'; +import { createWrapper } from 'spec/helpers/testing-library'; +import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures'; +import * as localStorageHelpers from 'src/utils/localStorageHelpers'; + +import useDatabaseSelector from './useDatabaseSelector'; + +const middlewares = [thunk]; +const mockStore = configureStore(middlewares); + +const mockDatabase = { + id: 1, + database_name: 'main', + backend: 'mysql', +}; + +const mockDatabases = { + [mockDatabase.id]: mockDatabase, +}; + +const createInitialState = (overrides = {}) => ({ + ...initialState, + sqlLab: { + ...initialState.sqlLab, + databases: mockDatabases, + ...overrides, + }, +}); + +beforeEach(() => { + jest.spyOn(localStorageHelpers, 'getItem').mockReturnValue(null); + jest.spyOn(localStorageHelpers, 'setItem').mockImplementation(() => {}); +}); + +afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); +}); + +test('returns initial values from query editor', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + expect(result.current.catalog).toBe(defaultQueryEditor.catalog); + expect(result.current.schema).toBe(defaultQueryEditor.schema); + expect(typeof result.current.onDbChange).toBe('function'); + expect(typeof result.current.onCatalogChange).toBe('function'); + expect(typeof result.current.onSchemaChange).toBe('function'); + expect(typeof result.current.getDbList).toBe('function'); + expect(typeof result.current.handleError).toBe('function'); +}); + +test('returns database when dbId exists in store', () => { + const store = mockStore( + createInitialState({ + unsavedQueryEditor: { + id: defaultQueryEditor.id, + dbId: mockDatabase.id, + }, + }), + ); + + const { result, rerender } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + // Trigger effect by rerendering + rerender(); + + expect(result.current.db).toEqual(mockDatabase); +}); + +test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => { + const store = mockStore(createInitialState()); + const { result } = renderHook( + () => useDatabaseSelector(defaultQueryEditor.id), + { + wrapper: createWrapper({ + useRedux: true, + store, + }), + }, + ); + + act(() => { + result.current.onDbChange({ id: 2 }); + }); + + const actions = store.getActions(); + expect(actions).toContainEqual( + expect.objectContaining({ + type: 'QUERY_EDITOR_SETDB', + dbId: 2, + }), + ); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete dispatch assertion</b></div> <div id="fix"> Test checks dispatch type but not payload.dbId. Add expect.objectContaining({ dbId: 2 }). </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` --- a/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts +++ b/superset-frontend/src/SqlLab/components/SqlEditorTopBar/useDatabaseSelector.test.ts @@ -124,6 +124,7 @@ test('dispatches QUERY_EDITOR_SETDB action on onDbChange', () => { expect(actions).toContainEqual( expect.objectContaining({ type: 'QUERY_EDITOR_SETDB', + dbId: 2, }), ); }); ``` </div> </details> </div> <details> <summary><b>Citations</b></summary> <ul> <li> Rule Violated: <a href="https://github.com/apache/superset/blob/c689c41/AGENTS.md#L21";>AGENTS.md:21</a> </li> </ul> </details> <small><i>Code Review Run #09d82e</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
