bito-code-review[bot] commented on code in PR #37712: URL: https://github.com/apache/superset/pull/37712#discussion_r2805326523
########## superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformPropsWithDimensions.test.ts: ########## @@ -0,0 +1,510 @@ +/** + * 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. + */ + +/** + * Tests for X-Axis Sort By with Dimensions in Bar Charts + * + * These tests verify the fix for issue #34352, ensuring: + * 1. X-Axis Sort By actually works in multi-series scenarios + * 2. Categories are ordered correctly based on sort configuration + * 3. The mapXAxisSortToSeriesType function properly maps sort values + */ + +import { ChartProps, SqlaFormData } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/ui'; +import { EchartsTimeseriesChartProps } from '../../src/types'; +import transformProps from '../../src/Timeseries/transformProps'; +import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants'; +import { EchartsTimeseriesSeriesType } from '../../src/Timeseries/types'; + +describe('Bar Chart X-Axis Sort By with Dimensions', () => { + const baseFormData: SqlaFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'bnbColors', + datasource: '3__table', + granularity_sqla: '__timestamp', + viz_type: 'echarts_timeseries_bar', + seriesType: EchartsTimeseriesSeriesType.Bar, + orientation: 'vertical', + }; + + describe('Multi-series sorting (with groupby)', () => { + const createMultiSeriesData = () => [ + { + data: [ + { + genre: 'Action', + platform: 'Console', + na_sales: 150, + __timestamp: 1609459200000, + }, + { + genre: 'Sports', + platform: 'Console', + na_sales: 200, + __timestamp: 1609459200000, + }, + { + genre: 'RPG', + platform: 'Console', + na_sales: 80, + __timestamp: 1609459200000, + }, + { + genre: 'Action', + platform: 'PC', + na_sales: 120, + __timestamp: 1609459200000, + }, + { + genre: 'Sports', + platform: 'PC', + na_sales: 180, + __timestamp: 1609459200000, + }, + { + genre: 'RPG', + platform: 'PC', + na_sales: 90, + __timestamp: 1609459200000, + }, + ], + colnames: ['genre', 'platform', 'na_sales', '__timestamp'], + coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + it('should handle xAxisSort with dimensions present', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], // This creates multi-series + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: false, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + + // Verify chart renders successfully + expect(transformedProps.echartOptions).toBeDefined(); + expect(transformedProps.echartOptions.series).toBeDefined(); + + const series = transformedProps.echartOptions.series as any[]; + expect(series.length).toBeGreaterThan(0); + + // Each series should have data + series.forEach(s => { + expect(s.data).toBeDefined(); + expect(Array.isArray(s.data)).toBe(true); + }); + }); + + it('should sort categories by sum when xAxisSort=sum with dimension', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: false, // descending + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + + // Expected order by sum (descending): + // Sports: 200 + 180 = 380 + // Action: 150 + 120 = 270 + // RPG: 80 + 90 = 170 + + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + expect(xAxisData.length).toBe(3); + // Verify the order is correct + expect(xAxisData[0]).toBe('Sports'); + expect(xAxisData[1]).toBe('Action'); + expect(xAxisData[2]).toBe('RPG'); + } + }); + + it('should sort categories alphabetically when xAxisSort=name', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'name', + x_axis_sort_asc: true, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + // Alphabetical order: Action, RPG, Sports + expect(xAxisData[0]).toBe('Action'); + expect(xAxisData[1]).toBe('RPG'); + expect(xAxisData[2]).toBe('Sports'); + } + }); + + it('should apply ascending sort correctly', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: true, // ascending + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + // Ascending order by sum: RPG (170), Action (270), Sports (380) + expect(xAxisData[0]).toBe('RPG'); + expect(xAxisData[1]).toBe('Action'); + expect(xAxisData[2]).toBe('Sports'); + } + }); + + it('should sort by average when xAxisSort=avg', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'avg', + x_axis_sort_asc: false, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + expect(xAxisData.length).toBe(3); + // Average values: + // Sports: (200 + 180) / 2 = 190 + // Action: (150 + 120) / 2 = 135 + // RPG: (80 + 90) / 2 = 85 + expect(xAxisData[0]).toBe('Sports'); + expect(xAxisData[1]).toBe('Action'); + expect(xAxisData[2]).toBe('RPG'); + } + }); + }); + + describe('Day name sorting example', () => { + // This tests the specific use case mentioned in the issue: + // Sorting day names chronologically using a numeric helper column + const createDayData = () => [ + { + data: [ + { + day_name: 'Monday', + day_number: 1, + category: 'A', + sales: 100, + __timestamp: 1609459200000, + }, + { + day_name: 'Wednesday', + day_number: 3, + category: 'A', + sales: 150, + __timestamp: 1609459200000, + }, + { + day_name: 'Friday', + day_number: 5, + category: 'A', + sales: 200, + __timestamp: 1609459200000, + }, + { + day_name: 'Monday', + day_number: 1, + category: 'B', + sales: 80, + __timestamp: 1609459200000, + }, + { + day_name: 'Wednesday', + day_number: 3, + category: 'B', + sales: 120, + __timestamp: 1609459200000, + }, + { + day_name: 'Friday', + day_number: 5, + category: 'B', + sales: 90, + __timestamp: 1609459200000, + }, + ], + colnames: [ + 'day_name', + 'day_number', + 'category', + 'sales', + '__timestamp', + ], + coltypes: ['STRING', 'BIGINT', 'STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + it('should sort day names by numeric column when dimension is present', () => { + const formData = { + ...baseFormData, + groupby: ['category'], // dimension creates multi-series + metrics: ['sales'], + x_axis: 'day_name', + x_axis_sort: 'day_number', // sort by the numeric helper + x_axis_sort_asc: true, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createDayData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + // Should be in chronological order (by day_number) + // NOT alphabetical order (which would be Friday, Monday, Wednesday) + expect(xAxisData[0]).toBe('Monday'); + expect(xAxisData[1]).toBe('Wednesday'); + expect(xAxisData[2]).toBe('Friday'); + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unsupported x_axis_sort value</b></div> <div id="fix"> The test uses x_axis_sort: 'day_number' expecting sorting by a numeric column, but mapXAxisSortToSeriesType only supports predefined keywords ('sum', 'avg', 'name', etc.) and returns undefined for 'day_number', preventing sorting. </div> </div> <small><i>Code Review Run #d84cb8</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/plugins/plugin-chart-echarts/test/Timeseries/transformPropsWithDimensions.test.ts: ########## @@ -0,0 +1,510 @@ +/** + * 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. + */ + +/** + * Tests for X-Axis Sort By with Dimensions in Bar Charts + * + * These tests verify the fix for issue #34352, ensuring: + * 1. X-Axis Sort By actually works in multi-series scenarios + * 2. Categories are ordered correctly based on sort configuration + * 3. The mapXAxisSortToSeriesType function properly maps sort values + */ + +import { ChartProps, SqlaFormData } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/ui'; +import { EchartsTimeseriesChartProps } from '../../src/types'; +import transformProps from '../../src/Timeseries/transformProps'; +import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants'; +import { EchartsTimeseriesSeriesType } from '../../src/Timeseries/types'; + +describe('Bar Chart X-Axis Sort By with Dimensions', () => { + const baseFormData: SqlaFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'bnbColors', + datasource: '3__table', + granularity_sqla: '__timestamp', + viz_type: 'echarts_timeseries_bar', + seriesType: EchartsTimeseriesSeriesType.Bar, + orientation: 'vertical', + }; + + describe('Multi-series sorting (with groupby)', () => { + const createMultiSeriesData = () => [ + { + data: [ + { + genre: 'Action', + platform: 'Console', + na_sales: 150, + __timestamp: 1609459200000, + }, + { + genre: 'Sports', + platform: 'Console', + na_sales: 200, + __timestamp: 1609459200000, + }, + { + genre: 'RPG', + platform: 'Console', + na_sales: 80, + __timestamp: 1609459200000, + }, + { + genre: 'Action', + platform: 'PC', + na_sales: 120, + __timestamp: 1609459200000, + }, + { + genre: 'Sports', + platform: 'PC', + na_sales: 180, + __timestamp: 1609459200000, + }, + { + genre: 'RPG', + platform: 'PC', + na_sales: 90, + __timestamp: 1609459200000, + }, + ], + colnames: ['genre', 'platform', 'na_sales', '__timestamp'], + coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + it('should handle xAxisSort with dimensions present', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], // This creates multi-series + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: false, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + + // Verify chart renders successfully + expect(transformedProps.echartOptions).toBeDefined(); + expect(transformedProps.echartOptions.series).toBeDefined(); + + const series = transformedProps.echartOptions.series as any[]; + expect(series.length).toBeGreaterThan(0); + + // Each series should have data + series.forEach(s => { + expect(s.data).toBeDefined(); + expect(Array.isArray(s.data)).toBe(true); + }); + }); + + it('should sort categories by sum when xAxisSort=sum with dimension', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: false, // descending + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + + // Expected order by sum (descending): + // Sports: 200 + 180 = 380 + // Action: 150 + 120 = 270 + // RPG: 80 + 90 = 170 + + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + expect(xAxisData.length).toBe(3); + // Verify the order is correct + expect(xAxisData[0]).toBe('Sports'); + expect(xAxisData[1]).toBe('Action'); + expect(xAxisData[2]).toBe('RPG'); + } + }); + + it('should sort categories alphabetically when xAxisSort=name', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'name', + x_axis_sort_asc: true, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + // Alphabetical order: Action, RPG, Sports + expect(xAxisData[0]).toBe('Action'); + expect(xAxisData[1]).toBe('RPG'); + expect(xAxisData[2]).toBe('Sports'); + } + }); + + it('should apply ascending sort correctly', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: true, // ascending + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + // Ascending order by sum: RPG (170), Action (270), Sports (380) + expect(xAxisData[0]).toBe('RPG'); + expect(xAxisData[1]).toBe('Action'); + expect(xAxisData[2]).toBe('Sports'); + } + }); + + it('should sort by average when xAxisSort=avg', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'avg', + x_axis_sort_asc: false, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + expect(xAxisData.length).toBe(3); + // Average values: + // Sports: (200 + 180) / 2 = 190 + // Action: (150 + 120) / 2 = 135 + // RPG: (80 + 90) / 2 = 85 + expect(xAxisData[0]).toBe('Sports'); + expect(xAxisData[1]).toBe('Action'); + expect(xAxisData[2]).toBe('RPG'); + } + }); + }); + + describe('Day name sorting example', () => { + // This tests the specific use case mentioned in the issue: + // Sorting day names chronologically using a numeric helper column + const createDayData = () => [ + { + data: [ + { + day_name: 'Monday', + day_number: 1, + category: 'A', + sales: 100, + __timestamp: 1609459200000, + }, + { + day_name: 'Wednesday', + day_number: 3, + category: 'A', + sales: 150, + __timestamp: 1609459200000, + }, + { + day_name: 'Friday', + day_number: 5, + category: 'A', + sales: 200, + __timestamp: 1609459200000, + }, + { + day_name: 'Monday', + day_number: 1, + category: 'B', + sales: 80, + __timestamp: 1609459200000, + }, + { + day_name: 'Wednesday', + day_number: 3, + category: 'B', + sales: 120, + __timestamp: 1609459200000, + }, + { + day_name: 'Friday', + day_number: 5, + category: 'B', + sales: 90, + __timestamp: 1609459200000, + }, + ], + colnames: [ + 'day_name', + 'day_number', + 'category', + 'sales', + '__timestamp', + ], + coltypes: ['STRING', 'BIGINT', 'STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + it('should sort day names by numeric column when dimension is present', () => { + const formData = { + ...baseFormData, + groupby: ['category'], // dimension creates multi-series + metrics: ['sales'], + x_axis: 'day_name', + x_axis_sort: 'day_number', // sort by the numeric helper + x_axis_sort_asc: true, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createDayData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + // Should be in chronological order (by day_number) + // NOT alphabetical order (which would be Friday, Monday, Wednesday) + expect(xAxisData[0]).toBe('Monday'); + expect(xAxisData[1]).toBe('Wednesday'); + expect(xAxisData[2]).toBe('Friday'); + } + }); + }); + + describe('Edge cases', () => { + it('should handle single-series charts (backward compatibility)', () => { + const formData = { + ...baseFormData, + groupby: [], // No dimension + metrics: ['sales'], + x_axis: 'category', + x_axis_sort: undefined, + }; + + const testData = [ + { + data: [ + { category: 'B', sales: 100, __timestamp: 1609459200000 }, + { category: 'A', sales: 150, __timestamp: 1609459200000 }, + ], + colnames: ['category', 'sales', '__timestamp'], + coltypes: ['STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: testData, + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + // Should not throw + expect(() => transformProps(chartProps)).not.toThrow(); + + const transformedProps = transformProps(chartProps); + expect(transformedProps.echartOptions.series).toBeDefined(); + }); + + it('should handle empty data gracefully', () => { + const formData = { + ...baseFormData, + groupby: ['category'], + metrics: ['sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + }; + + const emptyData = [ + { + data: [], + colnames: ['genre', 'category', 'sales', '__timestamp'], + coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: emptyData, + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + expect(() => transformProps(chartProps)).not.toThrow(); + }); + + it('should handle horizontal orientation', () => { + const formData = { + ...baseFormData, + orientation: 'horizontal' as const, + groupby: ['category'], + metrics: ['sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + }; + + const testData = [ + { + data: [ + { + genre: 'A', + category: 'X', + sales: 100, + __timestamp: 1609459200000, + }, + { + genre: 'B', + category: 'X', + sales: 150, + __timestamp: 1609459200000, + }, + ], + colnames: ['genre', 'category', 'sales', '__timestamp'], + coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: testData, + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + expect(() => transformProps(chartProps)).not.toThrow(); + }); + }); + + describe('Multiple metrics with dimensions', () => { + it('should handle sorting with multiple metrics and dimensions', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales', 'eu_sales'], + x_axis: 'genre', + x_axis_sort: 'avg', + x_axis_sort_asc: false, + }; + + const testData = [ + { + data: [ + { + genre: 'Action', + platform: 'Console', + na_sales: 150, + eu_sales: 100, + __timestamp: 1609459200000, + }, + { + genre: 'Sports', + platform: 'Console', + na_sales: 200, + eu_sales: 120, + __timestamp: 1609459200000, + }, + ], + colnames: [ + 'genre', + 'platform', + 'na_sales', + 'eu_sales', + '__timestamp', + ], + coltypes: ['STRING', 'STRING', 'BIGINT', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: testData, + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + + expect(transformedProps.echartOptions.series).toBeDefined(); + const series = transformedProps.echartOptions.series as any[]; + + // Should have series for multiple metrics + expect(series.length).toBeGreaterThan(0); + }); + }); +}); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Weak test assertions</b></div> <div id="fix"> The added test cases primarily check that `transformProps` doesn't throw and that basic output exists, but they don't validate the actual business logic like correct series generation, data transformation, or sorting behavior. For instance, the multiple metrics test only confirms `series.length > 0` without checking if series correspond to each metric or contain expected data. This risks missing regressions in data processing. Consider adding assertions on `transformedProps.echartOptions.series` content, x-axis ordering, and metric-specific series to align with testing best practices. </div> </div> <small><i>Code Review Run #d84cb8</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/plugins/plugin-chart-echarts/test/Timeseries/transformPropsWithDimensions.test.ts: ########## @@ -0,0 +1,510 @@ +/** + * 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. + */ + +/** + * Tests for X-Axis Sort By with Dimensions in Bar Charts + * + * These tests verify the fix for issue #34352, ensuring: + * 1. X-Axis Sort By actually works in multi-series scenarios + * 2. Categories are ordered correctly based on sort configuration + * 3. The mapXAxisSortToSeriesType function properly maps sort values + */ + +import { ChartProps, SqlaFormData } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/ui'; +import { EchartsTimeseriesChartProps } from '../../src/types'; +import transformProps from '../../src/Timeseries/transformProps'; +import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants'; +import { EchartsTimeseriesSeriesType } from '../../src/Timeseries/types'; + +describe('Bar Chart X-Axis Sort By with Dimensions', () => { + const baseFormData: SqlaFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'bnbColors', + datasource: '3__table', + granularity_sqla: '__timestamp', + viz_type: 'echarts_timeseries_bar', + seriesType: EchartsTimeseriesSeriesType.Bar, + orientation: 'vertical', + }; + + describe('Multi-series sorting (with groupby)', () => { + const createMultiSeriesData = () => [ + { + data: [ + { + genre: 'Action', + platform: 'Console', + na_sales: 150, + __timestamp: 1609459200000, + }, + { + genre: 'Sports', + platform: 'Console', + na_sales: 200, + __timestamp: 1609459200000, + }, + { + genre: 'RPG', + platform: 'Console', + na_sales: 80, + __timestamp: 1609459200000, + }, + { + genre: 'Action', + platform: 'PC', + na_sales: 120, + __timestamp: 1609459200000, + }, + { + genre: 'Sports', + platform: 'PC', + na_sales: 180, + __timestamp: 1609459200000, + }, + { + genre: 'RPG', + platform: 'PC', + na_sales: 90, + __timestamp: 1609459200000, + }, + ], + colnames: ['genre', 'platform', 'na_sales', '__timestamp'], + coltypes: ['STRING', 'STRING', 'BIGINT', 'TIMESTAMP'], + }, + ]; + + it('should handle xAxisSort with dimensions present', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], // This creates multi-series + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: false, + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + + // Verify chart renders successfully + expect(transformedProps.echartOptions).toBeDefined(); + expect(transformedProps.echartOptions.series).toBeDefined(); + + const series = transformedProps.echartOptions.series as any[]; + expect(series.length).toBeGreaterThan(0); + + // Each series should have data + series.forEach(s => { + expect(s.data).toBeDefined(); + expect(Array.isArray(s.data)).toBe(true); + }); + }); + + it('should sort categories by sum when xAxisSort=sum with dimension', () => { + const formData = { + ...baseFormData, + groupby: ['platform'], + metrics: ['na_sales'], + x_axis: 'genre', + x_axis_sort: 'sum', + x_axis_sort_asc: false, // descending + }; + + const chartProps = new ChartProps({ + width: 800, + height: 600, + formData, + queriesData: createMultiSeriesData(), + theme: supersetTheme, + }) as unknown as EchartsTimeseriesChartProps; + + const transformedProps = transformProps(chartProps); + + // Expected order by sum (descending): + // Sports: 200 + 180 = 380 + // Action: 150 + 120 = 270 + // RPG: 80 + 90 = 170 + + const xAxisData = (transformedProps.echartOptions.xAxis as any)?.data; + + if (xAxisData && Array.isArray(xAxisData)) { + expect(xAxisData.length).toBe(3); + // Verify the order is correct + expect(xAxisData[0]).toBe('Sports'); + expect(xAxisData[1]).toBe('Action'); + expect(xAxisData[2]).toBe('RPG'); + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Invalid xAxis.data assertion</b></div> <div id="fix"> The test checks transformedProps.echartOptions.xAxis.data for sorted order, but the transformProps function does not set this property. ECharts may infer category data from series, but the test assertion won't execute since xAxis.data is undefined. </div> </div> <small><i>Code Review Run #d84cb8</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]
