bito-code-review[bot] commented on code in PR #44848: URL: https://github.com/apache/superset/pull/44848#discussion_r4150805795
########## superset-frontend/src/explore/components/ChartPills.test.tsx: ########## @@ -0,0 +1,166 @@ +/** + * 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 { QueryData, VizType } from '@superset-ui/core'; +import { render, screen, userEvent } from 'spec/helpers/testing-library'; +import { ChartPills, ChartPillsProps } from './ChartPills'; + +const ROW_LIMIT = 10000; + +const renderPills = (props: Partial<ChartPillsProps> = {}) => { + const refreshCachedQuery = jest.fn(); + render( + <ChartPills + chartStatus="success" + chartUpdateStartTime={0} + rowLimit={ROW_LIMIT} + refreshCachedQuery={refreshCachedQuery} + {...props} + />, + ); + return { refreshCachedQuery }; +}; + +// The table viz reports its total row count in a second query when the +// dataset is paginated on the server. +const paginatedTableResponses = ( + pageRowCount: number, + totalRowCount: number, +): QueryData[] => [ + { rowcount: pageRowCount, data: [] }, + { data: [{ rowcount: totalRowCount }] }, +]; + +test.each([VizType.Table, VizType.TableAgGrid])( + 'reads the row count of a server-paginated %s from the second query', + vizType => { + renderPills({ + queriesResponse: paginatedTableResponses(25, 250), + formData: { viz_type: vizType, server_pagination: true }, + }); + + expect(screen.getByText('250 rows')).toBeInTheDocument(); + expect(screen.queryByText('25 rows')).not.toBeInTheDocument(); + }, +); + +test('reads the row count from the first query for a table with a single query', () => { + renderPills({ + queriesResponse: [{ rowcount: 25, data: [] }], + formData: { viz_type: VizType.Table, server_pagination: true }, + }); + + expect(screen.getByText('25 rows')).toBeInTheDocument(); +}); + +test('reads the row count from the first query for non-table charts even with a second query', () => { + renderPills({ + queriesResponse: paginatedTableResponses(25, 250), + formData: { viz_type: VizType.Histogram }, + }); + + expect(screen.getByText('25 rows')).toBeInTheDocument(); + expect(screen.queryByText('250 rows')).not.toBeInTheDocument(); +}); + +test('prefers sql_rowcount over rowcount', () => { + renderPills({ + queriesResponse: [{ sql_rowcount: 40, rowcount: 7, data: [] }], + formData: { viz_type: VizType.Histogram }, + }); + + expect(screen.getByText('40 rows')).toBeInTheDocument(); + expect(screen.queryByText('7 rows')).not.toBeInTheDocument(); +}); + +test('falls back to rowcount when sql_rowcount is absent', () => { + renderPills({ + queriesResponse: [{ rowcount: 7, data: [] }], + formData: { viz_type: VizType.Histogram }, + }); + + expect(screen.getByText('7 rows')).toBeInTheDocument(); +}); + +test('hides the row count but keeps the timer when hideRowCount is set', () => { + renderPills({ + queriesResponse: [{ rowcount: 25, data: [] }], + hideRowCount: true, + }); + + expect(screen.queryByText('25 rows')).not.toBeInTheDocument(); + expect(screen.getByRole('timer')).toBeInTheDocument(); +}); + +test('does not show the row count without a query response', () => { + renderPills({ queriesResponse: [] }); + + expect(screen.queryByText(/rows?$/)).not.toBeInTheDocument(); +}); + +test('shows the cached label for a cached response and refreshes on click', async () => { + const { refreshCachedQuery } = renderPills({ + queriesResponse: [ + { + rowcount: 25, + is_cached: true, + cached_dttm: '2024-01-01T00:00:00', + data: [], + }, + ], Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicated cached fixture</b></div> <div id="fix"> The cached-response literal is duplicated verbatim at lines 143-150. Extract a `cachedResponse()` helper beside `paginatedTableResponses` so the two fixtures cannot silently diverge when the cached payload changes. </div> </div> <small><i>Code Review Run #d837aa</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/explore/components/ChartPills.test.tsx: ########## @@ -0,0 +1,166 @@ +/** + * 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 { QueryData, VizType } from '@superset-ui/core'; +import { render, screen, userEvent } from 'spec/helpers/testing-library'; +import { ChartPills, ChartPillsProps } from './ChartPills'; + +const ROW_LIMIT = 10000; + +const renderPills = (props: Partial<ChartPillsProps> = {}) => { + const refreshCachedQuery = jest.fn(); + render( + <ChartPills + chartStatus="success" + chartUpdateStartTime={0} + rowLimit={ROW_LIMIT} + refreshCachedQuery={refreshCachedQuery} + {...props} + />, + ); + return { refreshCachedQuery }; +}; + +// The table viz reports its total row count in a second query when the +// dataset is paginated on the server. +const paginatedTableResponses = ( + pageRowCount: number, + totalRowCount: number, +): QueryData[] => [ + { rowcount: pageRowCount, data: [] }, + { data: [{ rowcount: totalRowCount }] }, +]; + +test.each([VizType.Table, VizType.TableAgGrid])( + 'reads the row count of a server-paginated %s from the second query', + vizType => { + renderPills({ + queriesResponse: paginatedTableResponses(25, 250), + formData: { viz_type: vizType, server_pagination: true }, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inert server_pagination fixture</b></div> <div id="fix"> `ChartPills` never reads `formData.server_pagination` (only `viz_type`; see `ChartPills.tsx`), so this flag is inert and the test name implies a causality the component does not implement - the second query is used whenever `queriesResponse.length > 1`. Drop the flag (also at line 65) or implement the gate in `ChartPills`. </div> </div> <small><i>Code Review Run #d837aa</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/explore/components/ChartPills.test.tsx: ########## @@ -0,0 +1,166 @@ +/** + * 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 { QueryData, VizType } from '@superset-ui/core'; +import { render, screen, userEvent } from 'spec/helpers/testing-library'; +import { ChartPills, ChartPillsProps } from './ChartPills'; + +const ROW_LIMIT = 10000; + +const renderPills = (props: Partial<ChartPillsProps> = {}) => { + const refreshCachedQuery = jest.fn(); + render( + <ChartPills + chartStatus="success" + chartUpdateStartTime={0} + rowLimit={ROW_LIMIT} + refreshCachedQuery={refreshCachedQuery} + {...props} + />, + ); + return { refreshCachedQuery }; +}; + +// The table viz reports its total row count in a second query when the +// dataset is paginated on the server. +const paginatedTableResponses = ( + pageRowCount: number, + totalRowCount: number, +): QueryData[] => [ + { rowcount: pageRowCount, data: [] }, + { data: [{ rowcount: totalRowCount }] }, +]; + +test.each([VizType.Table, VizType.TableAgGrid])( + 'reads the row count of a server-paginated %s from the second query', + vizType => { + renderPills({ + queriesResponse: paginatedTableResponses(25, 250), + formData: { viz_type: vizType, server_pagination: true }, + }); + + expect(screen.getByText('250 rows')).toBeInTheDocument(); + expect(screen.queryByText('25 rows')).not.toBeInTheDocument(); + }, +); + +test('reads the row count from the first query for a table with a single query', () => { + renderPills({ + queriesResponse: [{ rowcount: 25, data: [] }], + formData: { viz_type: VizType.Table, server_pagination: true }, + }); + + expect(screen.getByText('25 rows')).toBeInTheDocument(); +}); + +test('reads the row count from the first query for non-table charts even with a second query', () => { + renderPills({ + queriesResponse: paginatedTableResponses(25, 250), + formData: { viz_type: VizType.Histogram }, + }); + + expect(screen.getByText('25 rows')).toBeInTheDocument(); + expect(screen.queryByText('250 rows')).not.toBeInTheDocument(); +}); + +test('prefers sql_rowcount over rowcount', () => { + renderPills({ + queriesResponse: [{ sql_rowcount: 40, rowcount: 7, data: [] }], + formData: { viz_type: VizType.Histogram }, + }); + + expect(screen.getByText('40 rows')).toBeInTheDocument(); + expect(screen.queryByText('7 rows')).not.toBeInTheDocument(); +}); + +test('falls back to rowcount when sql_rowcount is absent', () => { + renderPills({ + queriesResponse: [{ rowcount: 7, data: [] }], + formData: { viz_type: VizType.Histogram }, + }); + + expect(screen.getByText('7 rows')).toBeInTheDocument(); +}); + +test('hides the row count but keeps the timer when hideRowCount is set', () => { + renderPills({ + queriesResponse: [{ rowcount: 25, data: [] }], + hideRowCount: true, + }); + + expect(screen.queryByText('25 rows')).not.toBeInTheDocument(); + expect(screen.getByRole('timer')).toBeInTheDocument(); +}); + +test('does not show the row count without a query response', () => { + renderPills({ queriesResponse: [] }); + + expect(screen.queryByText(/rows?$/)).not.toBeInTheDocument(); +}); + +test('shows the cached label for a cached response and refreshes on click', async () => { + const { refreshCachedQuery } = renderPills({ + queriesResponse: [ + { + rowcount: 25, + is_cached: true, + cached_dttm: '2024-01-01T00:00:00', + data: [], + }, + ], + }); + + expect(refreshCachedQuery).not.toHaveBeenCalled(); + await userEvent.click(screen.getByText('Cached')); + + expect(refreshCachedQuery).toHaveBeenCalledTimes(1); +}); + +test('does not show the cached label for an uncached response', () => { + renderPills({ queriesResponse: [{ rowcount: 25, data: [] }] }); + + expect(screen.queryByText('Cached')).not.toBeInTheDocument(); +}); + +test('shows neither the row count nor the cached label while loading', () => { + renderPills({ + chartStatus: 'loading', + queriesResponse: [ + { + rowcount: 25, + is_cached: true, + cached_dttm: '2024-01-01T00:00:00', + data: [], + }, + ], + }); + + expect(screen.queryByText('25 rows')).not.toBeInTheDocument(); + expect(screen.queryByText('Cached')).not.toBeInTheDocument(); + expect(screen.getByRole('timer')).toBeInTheDocument(); +}); + +test('shows the elapsed time between the update start and end', () => { + renderPills({ + chartUpdateStartTime: 1000, + chartUpdateEndTime: 3500, + queriesResponse: [{ rowcount: 25, data: [] }], + }); + + expect(screen.getByRole('timer')).toHaveTextContent('00:00:02.50'); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Weak timer assertion</b></div> <div id="fix"> `Timer` renders `fDuration(startTime, endTime)` with format `HH:mm:ss.SSS` (`packages/superset-ui-core/src/utils/dates.ts`), so the label shows '00:00:02.500'. `toHaveTextContent` matches substrings, so expecting '00:00:02.50' passes but would not catch a regression to two-digit milliseconds. Pin the full rendered value. </div> </div> <small><i>Code Review Run #d837aa</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]
