codeant-ai-for-open-source[bot] commented on code in PR #37532:
URL: https://github.com/apache/superset/pull/37532#discussion_r2738886744
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts:
##########
@@ -17,188 +17,204 @@
* under the License.
*/
import controlPanel from '../../../src/Timeseries/Regular/Bar/controlPanel';
-
-describe('Bar Chart Control Panel', () => {
- describe('x_axis_time_format control', () => {
- it('should include x_axis_time_format control in the panel', () => {
- const config = controlPanel;
-
- // Look for x_axis_time_format control in all sections and rows
- let foundTimeFormatControl = false;
-
- for (const section of config.controlPanelSections) {
- if (section && section.controlSetRows) {
- for (const row of section.controlSetRows) {
- for (const control of row) {
- if (
- typeof control === 'object' &&
- control !== null &&
- 'name' in control &&
- control.name === 'x_axis_time_format'
- ) {
- foundTimeFormatControl = true;
- break;
- }
- }
- if (foundTimeFormatControl) break;
- }
- if (foundTimeFormatControl) break;
- }
- }
-
- expect(foundTimeFormatControl).toBe(true);
- });
-
- it('should have correct default value for x_axis_time_format', () => {
- const config = controlPanel;
-
- // Find the x_axis_time_format control
- let timeFormatControl: any = null;
-
- for (const section of config.controlPanelSections) {
- if (section && section.controlSetRows) {
- for (const row of section.controlSetRows) {
- for (const control of row) {
- if (
- typeof control === 'object' &&
- control !== null &&
- 'name' in control &&
- control.name === 'x_axis_time_format'
- ) {
- timeFormatControl = control;
- break;
- }
- }
- if (timeFormatControl) break;
+import {
+ BarChartStackControlOptions,
+ StackControlsValue,
+} from '../../../src/constants';
+
+const config = controlPanel;
+
+const getControl = (controlName: string) => {
+ for (const section of config.controlPanelSections) {
+ if (section && section.controlSetRows) {
+ for (const row of section.controlSetRows) {
+ for (const control of row) {
+ if (
+ typeof control === 'object' &&
+ control !== null &&
+ 'name' in control &&
+ control.name === controlName
+ ) {
+ return control;
}
- if (timeFormatControl) break;
}
}
+ }
+ }
+
+ return null;
+};
+
+// Mock getStandardizedControls
+jest.mock('@superset-ui/chart-controls', () => {
+ const actual = jest.requireActual('@superset-ui/chart-controls');
+ return {
+ ...actual,
+ getStandardizedControls: jest.fn(() => ({
+ popAllMetrics: jest.fn(() => []),
+ popAllColumns: jest.fn(() => []),
+ })),
+ };
+});
- expect(timeFormatControl).toBeDefined();
- expect(timeFormatControl.config).toBeDefined();
- expect(timeFormatControl.config.default).toBe('smart_date');
- });
-
- it('should have visibility function for x_axis_time_format', () => {
- const config = controlPanel;
-
- // Find the x_axis_time_format control
- let timeFormatControl: any = null;
-
- for (const section of config.controlPanelSections) {
- if (section && section.controlSetRows) {
- for (const row of section.controlSetRows) {
- for (const control of row) {
- if (
- typeof control === 'object' &&
- control !== null &&
- 'name' in control &&
- control.name === 'x_axis_time_format'
- ) {
- timeFormatControl = control;
- break;
- }
- }
- if (timeFormatControl) break;
- }
- if (timeFormatControl) break;
- }
- }
+test('should include x_axis_time_format control in the panel', () => {
+ const timeFormatControl = getControl('x_axis_time_format');
+ expect(timeFormatControl).toBeDefined();
+});
- expect(timeFormatControl).toBeDefined();
- expect(timeFormatControl.config.visibility).toBeDefined();
- expect(typeof timeFormatControl.config.visibility).toBe('function');
-
- // The visibility function exists - the exact logic is tested implicitly
through UI behavior
- // The important part is that the control has proper visibility
configuration
- });
-
- it('should have proper control configuration', () => {
- const config = controlPanel;
-
- // Find the x_axis_time_format control
- let timeFormatControl: any = null;
-
- for (const section of config.controlPanelSections) {
- if (section && section.controlSetRows) {
- for (const row of section.controlSetRows) {
- for (const control of row) {
- if (
- typeof control === 'object' &&
- control !== null &&
- 'name' in control &&
- control.name === 'x_axis_time_format'
- ) {
- timeFormatControl = control;
- break;
- }
- }
- if (timeFormatControl) break;
- }
- if (timeFormatControl) break;
- }
- }
+test('should have correct default value for x_axis_time_format', () => {
+ const timeFormatControl: any = getControl('x_axis_time_format');
+ expect(timeFormatControl).toBeDefined();
+ expect(timeFormatControl.config).toBeDefined();
+ expect(timeFormatControl.config.default).toBe('smart_date');
+});
- expect(timeFormatControl).toBeDefined();
- expect(timeFormatControl.config).toMatchObject({
- default: 'smart_date',
- disableStash: true,
- resetOnHide: false,
- });
+test('should have visibility function for x_axis_time_format', () => {
+ const timeFormatControl: any = getControl('x_axis_time_format');
+ expect(timeFormatControl).toBeDefined();
+ expect(timeFormatControl.config.visibility).toBeDefined();
+ expect(typeof timeFormatControl.config.visibility).toBe('function');
+});
- // 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', () => {
+ const timeFormatControl: any = getControl('x_axis_time_format');
+ expect(timeFormatControl).toBeDefined();
+ expect(timeFormatControl.config).toMatchObject({
+ default: 'smart_date',
+ disableStash: true,
+ resetOnHide: false,
});
+ expect(timeFormatControl.config.description).toContain('D3');
+});
- describe('Control panel structure for bar charts', () => {
- it('should have Chart Orientation section', () => {
- const config = controlPanel;
+test('should have Chart Orientation section', () => {
+ const orientationSection = config.controlPanelSections.find(
+ section => section && section.label === 'Chart Orientation',
+ );
+ expect(orientationSection).toBeDefined();
+ expect(orientationSection!.expanded).toBe(true);
+});
- const orientationSection = config.controlPanelSections.find(
- section => section && section.label === 'Chart Orientation',
- );
+test('should have Chart Options section with X Axis controls', () => {
+ const chartOptionsSection = config.controlPanelSections.find(
+ section => section && section.label === 'Chart Options',
+ );
+ expect(chartOptionsSection).toBeDefined();
+ expect(chartOptionsSection!.expanded).toBe(true);
+ expect(chartOptionsSection!.controlSetRows).toBeDefined();
+ expect(chartOptionsSection!.controlSetRows!.length).toBeGreaterThan(0);
+});
- expect(orientationSection).toBeDefined();
- expect(orientationSection!.expanded).toBe(true);
- });
+test('should have proper form data overrides', () => {
+ expect(config.formDataOverrides).toBeDefined();
+ expect(typeof config.formDataOverrides).toBe('function');
- it('should have Chart Options section with X Axis controls', () => {
- const config = controlPanel;
+ const mockFormData = {
+ datasource: '1__table',
+ viz_type: 'echarts_timeseries_bar',
+ metrics: ['test_metric'],
+ groupby: ['test_column'],
+ other_field: 'test',
+ };
- const chartOptionsSection = config.controlPanelSections.find(
- section => section && section.label === 'Chart Options',
- );
+ const result = config.formDataOverrides!(mockFormData);
- expect(chartOptionsSection).toBeDefined();
- expect(chartOptionsSection!.expanded).toBe(true);
+ expect(result).toHaveProperty('metrics');
+ expect(result).toHaveProperty('groupby');
+ expect(result).toHaveProperty('other_field', 'test');
+});
- // Should contain X Axis subsection header - this is sufficient proof
- expect(chartOptionsSection!.controlSetRows).toBeDefined();
- expect(chartOptionsSection!.controlSetRows!.length).toBeGreaterThan(0);
- });
+test('should include stack control in the panel', () => {
+ const stackControl = getControl('stack');
+ expect(stackControl).toBeDefined();
+});
- it('should have proper form data overrides', () => {
- const config = controlPanel;
+test('should use BarChartStackControlOptions for stack control', () => {
+ const stackControl: any = getControl('stack');
+ expect(stackControl).toBeDefined();
+ expect(stackControl.config).toBeDefined();
+ expect(stackControl.config.choices).toBe(BarChartStackControlOptions);
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit test failure in Bar controlPanel test file.
- ⚠️ CI job may fail blocking merges.
```
</details>
```suggestion
expect(stackControl.config.choices).toEqual(BarChartStackControlOptions);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</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/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.
```
</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]
