codeant-ai-for-open-source[bot] commented on code in PR #38083:
URL: https://github.com/apache/superset/pull/38083#discussion_r2825284977
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -289,6 +302,99 @@ test('should configure time axis labels to show max label
for last month visibil
);
});
+test('should provide a defined formatter when showMaxLabel is true to prevent
raw timestamp display', () => {
+ const formData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: 'ds',
+ metric: 'sum__num',
+ viz_type: 'my_viz',
+ };
+ const queriesData = [
+ {
+ data: [
+ { sum__num: 100, __timestamp: new Date('2020-01-01').getTime() },
+ { sum__num: 200, __timestamp: new Date('2021-01-01').getTime() },
+ { sum__num: 300, __timestamp: new Date('2022-01-01').getTime() },
+ { sum__num: 400, __timestamp: new Date('2023-01-01').getTime() },
+ ],
+ colnames: ['sum__num', '__timestamp'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Temporal],
+ },
+ ];
+ const chartProps = new ChartProps({
+ formData,
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const result = transformProps(
+ chartProps as unknown as EchartsTimeseriesChartProps,
+ );
+
+ const xAxis = result.echartOptions.xAxis as {
+ axisLabel: {
+ showMaxLabel?: boolean;
+ formatter?: (...args: unknown[]) => string;
+ };
+ };
+ // showMaxLabel must be true and formatter must be defined so ECharts
+ // formats the forced max label consistently with other labels
+ expect(xAxis.axisLabel.showMaxLabel).toBe(true);
+ expect(xAxis.axisLabel.formatter).toBeDefined();
+ expect(typeof xAxis.axisLabel.formatter).toBe('function');
+
+ // The max tick (last data point) must format as a year, not a raw timestamp
+ const maxTimestamp = new Date('2023-01-01').getTime();
+ expect(xAxis.axisLabel.formatter!(maxTimestamp)).toBe('2023');
+});
+
+test('should format max label using explicit xAxisTimeFormat when showMaxLabel
is true', () => {
+ const formData = {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ granularity_sqla: 'ds',
+ metric: 'sum__num',
+ viz_type: 'my_viz',
+ x_axis_time_format: '%Y-%m-%d',
+ };
+ const queriesData = [
+ {
+ data: [
+ { sum__num: 100, __timestamp: new Date('2022-06-15').getTime() },
+ { sum__num: 200, __timestamp: new Date('2023-03-20').getTime() },
+ ],
+ colnames: ['sum__num', '__timestamp'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Temporal],
+ },
+ ];
+ const chartProps = new ChartProps({
+ formData,
+ width: 800,
+ height: 600,
+ queriesData,
+ theme: supersetTheme,
+ });
+
+ const result = transformProps(
+ chartProps as unknown as EchartsTimeseriesChartProps,
+ );
+
+ const xAxis = result.echartOptions.xAxis as {
+ axisLabel: {
+ showMaxLabel?: boolean;
+ formatter?: (...args: unknown[]) => string;
+ };
+ };
+ expect(xAxis.axisLabel.showMaxLabel).toBe(true);
+ expect(xAxis.axisLabel.formatter).toBeDefined();
+
+ const maxTimestamp = new Date('2023-03-20').getTime();
Review Comment:
**Suggestion:** The new timeseries tests build timestamps with `new
Date('YYYY-MM-DD')`, which is interpreted as UTC and then formatted in local
time, so checking that the max label formats to a specific year or date can
intermittently fail depending on the machine's timezone; switching to the
numeric Date constructor removes this timezone sensitivity. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Timeseries transformer Jest tests can fail on non-UTC machines.
- ⚠️ CI or developer environments may see flaky test behavior.
```
</details>
```suggestion
{ sum__num: 100, __timestamp: new Date(2020, 0, 1).getTime() },
{ sum__num: 200, __timestamp: new Date(2021, 0, 1).getTime() },
{ sum__num: 300, __timestamp: new Date(2022, 0, 1).getTime() },
{ sum__num: 400, __timestamp: new Date(2023, 0, 1).getTime() },
],
colnames: ['sum__num', '__timestamp'],
coltypes: [GenericDataType.Numeric, GenericDataType.Temporal],
},
];
const chartProps = new ChartProps({
formData,
width: 800,
height: 600,
queriesData,
theme: supersetTheme,
});
const result = transformProps(
chartProps as unknown as EchartsTimeseriesChartProps,
);
const xAxis = result.echartOptions.xAxis as {
axisLabel: {
showMaxLabel?: boolean;
formatter?: (...args: unknown[]) => string;
};
};
// showMaxLabel must be true and formatter must be defined so ECharts
// formats the forced max label consistently with other labels
expect(xAxis.axisLabel.showMaxLabel).toBe(true);
expect(xAxis.axisLabel.formatter).toBeDefined();
expect(typeof xAxis.axisLabel.formatter).toBe('function');
// The max tick (last data point) must format as a year, not a raw
timestamp
const maxTimestamp = new Date(2023, 0, 1).getTime();
expect(xAxis.axisLabel.formatter!(maxTimestamp)).toBe('2023');
});
test('should format max label using explicit xAxisTimeFormat when
showMaxLabel is true', () => {
const formData = {
colorScheme: 'bnbColors',
datasource: '3__table',
granularity_sqla: 'ds',
metric: 'sum__num',
viz_type: 'my_viz',
x_axis_time_format: '%Y-%m-%d',
};
const queriesData = [
{
data: [
{ sum__num: 100, __timestamp: new Date(2022, 5, 15).getTime() },
{ sum__num: 200, __timestamp: new Date(2023, 2, 20).getTime() },
],
colnames: ['sum__num', '__timestamp'],
coltypes: [GenericDataType.Numeric, GenericDataType.Temporal],
},
];
const chartProps = new ChartProps({
formData,
width: 800,
height: 600,
queriesData,
theme: supersetTheme,
});
const result = transformProps(
chartProps as unknown as EchartsTimeseriesChartProps,
);
const xAxis = result.echartOptions.xAxis as {
axisLabel: {
showMaxLabel?: boolean;
formatter?: (...args: unknown[]) => string;
};
};
expect(xAxis.axisLabel.showMaxLabel).toBe(true);
expect(xAxis.axisLabel.formatter).toBeDefined();
const maxTimestamp = new Date(2023, 2, 20).getTime();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Jest test suite for the ECharts timeseries transformers
(`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts`)
on
a machine or CI runner configured with a negative timezone offset, e.g.
`TZ=Pacific/Honolulu` or `TZ=America/Los_Angeles`.
2. Jest executes the test `should provide a defined formatter when
showMaxLabel is true to
prevent raw timestamp display` starting at line 305, where `__timestamp`
values and
`maxTimestamp` are created with `new Date('2023-01-01').getTime()`.
3. The test calls `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps`
which, via
its internal `getXAxisFormatter` logic, uses the smart date formatter
registered in
`beforeAll` (this test file) and implemented in
`packages/superset-ui-core/src/time-format/formatters/smartDate.ts:25-41` to
produce
`xAxis.axisLabel.formatter`.
4. Inside the test, `xAxis.axisLabel.formatter!(maxTimestamp)` is invoked
and formats the
timestamp using local time; in a timezone where local midnight differs from
UTC midnight,
the resulting year (derived from local time) can be `2022` instead of
`2023`, causing the
expectation `toBe('2023')` (line 351) to fail and making the test suite
timezone-dependent
and flaky; the same issue applies to the explicit format test using
`'2023-03-20'` at
lines 354-396.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
**Line:** 316:394
**Comment:**
*Logic Error: The new timeseries tests build timestamps with `new
Date('YYYY-MM-DD')`, which is interpreted as UTC and then formatted in local
time, so checking that the max label formats to a specific year or date can
intermittently fail depending on the machine's timezone; switching to the
numeric Date constructor removes this timezone sensitivity.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38083&comment_hash=31e520ab0307ff78e5171d89a5e900db4007413bb917e32c6d083508ba985752&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38083&comment_hash=31e520ab0307ff78e5171d89a5e900db4007413bb917e32c6d083508ba985752&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.test.ts:
##########
@@ -16,8 +16,47 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { NumberFormats } from '@superset-ui/core';
-import { getPercentFormatter } from '../../src/utils/formatters';
+import {
+ createSmartDateFormatter,
+ getTimeFormatterRegistry,
+ NumberFormats,
+ SMART_DATE_ID,
+} from '@superset-ui/core';
+import {
+ getPercentFormatter,
+ getXAxisFormatter,
+} from '../../src/utils/formatters';
+
+// Register the smart date formatter so tests use real formatting logic
+beforeAll(() => {
+ getTimeFormatterRegistry().registerValue(
+ SMART_DATE_ID,
+ createSmartDateFormatter(),
+ );
+});
+
+test('getXAxisFormatter should format a year-boundary date as a four-digit
year', () => {
+ const formatter = getXAxisFormatter(SMART_DATE_ID);
+ expect(formatter(new Date('2023-01-01T00:00:00Z'))).toBe('2023');
+});
+
+test('getXAxisFormatter should use smart date when no format is specified', ()
=> {
+ const formatter = getXAxisFormatter();
+ expect(formatter(new Date('2023-01-01T00:00:00Z'))).toBe('2023');
+});
+
+test('getXAxisFormatter should format numeric epoch input the same as Date
input', () => {
+ const formatter = getXAxisFormatter(SMART_DATE_ID);
+ const epoch = new Date('2023-01-01T00:00:00Z').getTime();
+ expect(formatter(epoch)).toBe('2023');
+});
+
+test('getXAxisFormatter should return a time formatter for explicit time
formats', () => {
+ const formatter = getXAxisFormatter('%Y-%m-%d');
+ expect(formatter).toBeDefined();
+ const date = new Date('2023-06-15T00:00:00Z');
Review Comment:
**Suggestion:** The tests for `getXAxisFormatter` create dates using ISO
strings with an explicit `Z` suffix, which are parsed as UTC and then formatted
in local time, making the expected `'2023'` and `'2023-06-15'` assertions
timezone-dependent and potentially flaky on non-UTC environments; using the
Date numeric constructor avoids this ambiguity and makes the tests
deterministic. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Formatter tests fail on machines with non-UTC timezones.
- ⚠️ Frontend unit test suite unreliable across developer environments.
```
</details>
```suggestion
expect(formatter(new Date(2023, 0, 1))).toBe('2023');
});
test('getXAxisFormatter should use smart date when no format is specified',
() => {
const formatter = getXAxisFormatter();
expect(formatter(new Date(2023, 0, 1))).toBe('2023');
});
test('getXAxisFormatter should format numeric epoch input the same as Date
input', () => {
const formatter = getXAxisFormatter(SMART_DATE_ID);
const epoch = new Date(2023, 0, 1).getTime();
expect(formatter(epoch)).toBe('2023');
});
test('getXAxisFormatter should return a time formatter for explicit time
formats', () => {
const formatter = getXAxisFormatter('%Y-%m-%d');
expect(formatter).toBeDefined();
const date = new Date(2023, 5, 15);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Set the Node.js process timezone to a non-UTC zone, e.g. run tests with
`TZ=America/Los_Angeles` in the Superset repo root.
2. Execute the frontend unit tests that include
`superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.test.ts`,
which
imports `getXAxisFormatter` from
`superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:89-94`.
3. In the first three tests (lines 38–51),
`getXAxisFormatter(SMART_DATE_ID)` returns a
smart date `TimeFormatter` backed by `createSmartDateFormatter` in
`packages/superset-ui-core/src/time-format/formatters/smartDate.ts:25-41`,
which uses d3's
local-time formatting; it is invoked with `new Date('2023-01-01T00:00:00Z')`
or its epoch
value, representing 2022-12-31 in local time for America/Los_Angeles, so
`%Y` resolves to
`'2022'`.
4. The Jest expectations `.toBe('2023')` (lines 40, 45, 51) and
`.toBe('2023-06-15')` in
the fourth test (line 59, where `new Date('2023-06-15T00:00:00Z')` becomes
2023-06-14
local time) fail on non-UTC machines, causing the formatter test suite to be
red purely
due to the host timezone.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.test.ts
**Line:** 40:57
**Comment:**
*Logic Error: The tests for `getXAxisFormatter` create dates using ISO
strings with an explicit `Z` suffix, which are parsed as UTC and then formatted
in local time, making the expected `'2023'` and `'2023-06-15'` assertions
timezone-dependent and potentially flaky on non-UTC environments; using the
Date numeric constructor avoids this ambiguity and makes the tests
deterministic.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38083&comment_hash=f0ed2587c1ad0fdef7cd4f512f96ddbd61d5e6f8b900383eb58fed2e321071e9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38083&comment_hash=f0ed2587c1ad0fdef7cd4f512f96ddbd61d5e6f8b900383eb58fed2e321071e9&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/BigNumber/transformProps.test.ts:
##########
@@ -551,3 +565,31 @@ test('BigNumberWithTrendline should preserve static
currency format', () => {
// Static mode should always show £
expect(transformed.headerFormatter(1000)).toContain('£');
});
+
+test('BigNumberWithTrendline should provide a defined formatter when
showXAxisMinMaxLabels is true', () => {
+ const props = generateProps(
+ [
+ { __timestamp: new Date('2020-01-01').getTime(), value: 100 },
+ { __timestamp: new Date('2021-01-01').getTime(), value: 200 },
+ { __timestamp: new Date('2022-01-01').getTime(), value: 300 },
+ { __timestamp: new Date('2023-01-01').getTime(), value: 400 },
+ ],
+ { showTrendLine: true, showXAxisMinMaxLabels: true },
+ );
+
+ const transformed = transformProps(props);
+ const xAxis = transformed.echartOptions?.xAxis as {
+ axisLabel: {
+ showMaxLabel?: boolean;
+ formatter?: (...args: unknown[]) => string;
+ };
+ };
+
+ expect(xAxis.axisLabel.showMaxLabel).toBe(true);
+ expect(xAxis.axisLabel.formatter).toBeDefined();
+ expect(typeof xAxis.axisLabel.formatter).toBe('function');
+
+ // The max tick must format as a year, not a raw timestamp
+ const maxTimestamp = new Date('2023-01-01').getTime();
Review Comment:
**Suggestion:** The new BigNumberWithTrendline test constructs timestamps
using `new Date('YYYY-MM-DD')`, which is parsed as UTC and then formatted in
local time, so asserting that the formatter returns `'2023'` can fail on
machines in negative timezones; using the numeric Date constructor keeps the
local date fields consistent and avoids timezone-dependent failures. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unit test flaky on non-UTC developer machines.
- ⚠️ Local test runs may fail, blocking contributions.
```
</details>
```suggestion
{ __timestamp: new Date(2020, 0, 1).getTime(), value: 100 },
{ __timestamp: new Date(2021, 0, 1).getTime(), value: 200 },
{ __timestamp: new Date(2022, 0, 1).getTime(), value: 300 },
{ __timestamp: new Date(2023, 0, 1).getTime(), value: 400 },
],
{ showTrendLine: true, showXAxisMinMaxLabels: true },
);
const transformed = transformProps(props);
const xAxis = transformed.echartOptions?.xAxis as {
axisLabel: {
showMaxLabel?: boolean;
formatter?: (...args: unknown[]) => string;
};
};
expect(xAxis.axisLabel.showMaxLabel).toBe(true);
expect(xAxis.axisLabel.formatter).toBeDefined();
expect(typeof xAxis.axisLabel.formatter).toBe('function');
// The max tick must format as a year, not a raw timestamp
const maxTimestamp = new Date(2023, 0, 1).getTime();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Set the Node.js process timezone to a negative offset (for example, run
tests with
`TZ=America/Los_Angeles`) so local time is behind UTC.
2. Run the Jest test suite that includes
`superset-frontend/plugins/plugin-chart-echarts/test/BigNumber/transformProps.test.ts`,
which executes the test `BigNumberWithTrendline should provide a defined
formatter when
showXAxisMinMaxLabels is true` at lines 569–595.
3. In this test, timestamps are created using `new
Date('2020-01-01').getTime()` … `new
Date('2023-01-01').getTime()`, producing epoch milliseconds for
`YYYY-01-01T00:00:00Z`
(UTC) even though formatting later happens in local time.
4. `beforeAll` at lines 36–41 registers the smart date formatter via
`getTimeFormatterRegistry().registerValue(SMART_DATE_ID,
createSmartDateFormatter())`, and
`transformProps` builds an ECharts x-axis whose `axisLabel.formatter` uses
this local-time
formatter; when `axisLabel.formatter` is invoked with `maxTimestamp = new
Date('2023-01-01').getTime()`, the local-time conversion yields `2022-12-31`
in a negative
timezone so the formatter returns `'2022'`, causing
`expect(xAxis.axisLabel.formatter!(maxTimestamp)).toBe('2023');` (line 594)
to fail.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/test/BigNumber/transformProps.test.ts
**Line:** 572:593
**Comment:**
*Logic Error: The new BigNumberWithTrendline test constructs timestamps
using `new Date('YYYY-MM-DD')`, which is parsed as UTC and then formatted in
local time, so asserting that the formatter returns `'2023'` can fail on
machines in negative timezones; using the numeric Date constructor keeps the
local date fields consistent and avoids timezone-dependent failures.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38083&comment_hash=3555672d0e0efa8a2f0f3dc01b9332c6f57d2cac646c5309d777458e8ba92d7c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38083&comment_hash=3555672d0e0efa8a2f0f3dc01b9332c6f57d2cac646c5309d777458e8ba92d7c&reaction=dislike'>👎</a>
--
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]
