rusackas commented on code in PR #42594:
URL: https://github.com/apache/superset/pull/42594#discussion_r3693914160
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:
##########
@@ -694,6 +695,39 @@ describe('extractGroupbyLabel', () => {
});
});
+describe('extractDataTotalValues', () => {
+ test('sums numeric metric values across a stacked datum', () => {
+ const { totalStackedValues, thresholdValues } = extractDataTotalValues(
+ [{ __timestamp: '2000-01-01', metric_a: 10, metric_b: 20 }],
+ { stack: true, percentageThreshold: 50, xAxisCol: '__timestamp' },
+ );
+ expect(totalStackedValues).toEqual([30]);
+ expect(thresholdValues).toEqual([15]);
+ });
+
+ // Regression for #36401: query results containing integers beyond
+ // Number.MAX_SAFE_INTEGER are parsed as native BigInt (see
+ // packages/superset-ui-core/src/connection/callApi/parseResponse.ts).
+ // Summing a BigInt datum value against the Number accumulator here
+ // throws instead of producing a stacked total.
+ test('does not throw when a stacked datum contains a BigInt metric value',
() => {
+ const data: DataRecord[] = [
+ {
+ __timestamp: '2000-01-01',
+ metric_a: 10,
+ metric_b: BigInt('9007199254740993'),
+ },
+ ];
+ expect(() =>
+ extractDataTotalValues(data, {
+ stack: true,
+ percentageThreshold: 50,
+ xAxisCol: '__timestamp',
+ }),
+ ).not.toThrow();
Review Comment:
Good catch, fixed. The test only asserted it did not throw, so a broken
normalization could still slip through. Pushed 7ef18a1e with explicit
assertions on totalStackedValues and thresholdValues (accounting for the
expected precision loss when the BigInt exceeds MAX_SAFE_INTEGER).
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:
##########
@@ -694,6 +695,39 @@ describe('extractGroupbyLabel', () => {
});
});
+describe('extractDataTotalValues', () => {
+ test('sums numeric metric values across a stacked datum', () => {
+ const { totalStackedValues, thresholdValues } = extractDataTotalValues(
+ [{ __timestamp: '2000-01-01', metric_a: 10, metric_b: 20 }],
+ { stack: true, percentageThreshold: 50, xAxisCol: '__timestamp' },
+ );
+ expect(totalStackedValues).toEqual([30]);
+ expect(thresholdValues).toEqual([15]);
+ });
+
+ // Regression for #36401: query results containing integers beyond
+ // Number.MAX_SAFE_INTEGER are parsed as native BigInt (see
+ // packages/superset-ui-core/src/connection/callApi/parseResponse.ts).
+ // Summing a BigInt datum value against the Number accumulator here
+ // throws instead of producing a stacked total.
+ test('does not throw when a stacked datum contains a BigInt metric value',
() => {
+ const data: DataRecord[] = [
+ {
+ __timestamp: '2000-01-01',
+ metric_a: 10,
+ metric_b: BigInt('9007199254740993'),
+ },
+ ];
+ expect(() =>
+ extractDataTotalValues(data, {
+ stack: true,
+ percentageThreshold: 50,
+ xAxisCol: '__timestamp',
+ }),
+ ).not.toThrow();
Review Comment:
Yep, that's the point of the PR title being a bit misleading as a pure
test-only PR. I pushed the production normalization fix (775f26e) alongside the
pinning test, so the Jest shard should be green now. Also just tightened the
assertion itself in 7ef18a1e per the other thread.
--
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]
