-
Notifications
You must be signed in to change notification settings - Fork 16.6k
feat(timeseries): remove stream style for bar charts #37532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(timeseries): remove stream style for bar charts #37532
Conversation
Code Review Agent Run #8e558fActionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| const stackControl: any = getControl('stack'); | ||
| expect(stackControl).toBeDefined(); | ||
| expect(stackControl.config).toBeDefined(); | ||
| expect(stackControl.config.choices).toBe(BarChartStackControlOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The test asserts reference equality with toBe for choices against BarChartStackControlOptions; if the control panel clones or creates an identical array rather than reusing the same reference, the test will fail even though values are correct — use a deep equality assertion instead. [logic error]
Severity Level: Major ⚠️
- ❌ Unit test failure in Bar controlPanel test file.
- ⚠️ CI job may fail blocking merges.| expect(stackControl.config.choices).toBe(BarChartStackControlOptions); | |
| expect(stackControl.config.choices).toEqual(BarChartStackControlOptions); |
Steps of Reproduction ✅
1. Run the Jest test suite targeting this file:
- npm/yarn test -- test/Timeseries/Bar/controlPanel.test.ts
- The test with the assertion is at
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts:132-137
(test name: "should use BarChartStackControlOptions for stack control").
2. The test imports controlPanel at line 19: import controlPanel from
'../../../src/Timeseries/Regular/Bar/controlPanel';
- getControl (defined at line 27) locates the 'stack' control and reads
stackControl.config.choices.
3. If the implementation of src/Timeseries/Regular/Bar/controlPanel constructs a new array
(structurally identical) instead of reusing the exported constant from src/constants
(BarChartStackControlOptions, imported at lines 20-23), then stackControl.config.choices
!== BarChartStackControlOptions.
4. The assertion at line 136 uses toBe (reference equality) and will fail with a Jest diff
showing different object references even though array contents match; replacing with
toEqual prevents this brittle failure.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts
**Line:** 136:136
**Comment:**
*Logic Error: The test asserts reference equality with `toBe` for `choices` against `BarChartStackControlOptions`; if the control panel clones or creates an identical array rather than reusing the same reference, the test will fail even though values are correct — use a deep equality assertion 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.
Code Review Agent Run #e1b455Actionable 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 |
alexandrusoare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just some nits
| // Should have a description that includes D3 time format docs | ||
| expect(timeFormatControl.config.description).toContain('D3'); | ||
| }); | ||
| test('should have proper control configuration for x_axis_time_format', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about combining some of the tests? For example for x_axis_time_format, we can check all the properties in one test since we are not interacting with it rather than having 3-4 separate tests
| ['color_scheme'], | ||
| ['time_shift_color'], | ||
| ...showValueSection, | ||
| [showValueControl], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can do the conditional inside plugins/plugin-chart-echarts/src/controls.tsx L:186?
SUMMARY
Removes the Stream style option from Bar charts while keeping it for Area and Line charts.
Note: previous tests were flattened on
"superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts"BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
89481-after-1.mov
TESTING INSTRUCTIONS
When changing from a line chart (or another one that can have Stream style) to a bar chart, the bar chart must not appear with Stream style.
ADDITIONAL INFORMATION