codeant-ai-for-open-source[bot] commented on code in PR #37522:
URL: https://github.com/apache/superset/pull/37522#discussion_r2736487103
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -322,3 +417,66 @@ test('legend margin: right orientation sets grid.right
correctly', () => {
expect((transformed.echartOptions.grid as any).right).toEqual(270);
});
+
+test('should add a formula annotation when X-axis column has dataset-level
label', () => {
+ const formula: FormulaAnnotationLayer = {
+ name: 'My Formula',
+ annotationType: AnnotationType.Formula,
+ value: 'x*2',
+ style: AnnotationStyle.Solid,
+ show: true,
+ showLabel: true,
+ };
+ const timeColumnName = 'ds';
+ const timeColumnLabel = 'Time Label';
+ const testData = [
+ {
+ [timeColumnLabel]: new Date(599616000000).toISOString(),
+ boy: 1,
+ girl: 2,
+ },
+ {
+ [timeColumnLabel]: new Date(599916000000).toISOString(),
Review Comment:
**Suggestion:** The test creates timestamp values using `new
Date(...).toISOString()`, producing ISO strings while other test data uses
numeric epoch milliseconds; if the transform expects numeric timestamps the ISO
strings will prevent series/annotations from being generated—use numeric epoch
milliseconds instead. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test fails in CI for this chart transform.
- ⚠️ Formula annotation behavior not validated.
- ⚠️ Blocks PR merging until test fixed.
```
</details>
```suggestion
[timeColumnLabel]: 599616000000,
boy: 1,
girl: 2,
},
{
[timeColumnLabel]: 599916000000,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open test file
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
and locate the testData definition at lines 432-443 (the block that uses new
Date(...).toISOString()).
2. Note createTestChartProps (used to build chartProps) is defined earlier
in this file at
line 77 (function createTestChartProps(...): EchartsMixedTimeseriesProps),
which passes
the supplied queriesData through to transformProps unchanged.
3. The test invokes transformProps at line 474 (const result =
transformProps(chartProps);) using chartProps built from the testData
defined at 432-443.
4. Because other tests and fixtures in this file use numeric epoch
milliseconds for
timestamp values (the file contains numeric ds timestamps elsewhere),
transformProps
expects numeric epoch values; passing ISO strings (lines 434 and 439) causes
transformProps to not produce the expected formula annotation series. The
subsequent
assertions at lines 475-481 then fail (formulaSeries undefined or data
empty). Running the
single test (e.g., yarn test transformProps.test.ts -t "should add a formula
annotation
when X-axis column has dataset-level label") reproduces the failing
assertion.
```
</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/MixedTimeseries/transformProps.test.ts
**Line:** 434:439
**Comment:**
*Type Error: The test creates timestamp values using `new
Date(...).toISOString()`, producing ISO strings while other test data uses
numeric epoch milliseconds; if the transform expects numeric timestamps the ISO
strings will prevent series/annotations from being generated—use numeric epoch
milliseconds instead.
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>
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -322,3 +417,66 @@ test('legend margin: right orientation sets grid.right
correctly', () => {
expect((transformed.echartOptions.grid as any).right).toEqual(270);
});
+
+test('should add a formula annotation when X-axis column has dataset-level
label', () => {
+ const formula: FormulaAnnotationLayer = {
+ name: 'My Formula',
+ annotationType: AnnotationType.Formula,
+ value: 'x*2',
+ style: AnnotationStyle.Solid,
+ show: true,
+ showLabel: true,
+ };
+ const timeColumnName = 'ds';
+ const timeColumnLabel = 'Time Label';
+ const testData = [
+ {
+ [timeColumnLabel]: new Date(599616000000).toISOString(),
+ boy: 1,
+ girl: 2,
+ },
+ {
+ [timeColumnLabel]: new Date(599916000000).toISOString(),
+ boy: 3,
+ girl: 4,
+ },
+ ];
+ const chartProps = createTestChartProps({
+ formData: {
+ ...formData,
+ x_axis: timeColumnName,
+ annotationLayers: [formula],
+ },
+ queriesData: [
+ createTestQueryData(testData, {
+ label_map: {
+ [timeColumnLabel]: [timeColumnName],
+ boy: ['boy'],
+ girl: ['girl'],
+ },
+ }),
+ createTestQueryData(testData, {
+ label_map: {
+ [timeColumnLabel]: [timeColumnName],
Review Comment:
**Suggestion:** The `label_map` entries in the test are keyed by the display
label (`Time Label`) mapping to the column name; elsewhere in the file
`label_map` uses column-name keys, so this inconsistent orientation may prevent
the transform from resolving the dataset column correctly—use the column name
as the `label_map` key and list its display label as the value. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test fails in CI for this chart transform.
- ⚠️ Annotation resolution logic not validated.
- ⚠️ Misleading test coverage for dataset label handling.
```
</details>
```suggestion
[timeColumnName]: [timeColumnLabel],
boy: ['boy'],
girl: ['girl'],
},
}),
createTestQueryData(testData, {
label_map: {
[timeColumnName]: [timeColumnLabel],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
and find the queriesData passed into createTestChartProps at lines 450-465;
observe
label_map entries use the display label as the key (e.g., [timeColumnLabel]:
[timeColumnName]) at lines 452-456 and 459-463.
2. The test calls transformProps with the constructed chartProps at line 474
(const result
= transformProps(chartProps);). transformProps expects to resolve columns
using the
dataset column name as the label_map key (consistent with other query
fixtures in this
file that use column-name keys), but the test's mapping orientation is
reversed.
3. Because transformProps looks up the column name (formData.x_axis ===
'ds') in label_map
and the test provided the display label as the key, transformProps fails to
match the
x-axis column to dataset fields, so the formula annotation series is not
created or is
empty. The assertions at lines 475-481 then fail.
4. Run the single test (yarn test transformProps.test.ts -t "should add a
formula
annotation when X-axis column has dataset-level label") to reproduce the
failure;
replacing the label_map key to the column name (as in the improved code)
makes
transformProps resolve the column and the assertions pass.
```
</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/MixedTimeseries/transformProps.test.ts
**Line:** 453:460
**Comment:**
*Logic Error: The `label_map` entries in the test are keyed by the
display label (`Time Label`) mapping to the column name; elsewhere in the file
`label_map` uses column-name keys, so this inconsistent orientation may prevent
the transform from resolving the dataset column correctly—use the column name
as the `label_map` key and list its display label as the value.
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>
--
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]
