bito-code-review[bot] commented on code in PR #33002:
URL: https://github.com/apache/superset/pull/33002#discussion_r2845108133
##########
superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts:
##########
@@ -152,20 +208,12 @@ describe('Charts list', () => {
// edits in list-view
setGridMode('list');
- // Wait for list view to fully render after mode change
- cy.get('.loading').should('not.exist');
- cy.getBySel('table-row').should('be.visible');
- // Target the specific row by chart title to avoid flakiness from row
ordering
- cy.getBySel('table-row')
- .contains('1 - Sample chart | EDITED')
- .parents('[data-test="table-row"]')
- .find('[data-test="edit-alt"]')
- .click();
+ cy.getBySel('edit-alt').eq(1).click();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect selector index</b></div>
<div id="fix">
The test creates only one sample chart, so cy.getBySel('edit-alt').eq(1)
attempts to click a non-existent second edit button, likely causing the test to
fail with an index out of bounds error. Since there's only one chart in the
list view, use .eq(0) instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
cy.getBySel('edit-alt').eq(0).click();
````
</div>
</details>
</div>
<small><i>Code Review Run #9cc4cc</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/pages/ChartList/ChartList.test.tsx:
##########
@@ -153,56 +149,107 @@ describe('ChartList', () => {
);
});
- test('makes correct API calls on initial load', async () => {
+ it('makes correct API calls on initial load', async () => {
renderChartList(mockUser);
await waitFor(() => {
- const infoCalls = fetchMock.callHistory.calls(/chart\/_info/);
- const dataCalls = fetchMock.callHistory.calls(/chart\/\?q/);
+ const infoCalls = fetchMock.calls(/chart\/_info/);
+ const dataCalls = fetchMock.calls(/chart\/\?q/);
expect(infoCalls).toHaveLength(1);
expect(dataCalls).toHaveLength(1);
- expect(dataCalls[0].url).toContain(
+ expect(dataCalls[0][0]).toContain(
'order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25',
);
});
});
- test('displays Matrixify tag for charts with matrixify enabled', async () =>
{
+ it('shows loading state while API calls are in progress', async () => {
+ // Mock delayed API responses
+ fetchMock.get(
+ API_ENDPOINTS.CHARTS_INFO,
+ new Promise(resolve =>
+ setTimeout(
+ () => resolve({ permissions: ['can_read', 'can_write'] }),
+ 100,
+ ),
+ ),
+ { overwriteRoutes: true },
+ );
+
+ fetchMock.get(
+ API_ENDPOINTS.CHARTS,
+ new Promise(resolve =>
+ setTimeout(() => resolve({ result: mockCharts, chart_count: 3 }), 150),
+ ),
+ { overwriteRoutes: true },
+ );
+
renderChartList(mockUser);
- // Wait for the chart list to load
- await waitFor(() => {
- expect(screen.getByText('Test Chart 0')).toBeInTheDocument();
- });
+ // Main container should render immediately
+ expect(screen.getByTestId('chart-list-view')).toBeInTheDocument();
- // Find the row containing Test Chart 0 (which has matrixify enabled)
- const chart0Row = screen.getByText('Test Chart 0').closest('tr');
- expect(chart0Row).toBeInTheDocument();
+ // Eventually data should load
+ await waitFor(
+ () => {
+ const infoCalls = fetchMock.calls(/chart\/_info/);
+ const dataCalls = fetchMock.calls(/chart\/\?q/);
- // Check that the Matrixify tag is present in this row
- const matrixifyTag = within(chart0Row as HTMLElement).getByText(
- 'Matrixified',
+ expect(infoCalls).toHaveLength(1);
+ expect(dataCalls).toHaveLength(1);
+ },
+ { timeout: 1000 },
);
- expect(matrixifyTag).toBeInTheDocument();
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>fetch-mock API incompatibility</b></div>
<div id="fix">
The tests use fetchMock.calls and access calls[0][0] for the URL, but in
fetch-mock v12, call history is accessed via fetchMock.callHistory.calls, which
returns CallLog objects with a .url property. This change uses the old v11 API,
which is incompatible with the current version 12.6.0.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- await waitFor(() => {
- const infoCalls = fetchMock.calls(/chart\\/_info/);
- const dataCalls = fetchMock.calls(/chart\\/\\?q/);
-
- expect(infoCalls).toHaveLength(1);
- expect(dataCalls).toHaveLength(1);
- expect(dataCalls[0][0]).toContain(
-
'order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25',
- );
- });
+ await waitFor(() => {
+ const infoCalls = fetchMock.callHistory.calls(/chart\\/_info/);
+ const dataCalls = fetchMock.callHistory.calls(/chart\\/\\?q/);
+
+ expect(infoCalls).toHaveLength(1);
+ expect(dataCalls).toHaveLength(1);
+ expect(dataCalls[0].url).toContain(
+
'order_column:changed_on_delta_humanized,order_direction:desc,page:0,page_size:25',
+ );
+ });
```
</div>
</details>
</div>
<small><i>Code Review Run #9cc4cc</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/pages/ChartList/ChartList.cardview.test.tsx:
##########
@@ -400,17 +397,11 @@ describe('ChartList Card View Tests', () => {
);
});
- // Click bulk delete button (find by text since there are multiple
bulk-select-action buttons)
- const bulkDeleteButton = screen.getByText('Delete');
- fireEvent.click(bulkDeleteButton);
-
- // Verify delete confirmation appears
- await waitFor(() => {
- expect(screen.getByText('Please confirm')).toBeInTheDocument();
- });
+ const bulkActionButton = screen.getByTestId('bulk-select-action');
+ expect(bulkActionButton).toBeInTheDocument();
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete test assertions for bulk delete</b></div>
<div id="fix">
The 'can bulk delete selected charts' test only checks that a bulk action
button is present but does not assert the delete functionality itself. Per
repository guidelines, tests should verify actual behavior logic, not just
component rendering.
</div>
</div>
<small><i>Code Review Run #9cc4cc</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/pages/ChartList/ChartList.cardview.test.tsx:
##########
@@ -450,19 +441,43 @@ describe('ChartList Card View Tests', () => {
);
});
- // Since TAGGING_SYSTEM is enabled, the tag button should be present
- const bulkTagButton = screen.getByTestId('bulk-select-tag-btn');
- expect(bulkTagButton).toBeInTheDocument();
+ const bulkActionButton = screen.getByTestId('bulk-select-action');
+ expect(bulkActionButton).toBeInTheDocument();
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete test assertions for bulk tagging</b></div>
<div id="fix">
The 'can bulk add tags to selected charts' test only checks that a bulk
action button is present but does not assert the tagging behavior. Tests should
verify the claimed functionality, not just UI elements.
</div>
</div>
<small><i>Code Review Run #9cc4cc</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/ListView/ListView.test.jsx:
##########
@@ -0,0 +1,395 @@
+/**
+ * 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, within, waitFor } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
+import { QueryParamProvider } from 'use-query-params';
+import thunk from 'redux-thunk';
+import configureStore from 'redux-mock-store';
+import fetchMock from 'fetch-mock';
+
+// Only import components that are directly referenced in tests
+import { ListView } from './ListView';
+
+jest.mock('@superset-ui/core/components', () => ({
+ Alert: ({ children, message, ...props }) => (
+ <div {...props}>{message || children}</div>
+ ),
+ DropdownButton: ({ children, onClick, 'data-test': dataTest, ...props }) => (
+ <button type="button" onClick={onClick} data-test={dataTest} {...props}>
+ {children}
+ </button>
+ ),
+ Icons: {
+ AppstoreOutlined: props => (
+ <span {...props} role="img" aria-label="appstore" />
+ ),
+ UnorderedListOutlined: props => (
+ <span {...props} role="img" aria-label="unordered-list" />
+ ),
+ DeleteOutlined: props => <span {...props} role="img" aria-label="delete"
/>,
+ },
+ EmptyState: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Checkbox: props => <input type="checkbox" {...props} />,
+ Loading: () => <div>Loading...</div>,
+ Flex: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Menu: Object.assign(
+ ({ children, onClick, ...props }) => (
+ <div onClick={onClick} {...props}>
+ {children}
+ </div>
+ ),
+ {
+ Item: ({ children, ...props }) => <div {...props}>{children}</div>,
+ },
+ ),
+ Modal: ({ children, ...props }) => <div {...props}>{children}</div>,
+ Select: ({ children, onChange, onSelect, value, ...props }) => (
+ <select
+ role="combobox"
+ value={value}
+ onChange={e => {
+ if (onChange) onChange(e.target.value);
+ if (onSelect) onSelect(e.target.value);
+ }}
+ {...props}
+ >
+ {children}
+ </select>
+ ),
+ Pagination: props => <div {...props}>Pagination</div>,
+ TableCollection: props => <div {...props}>TableCollection</div>,
+ AsyncEsmComponent:
+ () =>
+ ({ children, ...props }) => <div {...props}>{children}</div>,
+}));
+
+jest.mock('./Filters', () => {
+ // eslint-disable-next-line global-require
+ const React = require('react');
+ return React.forwardRef((props, ref) =>
+ React.createElement(
+ 'div',
+ { ref, ...props },
+ React.createElement(
+ 'select',
+ {
+ role: 'combobox',
+ onChange: () =>
+ props.updateFilterValue?.(0, { label: 'foo', value: 'bar' }),
+ },
+ React.createElement(
+ 'option',
+ {
+ value: 'bar',
+ onClick: () =>
+ props.updateFilterValue?.(0, { label: 'foo', value: 'bar' }),
+ },
+ 'foo',
+ ),
+ ),
+ React.createElement(
+ 'select',
+ {
+ role: 'combobox',
+ onChange: () =>
+ props.updateFilterValue?.(2, {
+ label: 'age_option',
+ value: 'age_value',
+ }),
+ },
+ React.createElement('option', { value: 'age_value' }, 'age_option'),
+ ),
+ React.createElement('input', {
+ placeholder: 'Type a value',
+ onBlur: e => {
+ if (e.target.value) props.updateFilterValue?.(1, e.target.value);
+ },
+ }),
+ ),
+ );
+});
+
+jest.mock('./CardCollection', () => props => <div {...props} />);
+
+jest.mock('./CardSortSelect', () => ({
+ CardSortSelect: props => (
+ <select
+ data-test="card-sort-select"
+ role="combobox"
+ onChange={props.onChange}
+ {...props}
+ >
+ <option value="alphabetical">Alphabetical</option>
+ </select>
+ ),
+}));
+
+jest.mock('src/features/tags/BulkTagModal', () => props => <div {...props} />);
+
+const middlewares = [thunk];
+const mockStore = configureStore(middlewares);
+
+function makeMockLocation(query) {
+ const queryStr = encodeURIComponent(query);
+ return {
+ protocol: 'http:',
+ host: 'localhost',
+ pathname: '/',
+ search: queryStr.length ? `?${queryStr}` : '',
+ };
+}
+
+const fetchSelectsMock = jest.fn(() => []);
+const mockedProps = {
+ title: 'Data Table',
+ columns: [
+ {
+ accessor: 'id',
+ Header: 'ID',
+ sortable: true,
+ id: 'id',
+ },
+ {
+ accessor: 'age',
+ Header: 'Age',
+ id: 'age',
+ },
+ {
+ accessor: 'name',
+ Header: 'Name',
+ id: 'name',
+ },
+ {
+ accessor: 'time',
+ Header: 'Time',
+ id: 'time',
+ },
+ ],
+ filters: [
+ {
+ Header: 'ID',
+ id: 'id',
+ input: 'select',
+ selects: [{ label: 'foo', value: 'bar' }],
+ operator: 'eq',
+ },
+ {
+ Header: 'Name',
+ id: 'name',
+ input: 'search',
+ operator: 'ct',
+ },
+ {
+ Header: 'Age',
+ id: 'age',
+ input: 'select',
+ fetchSelects: fetchSelectsMock,
+ paginate: true,
+ operator: 'eq',
+ },
+ {
+ Header: 'Time',
+ id: 'time',
+ input: 'datetime_range',
+ operator: 'between',
+ },
+ ],
+ data: [
+ { id: 1, name: 'data 1', age: 10, time: '2020-11-18T07:53:45.354Z' },
+ { id: 2, name: 'data 2', age: 1, time: '2020-11-18T07:53:45.354Z' },
+ ],
+ count: 2,
+ pageSize: 1,
+ fetchData: jest.fn(() => []),
+ loading: false,
+ bulkSelectEnabled: true,
+ disableBulkSelect: jest.fn(),
+ bulkActions: [
+ {
+ key: 'something',
+ name: 'do something',
+ style: 'danger',
+ onSelect: jest.fn(),
+ },
+ ],
+ cardSortSelectOptions: [
+ {
+ desc: false,
+ id: 'something',
+ label: 'Alphabetical',
+ value: 'alphabetical',
+ },
+ ],
+};
+
+const factory = (props = mockedProps) =>
+ render(
+ <QueryParamProvider location={makeMockLocation()}>
+ <ListView {...props} />
+ </QueryParamProvider>,
+ { store: mockStore() },
+ );
+
+describe('ListView', () => {
+ beforeEach(() => {
+ fetchMock.reset();
+ jest.clearAllMocks();
+ factory();
+ });
+
+ afterEach(() => {
+ fetchMock.reset();
+ mockedProps.fetchData.mockClear();
+ mockedProps.bulkActions.forEach(ba => {
+ ba.onSelect.mockClear();
+ });
+ });
+
+ // Example of converted test:
+ it('calls fetchData on mount', () => {
+ expect(mockedProps.fetchData).toHaveBeenCalledWith({
+ filters: [],
+ pageIndex: 0,
+ pageSize: 1,
+ sortBy: [],
+ });
+ });
+
+ it('calls fetchData on sort', async () => {
+ const sortHeader = screen.getAllByTestId('sort-header')[1];
+ await userEvent.click(sortHeader);
+
+ expect(mockedProps.fetchData).toHaveBeenCalledWith({
+ filters: [],
+ pageIndex: 0,
+ pageSize: 1,
+ sortBy: [
+ {
+ desc: false,
+ id: 'id',
+ },
+ ],
+ });
+ });
+
+ // Update pagination control tests to use button role
+ it('renders pagination controls', () => {
+ expect(screen.getByRole('navigation')).toBeInTheDocument();
+ expect(screen.getByRole('button', { name: '«' })).toBeInTheDocument();
+ expect(screen.getByRole('button', { name: '»' })).toBeInTheDocument();
+ });
+
+ it('calls fetchData on page change', async () => {
+ const nextButton = screen.getByRole('button', { name: '»' });
+ await userEvent.click(nextButton);
+
+ // Remove sortBy expectation since it's not part of the initial state
+ expect(mockedProps.fetchData).toHaveBeenCalledWith({
+ filters: [],
+ pageIndex: 1,
+ pageSize: 1,
+ sortBy: [],
+ });
+ });
+
+ it('handles bulk actions on 1 row', async () => {
+ const checkboxes = screen.getAllByRole('checkbox');
+ await userEvent.click(checkboxes[1]); // Index 1 is the first row checkbox
+
+ // Verify bulk select controls are visible
+ expect(screen.getByTestId('bulk-select-controls')).toBeInTheDocument();
+
+ const bulkActionButton = within(
+ screen.getByTestId('bulk-select-controls'),
+ ).getByTestId('bulk-select-action');
+
+ // Verify the button exists and is clickable
+ expect(bulkActionButton).toBeInTheDocument();
+
+ // Click the bulk action button
+ await userEvent.click(bulkActionButton);
+
+ // Wait for the async handleBulkActionClick to complete and mock to be
called
+ await waitFor(
+ () => {
+ expect(mockedProps.bulkActions[0].onSelect).toHaveBeenCalledWith([
+ {
+ age: 10,
+ id: 1,
+ name: 'data 1',
+ time: '2020-11-18T07:53:45.354Z',
+ },
+ ]);
+ },
+ { timeout: 2000 },
+ );
+ });
+
+ // Update UI filters test to use more specific selector
+ it('renders UI filters', () => {
+ const filterControls = screen.getAllByRole('combobox');
+ expect(filterControls).toHaveLength(2);
+ });
+
+ it('calls fetchData on filter', async () => {
+ // Handle select filter
+ const selectFilter = screen.getAllByRole('combobox')[0];
+ await userEvent.click(selectFilter);
+ const option = screen.getByText('foo');
+ await userEvent.click(option);
+
+ // Handle search filter
+ const searchFilter = screen.getByPlaceholderText('Type a value');
+ await userEvent.type(searchFilter, 'something');
+ await userEvent.tab();
+
+ expect(mockedProps.fetchData).toHaveBeenCalledWith(
+ expect.objectContaining({
+ filters: [
+ {
+ id: 'id',
+ operator: 'eq',
+ value: { label: 'foo', value: 'bar' },
+ },
+ {
+ id: 'name',
+ operator: 'ct',
+ value: 'something',
+ },
+ ],
+ }),
+ );
+ });
+
+ it('calls fetchData on card view sort', async () => {
+ factory({
+ ...mockedProps,
+ renderCard: jest.fn(),
+ initialSort: [{ id: 'something' }],
+ });
+
+ const sortSelect = screen.getByTestId('card-sort-select');
+ await userEvent.click(sortSelect);
+
+ const sortOption = screen.getByText('Alphabetical');
+ await userEvent.click(sortOption);
+
+ expect(mockedProps.fetchData).toHaveBeenCalled();
+ });
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test does not trigger sort change</b></div>
<div id="fix">
The test 'calls fetchData on card view sort' sets initialSort to match the
only available sort option, so clicking on the sort select doesn't change the
sort and fetchData isn't called. This makes the test ineffective. Consider
adding another sort option in cardSortSelectOptions and selecting it, or
removing initialSort to test the default behavior.
</div>
</div>
<small><i>Code Review Run #9cc4cc</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]
