codeant-ai-for-open-source[bot] commented on code in PR #37712: URL: https://github.com/apache/superset/pull/37712#discussion_r2773496970
########## superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts: ########## @@ -0,0 +1,100 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ControlPanelState } from '../../src/types'; +import { xAxisSortControl } from '../../src/shared-controls/customControls'; + +const createState = ( + overrides: Partial<ControlPanelState>, +): ControlPanelState => + ({ + slice: { slice_id: 1 }, + form_data: {}, + datasource: { + column_formats: {}, + verbose_map: {}, + }, + controls: {}, + common: {}, + metadata: {}, + ...overrides, + }) as ControlPanelState; + +const createControlState = (value: unknown = undefined) => ({ value }) as any; + +test('xAxisSortControl includes axis and metric options when there is no dimension', () => { + const state = createState({ + controls: { + x_axis: { value: 'ds', type: 'Select' }, + groupby: { value: [], type: 'Select' }, + metrics: { value: ['metric_1'], type: 'Select' }, + timeseries_limit_metric: { value: 'sort_metric', type: 'Select' }, + datasource: { + datasource: { + columns: [], + metrics: [], + }, + type: 'Select', + }, + }, + }); + + const { options } = xAxisSortControl.config.mapStateToProps!( + state, + createControlState(), + ) as { options: { value: string }[] }; + + const values = options.map(opt => opt.value); + + expect(values).toContain('ds'); + expect(values).toContain('metric_1'); +}); + +test('xAxisSortControl keeps axis and metric options when a dimension is set', () => { + const state = createState({ + controls: { + x_axis: { value: 'ds', type: 'Select' }, + groupby: { value: ['dim'], type: 'Select' }, + metrics: { value: ['metric_1'], type: 'Select' }, + timeseries_limit_metric: { value: 'sort_metric', type: 'Select' }, + datasource: { + datasource: { + columns: [], + metrics: [], Review Comment: **Suggestion:** In the second test, the groupby dimension (`'dim'`), x-axis (`'ds'`), and metrics (`'metric_1'`, `'sort_metric'`) are referenced in control values, but the mocked `datasource` again has empty `columns` and `metrics` arrays; this inconsistency means any logic that relies on datasource metadata for these fields may receive `undefined`, leading to runtime errors or a test that doesn't faithfully match real control panel state. Populate the mocked `columns` and `metrics` arrays to align with the control values. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Tests for xAxisSortControl may be flaky. - ❌ False negatives hide dimension-related regressions. - ⚠️ CI feedback delayed by inaccurate unit tests. ``` </details> ```suggestion columns: [{ column_name: 'ds' }, { column_name: 'dim' }], metrics: [ { metric_name: 'metric_1' }, { metric_name: 'sort_metric' }, ], ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the test file superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts (`yarn test` or `jest`) which executes the second test starting at line 69. 2. The test constructs a mocked ControlPanelState at lines 70-84 where groupby includes 'dim' and metrics include 'metric_1' but datasource.datasource.columns and metrics arrays are empty (lines 76-80). 3. The test calls xAxisSortControl.config.mapStateToProps! at line 86 which may consult datasource metadata to build the options list for X-Axis Sort By. 4. Because the mocked metadata lacks entries for 'ds', 'dim', 'metric_1', and 'sort_metric', lookups inside mapStateToProps will return undefined or omit expected options; running the test will either fail the expect() assertions at lines 94-99 or not exercise the intended code paths. Adding realistic mocked columns/metrics reproduces the real control flow and validates the intended behavior. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts **Line:** 78:79 **Comment:** *Logic Error: In the second test, the groupby dimension (`'dim'`), x-axis (`'ds'`), and metrics (`'metric_1'`, `'sort_metric'`) are referenced in control values, but the mocked `datasource` again has empty `columns` and `metrics` arrays; this inconsistency means any logic that relies on datasource metadata for these fields may receive `undefined`, leading to runtime errors or a test that doesn't faithfully match real control panel state. Populate the mocked `columns` and `metrics` arrays to align with the control values. 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/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts: ########## @@ -0,0 +1,100 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ControlPanelState } from '../../src/types'; +import { xAxisSortControl } from '../../src/shared-controls/customControls'; + +const createState = ( + overrides: Partial<ControlPanelState>, +): ControlPanelState => + ({ + slice: { slice_id: 1 }, + form_data: {}, + datasource: { + column_formats: {}, + verbose_map: {}, + }, + controls: {}, + common: {}, + metadata: {}, + ...overrides, + }) as ControlPanelState; + +const createControlState = (value: unknown = undefined) => ({ value }) as any; + +test('xAxisSortControl includes axis and metric options when there is no dimension', () => { + const state = createState({ + controls: { + x_axis: { value: 'ds', type: 'Select' }, + groupby: { value: [], type: 'Select' }, + metrics: { value: ['metric_1'], type: 'Select' }, + timeseries_limit_metric: { value: 'sort_metric', type: 'Select' }, + datasource: { + datasource: { + columns: [], + metrics: [], Review Comment: **Suggestion:** In the first test, the control state references an x-axis column (`'ds'`) and metrics (`'metric_1'`, `'sort_metric'`), but the mocked `datasource` metadata has empty `columns` and `metrics` arrays; any control logic that looks up these columns/metrics by name in the datasource will get `undefined`, causing errors or making the test fail to exercise the real behavior. Populate the mocked `columns` and `metrics` with entries matching the control values. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Tests for xAxisSortControl may be flaky. - ❌ Incorrect test assertions hide X-Axis sort regressions. - ⚠️ Dev CI runs produce misleading failures. ``` </details> ```suggestion columns: [{ column_name: 'ds' }], metrics: [ { metric_name: 'metric_1' }, { metric_name: 'sort_metric' }, ], ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run unit tests that include this file: superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts (execute `yarn test` or `jest`). 2. Test execution starts the first test at line 41 which calls createState(...) to build the mocked ControlPanelState (file: superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts:41-56). 3. The test then calls xAxisSortControl.config.mapStateToProps! at line 58; this function (imported from ../../src/shared-controls/customControls) often inspects datasource.datasource.columns and datasource.datasource.metrics to resolve names like 'ds' and 'metric_1'. 4. Because the mocked datasource has columns: [] and metrics: [] (lines 49-51), any lookup by name (e.g., resolving 'ds' or 'metric_1') will return undefined; this either (a) causes mapStateToProps to produce different options than expected (making the assertions at lines 65-66 invalid), or (b) throws when code assumes metadata existence — both outcomes cause test failures or false positives. Reproducing the failure is done by running the single test file and observing mismatched options or an exception from mapStateToProps. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/xAxisSortControl.test.ts **Line:** 50:51 **Comment:** *Logic Error: In the first test, the control state references an x-axis column (`'ds'`) and metrics (`'metric_1'`, `'sort_metric'`), but the mocked `datasource` metadata has empty `columns` and `metrics` arrays; any control logic that looks up these columns/metrics by name in the datasource will get `undefined`, causing errors or making the test fail to exercise the real behavior. Populate the mocked `columns` and `metrics` with entries matching the control values. 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]
