Copilot commented on code in PR #35915: URL: https://github.com/apache/superset/pull/35915#discussion_r2519203224
########## superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts: ########## @@ -0,0 +1,172 @@ +/** + * 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 { ChartProps, SMART_DATE_ID, supersetTheme } from '@superset-ui/core'; +import transformProps from '../../../src/Timeseries/transformProps'; +import { DEFAULT_FORM_DATA } from '../../../src/Timeseries/constants'; +import { + EchartsTimeseriesSeriesType, + EchartsTimeseriesFormData, + EchartsTimeseriesChartProps, +} from '../../../src/Timeseries/types'; +import { GenericDataType } from '@apache-superset/core/api/core'; +import { + D3_FORMAT_OPTIONS, + D3_TIME_FORMAT_OPTIONS, +} from '@superset-ui/chart-controls'; + +describe('Scatter Chart X-axis Time Formatting', () => { + const baseFormData: EchartsTimeseriesFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'supersetColors', + datasource: '1__table', + granularity_sqla: '__timestamp', + metric: ['column 1'], + groupby: [], + viz_type: 'echarts_timeseries_scatter', + seriesType: EchartsTimeseriesSeriesType.Scatter, + }; + + const timeseriesData = [ + { + data: [ + { column_1: 0.72099, __timestamp: 1609459200000 }, + { column_1: 0.77954, __timestamp: 1612137600000 }, + { column_1: 2.83434, __timestamp: 1614556800000 }, + ], + colnames: ['column_1', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }, + ]; + + const baseChartPropsConfig = { + width: 800, + height: 600, + queriesData: timeseriesData, + theme: supersetTheme, + }; + + test('xAxisTimeFormat has no default formatter', () => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: baseFormData, + }); + + const transformedProps = transformProps( + chartProps as EchartsTimeseriesChartProps, + ); + + expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel'); + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + expect(xAxis.axisLabel.formatter).toBeUndefined(); + }); + + test.each(D3_TIME_FORMAT_OPTIONS)('should handle %s format', format => { Review Comment: The test.each is using an array of tuples but only destructuring the first element. The test title placeholder `%s` will display the entire tuple instead of just the format ID. Either destructure both elements (e.g., `([formatId, label])`) and use `formatId` in the test, or map the options to extract just the format IDs before passing to test.each. For example: ```typescript test.each(D3_TIME_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => { ``` ```suggestion test.each(D3_TIME_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => { ``` ########## superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts: ########## @@ -0,0 +1,156 @@ +/** + * 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 { ControlPanelsContainerProps } from '@superset-ui/chart-controls/types'; +import controlPanel from '../../../src/Timeseries/Regular/Scatter/controlPanel'; + +const config = controlPanel; + +const getControl = (controlName: string) => { + for (const section of config.controlPanelSections) { + if (section && section.controlSetRows) { + for (const row of section.controlSetRows) { + for (const control of row) { + if ( + typeof control === 'object' && + control !== null && + 'name' in control && + control.name === controlName + ) { + return control; + } + } + } + } + } + + return null; +}; + +const mockControls = ( + xAxisColumn: string | null, + xAxisType: string | null, +): ControlPanelsContainerProps => { + const options = xAxisType + ? [{ column_name: xAxisColumn, type: xAxisType }] + : []; + + return { + controls: { + // @ts-ignore + x_axis: { + value: xAxisColumn, + options: options, + }, + }, + }; +}; + +// tests for x_axis_time_format control +const timeFormatControl: any = getControl('x_axis_time_format'); + +test('scatter chart control panel should include x_axis_time_format control in the panel', () => { + expect(timeFormatControl).toBeDefined(); +}); + +test('scatter chart control panel should have correct default value for x_axis_time_format', () => { + expect(timeFormatControl).toBeDefined(); + expect(timeFormatControl.config).toBeDefined(); + expect(timeFormatControl.config.default).toBe('smart_date'); +}); + +test('scatter chart control panel should have visibility function for x_axis_time_format', () => { + expect(timeFormatControl).toBeDefined(); + expect(timeFormatControl.config.visibility).toBeDefined(); + expect(typeof timeFormatControl.config.visibility).toBe('function'); + + // The visibility function exists - the exact logic is tested implicitly through UI behavior + // The important part is that the control has proper visibility configuration +}); + +const isTimeVisible = ( + xAxisColumn: string | null, + xAxisType: string | null, +): boolean => { + const props = mockControls(xAxisColumn, xAxisType); + const visibilityFn = timeFormatControl?.config?.visibility; + return visibilityFn ? visibilityFn(props) : false; +}; + +test('x_axis_time_format control should be visible for any data types include TIME', () => { + expect(isTimeVisible('time_column', 'TIME')).toBe(true); + expect(isTimeVisible('time_column', 'TIME WITH TIME ZONE')).toBe(true); + expect(isTimeVisible('time_column', 'TIMESTAMP WITH TIME ZONE')).toBe(true); + expect(isTimeVisible('time_column', 'TIMESTAMP WITHOUT TIME ZONE')).toBe( + true, + ); +}); + +test('x_axis_time_format control should be hidden for any data types include TIME', () => { Review Comment: The test description is misleading. It says "should be hidden for any data types include TIME" but the test cases are actually checking for non-time data types (null, FLOAT). The description should say "should be hidden for data types that do NOT include TIME" or "should be hidden for non-time data types". ```suggestion test('x_axis_time_format control should be hidden for data types that do NOT include TIME', () => { ``` ########## superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts: ########## @@ -0,0 +1,172 @@ +/** + * 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 { ChartProps, SMART_DATE_ID, supersetTheme } from '@superset-ui/core'; +import transformProps from '../../../src/Timeseries/transformProps'; +import { DEFAULT_FORM_DATA } from '../../../src/Timeseries/constants'; +import { + EchartsTimeseriesSeriesType, + EchartsTimeseriesFormData, + EchartsTimeseriesChartProps, +} from '../../../src/Timeseries/types'; +import { GenericDataType } from '@apache-superset/core/api/core'; +import { + D3_FORMAT_OPTIONS, + D3_TIME_FORMAT_OPTIONS, +} from '@superset-ui/chart-controls'; + +describe('Scatter Chart X-axis Time Formatting', () => { + const baseFormData: EchartsTimeseriesFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'supersetColors', + datasource: '1__table', + granularity_sqla: '__timestamp', + metric: ['column 1'], + groupby: [], + viz_type: 'echarts_timeseries_scatter', + seriesType: EchartsTimeseriesSeriesType.Scatter, + }; + + const timeseriesData = [ + { + data: [ + { column_1: 0.72099, __timestamp: 1609459200000 }, + { column_1: 0.77954, __timestamp: 1612137600000 }, + { column_1: 2.83434, __timestamp: 1614556800000 }, + ], + colnames: ['column_1', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }, + ]; + + const baseChartPropsConfig = { + width: 800, + height: 600, + queriesData: timeseriesData, + theme: supersetTheme, + }; + + test('xAxisTimeFormat has no default formatter', () => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: baseFormData, + }); + + const transformedProps = transformProps( + chartProps as EchartsTimeseriesChartProps, + ); + + expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel'); + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + expect(xAxis.axisLabel.formatter).toBeUndefined(); + }); + + test.each(D3_TIME_FORMAT_OPTIONS)('should handle %s format', format => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: { + ...baseFormData, + xAxisTimeFormat: format, + }, + }); + + const transformedProps = transformProps( + chartProps as EchartsTimeseriesChartProps, + ); + + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + if (format === SMART_DATE_ID) { + expect(xAxis.axisLabel.formatter).toBeUndefined(); + } else { + expect(typeof xAxis.axisLabel.formatter).toBe('function'); + expect(xAxis.axisLabel.formatter.id).toBe(format); + } + }); +}); + +describe('Scatter Chart X-axis Number Formatting', () => { + const baseFormData: EchartsTimeseriesFormData = { + ...DEFAULT_FORM_DATA, + colorScheme: 'supersetColors', + datasource: '1__table', + metric: ['column_1'], + x_axis: 'column_2', + groupby: [], + viz_type: 'echarts_timeseries_scatter', + seriesType: EchartsTimeseriesSeriesType.Scatter, + }; + + const timeseriesData = [ + { + data: [ + { column_1: 0.72099, column_2: 3.01699 }, + { column_1: 0.77954, column_2: 3.44802 }, + { column_1: 2.83434, column_2: 3.58095 }, + ], + colnames: ['column_1', 'column_2'], + coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], + }, + ]; + + const baseChartPropsConfig = { + width: 800, + height: 600, + queriesData: timeseriesData, + theme: supersetTheme, + }; + + test('should use SMART_NUMBER as default xAxisNumberFormat', () => { + const chartProps = new ChartProps({ + ...baseChartPropsConfig, + formData: baseFormData, + }); + + const transformedProps = transformProps( + chartProps as EchartsTimeseriesChartProps, + ); + + expect(transformedProps.echartOptions.xAxis).toHaveProperty('axisLabel'); + const xAxis = transformedProps.echartOptions.xAxis as any; + expect(xAxis.axisLabel).toHaveProperty('formatter'); + expect(typeof xAxis.axisLabel.formatter).toBe('function'); + expect(xAxis.axisLabel.formatter.id).toBe('SMART_NUMBER'); + }); + + test.each(D3_FORMAT_OPTIONS)('should handle %s format', format => { Review Comment: The test.each is using an array of tuples but only destructuring the first element. The test title placeholder `%s` will display the entire tuple instead of just the format ID. Either destructure both elements (e.g., `([formatId, label])`) and use `formatId` in the test, or map the options to extract just the format IDs before passing to test.each. For example: ```typescript test.each(D3_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => { ``` ```suggestion test.each(D3_FORMAT_OPTIONS.map(([id]) => id))('should handle %s format', format => { ``` -- 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]
