test(echarts): pin Bar chart X Axis Title flows through untouched (#42560) - #42599
test(echarts): pin Bar chart X Axis Title flows through untouched (#42560)#42599rusackas wants to merge 2 commits into
Conversation
…2560) Closes #42560 Adds regression tests on Timeseries/transformProps.ts confirming xAxisTitle is assigned verbatim to the rendered axis name and is never derived from xAxisNumberFormat/yAxisFormat ("unit"). Also documents the existing horizontal-orientation axis swap, where the X Axis Title control maps onto the rendered category (left) axis rather than the bottom axis — a likely source of the reporter's confusion, separate from any "unit overwrite" in the code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dosubot
Bot
added
viz:charts:bar
Code Review Agent Run #5632f5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Nice work on this PR, @rusackas. The TDD approach here is solid — these two tests capture exactly the right things to validate. Test 1 (title preserved verbatim): This aligns with what I found when investigating #42560 — Test 2 (horizontal orientation axis swap): This is a great addition. The swap at lines 547–550 means Documenting this swap behavior in a test is valuable regardless of the #42560 outcome. 👍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| xAxisTitle: 'My X Axis', | ||
| xAxisNumberFormat: 'SMART_NUMBER', | ||
| yAxisFormat: '$,.2f', | ||
| }; | ||
|
|
||
| const chartProps = new ChartProps({ | ||
| ...baseChartPropsConfig, | ||
| formData, |
There was a problem hiding this comment.
Suggestion: The fixture uses temporal __timestamp data inherited from baseChartPropsConfig, so transformProps selects the time formatter and ignores xAxisNumberFormat. The test therefore only verifies direct title assignment and cannot detect an overwrite caused by numeric or currency formatting. Use numeric X-axis query data and assert the title alongside the numeric formatter path. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Numeric X-axis title overwrite regressions remain undetected.
- ⚠️ `SMART_NUMBER` is unused on the temporal formatter path.
- ⚠️ Bar chart formatting coverage does not match the test claim.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts
**Line:** 1003:1010
**Comment:**
*Incomplete Implementation: The fixture uses temporal `__timestamp` data inherited from `baseChartPropsConfig`, so `transformProps` selects the time formatter and ignores `xAxisNumberFormat`. The test therefore only verifies direct title assignment and cannot detect an overwrite caused by numeric or currency formatting. Use numeric X-axis query data and assert the title alongside the numeric formatter path.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
There was a problem hiding this comment.
Agreed. The inherited TIMESTAMP data selects the temporal formatter, so xAxisNumberFormat is not exercised. If this test is intended to pin the numeric-format interaction, could the fixture use numeric X-axis data?
There was a problem hiding this comment.
Agreed. The current fixture inherits temporal __timestamp data, so xAxisNumberFormat: 'SMART_NUMBER' is not exercised; the assertion only proves that xAxisTitle is assigned directly.
The test should override the query fixture with a numeric X-axis column and point the form data at it. For example:
const formData = {
...baseFormData,
orientation: 'vertical',
groupby: ['x'],
xAxisTitle: 'My X Axis',
xAxisNumberFormat: 'SMART_NUMBER',
yAxisFormat: '$,.2f',
};
const chartProps = new ChartProps({
...baseChartPropsConfig,
formData,
queriesData: [
{
...baseChartPropsConfig.queriesData[0],
data: [
{ x: 1000, metric: 10 },
{ x: 2000, metric: 20 },
],
},
],
});The exact numeric column name should match the fixture’s existing X-axis/query-field configuration. The important part is that the X-axis value is numeric and no longer __timestamp, so the numeric formatter path is selected. The test can then continue asserting:
expect((transformedProps.echartOptions.xAxis as any).name).toBe(
'My X Axis',
);This change is necessary for the regression test to validate its stated purpose; otherwise it does not cover numeric/unit formatting at all.
There was a problem hiding this comment.
Good catch, fixed! The fixture was using the inherited temporal data so xAxisNumberFormat never actually hit getNumberFormatter. Swapped in a numeric x-axis column so it does.
| xAxisTitle: 'My X Axis', | ||
| yAxisTitle: 'My Y Axis', | ||
| }; | ||
|
|
||
| const chartProps = new ChartProps({ | ||
| ...baseChartPropsConfig, | ||
| formData, |
There was a problem hiding this comment.
Suggestion: This test claims to validate mapping to the rendered category axis, but it also inherits temporal timestamp data from baseChartPropsConfig, so the rendered left axis is a temporal axis rather than a category axis. The assertion can pass without validating the category-axis behavior described by the test; provide category-based query data for this horizontal-bar case. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Category-axis title mapping remains untested.
- ⚠️ Horizontal Bar coverage exercises temporal data instead.
- ⚠️ Axis-swap assertions may miss category-specific behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts
**Line:** 1030:1036
**Comment:**
*Incomplete Implementation: This test claims to validate mapping to the rendered category axis, but it also inherits temporal timestamp data from `baseChartPropsConfig`, so the rendered left axis is a temporal axis rather than a category axis. The assertion can pass without validating the category-axis behavior described by the test; provide category-based query data for this horizontal-bar case.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Agreed. With the inherited TIMESTAMP fixture, the swapped axis remains a time axis rather than a category axis. Could this either use category data and assert the axis type, or rename the test and comment to describe the type-independent axis swap?
There was a problem hiding this comment.
Agreed—the test should use categorical query data so it validates the behavior it describes, rather than relying on inherited timestamp data.
Please override the fixture’s query data with a non-temporal dimension and assert the rendered axis type as well as the title. For example:
const chartProps = new ChartProps({
...baseChartPropsConfig,
formData,
queriesData: [
{
data: [
{ category: 'A', value: 10 },
{ category: 'B', value: 20 },
],
},
],
});Then assert:
expect(renderedYAxis.type).toBe('category');
expect(renderedYAxis.name).toBe('My X Axis');
expect(renderedXAxis.name).toBe('My Y Axis');The existing test name and comment can remain once the fixture is categorical: they accurately document that, for horizontal bars, the user-facing xAxisTitle is rendered on the category (yAxis) axis. Without that fixture change, renaming the test to describe only a type-independent axis swap would be more accurate, but would not cover the intended category-axis behavior.
There was a problem hiding this comment.
Fixed, swapped in categorical data and added a type assertion so this is actually proving the category-axis mapping instead of a temporal axis wearing a category label.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42599 +/- ##
==========================================
- Coverage 65.33% 65.33% -0.01%
==========================================
Files 2803 2803
Lines 158490 158490
Branches 36178 36178
==========================================
- Hits 103557 103551 -6
- Misses 52922 52928 +6
Partials 2011 2011
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ); | ||
| const xAxis = transformedProps.echartOptions.xAxis as any; | ||
|
|
||
| expect(xAxis.name).toBe('My X Axis'); |
There was a problem hiding this comment.
This assertion starts after the Customize control has already produced formData and stops before ECharts renders it; Superset 6.1.0 already has the same direct name: xAxisTitle assignment, so this would pass on the reported-broken release. Could this exercise the reported control-to-render flow with the reporter’s inputs before #42560 is closed?
There was a problem hiding this comment.
That's intentional, this is a transformProps unit test, not a full render-through-ECharts one. As the PR description notes, CI green here doesn't close #42560, it just rules out the number-format theory in this code path. A full control-to-render repro would be a separate, heavier test, worth doing if the bug's still showing up once this lands.
…gression tests Both regression tests inherited temporal `__timestamp` data from baseChartPropsConfig, so `xAxisNumberFormat` and the category-axis type were never actually exercised by the assertions. Switch to numeric and categorical query fixtures respectively so the tests exercise the code paths they claim to cover. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #7d16b3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
This is a test-only PR opened as a TDD-style validation of issue #42560.
#42560 reports that on an ECharts Bar Chart, a custom X Axis Title (Customize tab) is overwritten by "the unit" (e.g. a temporal grain/number-format label) instead of showing the user's custom text. @dosu investigated and found no code path in
Timeseries/transformProps.tsthat derivesxAxis.namefrom any unit/format value —xAxisTitleis assigned directly with no fallback.This PR independently re-verifies dosu's conclusion against the Bar chart's actual code path (Bar reuses the shared
Timeseries/transformProps.tsandTimeseries/buildQuery.ts— itsRegular/Bar/folder only containscontrolPanel.tsxandindex.ts, so there is no separate Bar-specific transform to check). No literalunitstring logic exists anywhere in that transform path.This PR adds 2 regression tests on
Timeseries/transformProps.ts(Bar-specific suite):custom X Axis Title is preserved verbatim, not overwritten by the axis number/currency format ("unit")— sets a customxAxisTitlealongside a currency-likeyAxisFormatand axAxisNumberFormat, and asserts the renderedxAxis.nameequals the custom title exactly.X Axis Title control maps onto the rendered category (left) axis in horizontal orientation, not the bottom axis— documents the existing (and easy to misread) axis swap: in horizontal Bar charts,xAxisTitleends up onechartOptions.yAxis.name(the vertical category axis) whileyAxisTitleends up onechartOptions.xAxis.name(the horizontal value axis). This swap is likely the real source of user confusion dosu flagged in point Fix documentation #2 of their investigation, separate from any "unit overwrite."How to interpret CI
xAxisTitleis never derived from a unit/number-format value in the code, ruling out that specific theory. This does not close Bar Chart: custom X-axis title is overwritten by the unit #42560 — if the bug is still reproducing for the reporter, the cause is elsewhere (stale/legacy stored form data, a rendering-layer issue outside this transform, or the horizontal-orientation control-mapping confusion documented in test 2).TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Disclosure: the local
type-checking-frontendpre-commit hook was skipped (SKIP=type-checking-frontend) on this worktree — it requires a pre-builtlib/spec/index.d.tsthat a fresh worktree doesn't have. All other hooks (prettier, oxlint, custom-rules-frontend, stylelint) passed locally; CI is the real gate for type-checking.🤖 Generated with Claude Code