sadpandajoe commented on code in PR #45555:
URL: https://github.com/apache/superset/pull/45555#discussion_r4415695389
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -1966,3 +1966,206 @@ test('converts xAxisLabelInterval string "0" to number
0', () => {
// "All" must also disable hideOverlap so ECharts never drops a label.
expect(xAxis.axisLabel.hideOverlap).toBe(false);
});
+
+describe('truncateMetric / truncateMetricB', () => {
+ const getSeriesNames = (
+ overrides: Partial<EchartsMixedTimeseriesFormData>,
+ ) => {
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: queriesData,
+ formData: { ...formData, ...overrides },
+ queriesData,
+ });
+ const transformed = transformProps(chartProps);
+ return (transformed.echartOptions.series as SeriesOption[]).map(
+ s => s.name,
+ );
+ };
+
+ test('drops the metric prefix for both queries when both flags are on', ()
=> {
+ const seriesNames = getSeriesNames({
+ truncateMetric: true,
+ truncateMetricB: true,
+ });
+
+ expect(seriesNames).toContain('boy');
+ expect(seriesNames).toContain('girl');
+ expect(seriesNames).not.toContain('sum__num, boy');
+ expect(seriesNames).not.toContain('sum__num, girl');
+ });
+
+ test('keeps the "metric, dimension" name when both flags are off', () => {
+ const seriesNames = getSeriesNames({});
+
+ expect(seriesNames).toContain('sum__num, boy');
+ expect(seriesNames).toContain('sum__num, girl');
+ });
+
+ test('truncateMetricB truncates query B independently of query A', () => {
+ const seriesNames = getSeriesNames({ truncateMetricB: true });
+
+ // Query A (not truncated) keeps the metric prefix.
+ expect(seriesNames).toContain('sum__num, boy');
+ expect(seriesNames).toContain('sum__num, girl');
+ // Query B (truncated) drops it.
+ expect(seriesNames).toContain('boy');
+ expect(seriesNames).toContain('girl');
+ });
+
+ test('attributes the truncated name to the correct query', () => {
+ const seriesNames = getSeriesNames({
+ truncateMetricB: true,
+ showQueryIdentifiers: true,
+ });
+
+ expect(seriesNames).toContain('sum__num (Query A), boy');
+ expect(seriesNames).toContain('boy (Query B)');
+ expect(seriesNames).not.toContain('boy (Query A)');
+ expect(seriesNames).not.toContain('sum__num (Query B), boy');
+ });
+
+ test('still appends the query identifier when truncating, so Query A and B
stay distinguishable (#38451)', () => {
+ const seriesNames = getSeriesNames({
+ truncateMetric: true,
+ truncateMetricB: true,
+ showQueryIdentifiers: true,
+ });
+
+ expect(seriesNames).toContain('boy (Query A)');
+ expect(seriesNames).toContain('girl (Query A)');
+ expect(seriesNames).toContain('boy (Query B)');
+ expect(seriesNames).toContain('girl (Query B)');
+ // The metric is still dropped - only the query suffix is added back.
+ expect(seriesNames).not.toContain('sum__num, boy (Query A)');
+ });
+
+ test('keeps every dimension when a single metric has multiple groupby
columns', () => {
+ const multiDimLabelMap = {
+ ds: ['ds'],
+ 'boy, CA': ['boy', 'CA'],
+ 'girl, NY': ['girl', 'NY'],
+ };
+ const rows = [
+ { 'boy, CA': 1, 'girl, NY': 2, ds: 599616000000 },
+ { 'boy, CA': 3, 'girl, NY': 4, ds: 599916000000 },
+ ];
+ const multiDimQueries = [
+ createTestQueryData(rows, { label_map: multiDimLabelMap }),
+ createTestQueryData(rows, { label_map: multiDimLabelMap }),
+ ];
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: multiDimQueries,
+ formData: {
+ ...formData,
+ groupby: ['gender', 'state'],
+ groupbyB: ['gender', 'state'],
+ truncateMetric: true,
+ truncateMetricB: true,
+ },
+ queriesData: multiDimQueries,
+ });
+ const seriesNames = (
+ transformProps(chartProps).echartOptions.series as SeriesOption[]
+ ).map(s => s.name);
+
+ expect(seriesNames).toContain('boy, CA');
+ expect(seriesNames).toContain('girl, NY');
+ });
+
+ test('drops only the metric for multi-metric queries and keeps identifiers',
() => {
+ const labelMap = {
+ ds: ['ds'],
+ 'm1, boy': ['m1', 'boy'],
+ 'm2, boy': ['m2', 'boy'],
+ };
+ const rows = [
+ { 'm1, boy': 1, 'm2, boy': 2, ds: 599616000000 },
+ { 'm1, boy': 3, 'm2, boy': 4, ds: 599916000000 },
+ ];
+ const queries = [
+ createTestQueryData(rows, { label_map: labelMap }),
+ createTestQueryData(rows, { label_map: labelMap }),
+ ];
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: queries,
+ formData: {
+ ...formData,
+ metrics: ['m1', 'm2'],
+ metricsB: ['m1', 'm2'],
+ truncateMetric: true,
+ truncateMetricB: true,
+ showQueryIdentifiers: true,
+ },
+ queriesData: queries,
+ });
+ const seriesNames = (
+ transformProps(chartProps).echartOptions.series as SeriesOption[]
+ ).map(s => s.name);
+
+ expect(seriesNames).toEqual(
+ expect.arrayContaining(['boy (Query A)', 'boy (Query B)']),
+ );
+ expect(seriesNames).not.toContain('m1, boy (Query A)');
+ });
+
+ test('applies the metric count per query: one metric for A, two for B', ()
=> {
Review Comment:
None of the new cases enable truncation together with a time comparison, or
with an empty groupby, so the `!hasTimeComparison` / `!hasTimeComparisonB` and
`groupby.length > 0` guards in `transformProps.ts` aren't exercised by any
test. Swapping `time_compare` and `time_compare_b` in those two conditions, or
dropping either guard, would still pass this suite, and a comparison series
would then collapse into the same truncated name as the current one.
Could you add a case with `truncateMetric: true`, `showQueryIdentifiers:
true`, and `time_compare` set only for Query A (then `time_compare_b` set only
for Query B), asserting the compared query keeps its offset-based names while
the other query still drops the metric prefix? A mirrored case with `groupby:
[]` and truncation on, asserting the verbose metric name is unchanged, would
cover the other guard.
--
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]
