codeant-ai-for-open-source[bot] commented on code in PR #37107:
URL: https://github.com/apache/superset/pull/37107#discussion_r2743380460
##########
superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx:
##########
@@ -56,90 +56,99 @@ const mockExportCurrentViewBehavior = () => {
} as any);
};
-const createProps = (additionalProps = {}) => ({
- chart: {
- id: 1,
- latestQueryFormData: {
- viz_type: VizType.Histogram,
- datasource: '49__table',
- slice_id: 318,
- url_params: {},
- granularity_sqla: 'time_start',
- time_range: 'No filter',
- all_columns_x: ['age'],
- adhoc_filters: [],
- row_limit: 10000,
- groupby: null,
- color_scheme: 'supersetColors',
- label_colors: {},
- link_length: '25',
- x_axis_label: 'age',
- y_axis_label: 'count',
- server_pagination: false as any,
+const createProps = (additionalProps = {}) =>
+ ({
+ chart: {
+ id: 1,
+ latestQueryFormData: {
+ viz_type: VizType.Histogram,
+ datasource: '49__table',
+ slice_id: 318,
+ url_params: {},
+ granularity_sqla: 'time_start',
+ time_range: 'No filter',
+ all_columns_x: ['age'],
+ adhoc_filters: [],
+ row_limit: 10000,
+ groupby: null,
+ color_scheme: 'supersetColors',
+ label_colors: {},
+ link_length: '25',
+ x_axis_label: 'age',
+ y_axis_label: 'count',
+ server_pagination: false,
+ },
+ chartStatus: 'rendered' as const,
+ chartAlert: null,
+ chartUpdateEndTime: null,
+ chartUpdateStartTime: 0,
+ lastRendered: 0,
+ sliceFormData: null,
+ queryController: null,
+ queriesResponse: null,
+ triggerQuery: false,
},
- chartStatus: 'rendered',
- },
- slice: {
- cache_timeout: null,
- changed_on: '2021-03-19T16:30:56.750230',
- changed_on_humanized: '7 days ago',
- datasource: 'FCC 2018 Survey',
- description: 'Simple description',
- description_markeddown: '',
- edit_url: '/chart/edit/318',
- form_data: {
- adhoc_filters: [],
- all_columns_x: ['age'],
- color_scheme: 'supersetColors',
- datasource: '49__table',
- granularity_sqla: 'time_start',
- groupby: null,
- label_colors: {},
- link_length: '25',
- queryFields: { groupby: 'groupby' },
- row_limit: 10000,
+ slice: {
+ cache_timeout: null,
+ changed_on: '2021-03-19T16:30:56.750230',
+ changed_on_humanized: '7 days ago',
+ datasource: 'FCC 2018 Survey',
+ description: 'Simple description',
+ description_markeddown: '',
+ edit_url: '/chart/edit/318',
+ form_data: {
+ adhoc_filters: [],
+ all_columns_x: ['age'],
+ color_scheme: 'supersetColors',
+ datasource: '49__table',
+ granularity_sqla: 'time_start',
+ groupby: null,
+ label_colors: {},
+ link_length: '25',
+ queryFields: { groupby: 'groupby' },
+ row_limit: 10000,
+ slice_id: 318,
+ time_range: 'No filter',
+ url_params: {},
+ viz_type: VizType.Histogram,
+ x_axis_label: 'age',
+ y_axis_label: 'count',
+ },
+ modified: '<span class="no-wrap">7 days ago</span>',
+ owners: [
+ {
+ text: 'Superset Admin',
+ value: 1,
+ },
+ ],
Review Comment:
**Suggestion:** The test fixture's `slice.owners` is an array of `{ text,
value }` objects, but the component expects an array of owner IDs (`number[]`),
so ownership checks using `includes(user.userId)` will always fail in tests,
meaning edit permissions cannot be realistically exercised and potential bugs
around ownership may be masked. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Tests may not exercise owner-restricted UI paths.
- ⚠️ Permission-related assertions are silently ineffective.
- ⚠️ Component edit-permissions logic not validated.
```
</details>
```suggestion
owners: [1],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test suite that includes ExploreChartHeader tests (file:
superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx).
2. The test fixture is created by createProps() in this file (createProps
definition spans
the lines around the slice fixture at lines 59–152). Inside that fixture the
slice.owners
value is the array-of-objects literal shown at lines 118–123.
3. When the ExploreHeader component code checks ownership it typically
compares owner IDs
(number) to the current user's id via code paths that call something like
owners.includes(user.userId) or similar (component consumer code expects
owner id array).
Because the test fixture supplies objects ({ text, value }) instead of plain
numeric IDs
the includes(...) check will not match (object !== number) and
ownership-guarded UI will
remain disabled in the test.
4. As a result tests that attempt to exercise owner-restricted behavior
(e.g., editing or
starring when owned) will not take the intended branch and will silently
pass or miss
exercising those code paths. This masking happens in real test runs of this
file because
other tests in the same file rely on createProps() and the owners fixture
(verify:
createProps usage across tests in this file).
Note: The existing pattern in the fixture (object with text/value) appears
to be a
realistic mismatch with the component's runtime expectation; changing to
owners: [1] makes
the test reflect real app data shape.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
**Line:** 118:123
**Comment:**
*Logic Error: The test fixture's `slice.owners` is an array of `{ text,
value }` objects, but the component expects an array of owner IDs (`number[]`),
so ownership checks using `includes(user.userId)` will always fail in tests,
meaning edit permissions cannot be realistically exercised and potential bugs
around ownership may be masked.
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/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx:
##########
@@ -168,27 +177,40 @@ describe('ExploreChartHeader', () => {
const props = createProps();
render(<ExploreHeader {...props} />, { useRedux: true });
const newChartName = 'New chart name';
- const prevChartName = props.slice_name;
+ const prevChartName = props.sliceName;
+
+ // Wait for the component to render with the chart title
expect(
- await screen.findByText(/add the name of the chart/i),
+ await screen.findByDisplayValue(prevChartName ?? ''),
).toBeInTheDocument();
- userEvent.click(screen.getByLabelText('Menu actions trigger'));
- userEvent.click(screen.getByText('Edit chart properties'));
+ await userEvent.click(screen.getByLabelText('Menu actions trigger'));
+ await userEvent.click(screen.getByText('Edit chart properties'));
const nameInput = await screen.findByRole('textbox', { name: 'Name' });
- userEvent.clear(nameInput);
- userEvent.type(nameInput, newChartName);
+ await userEvent.clear(nameInput);
+ await userEvent.type(nameInput, newChartName);
expect(screen.getByDisplayValue(newChartName)).toBeInTheDocument();
- userEvent.click(screen.getByRole('button', { name: 'Cancel' }));
+ await userEvent.click(screen.getByRole('button', { name: 'Cancel' }));
Review Comment:
**Suggestion:** The `afterEach` block unconditionally calls `.restore()` on
the Sinon spies, but several tests also call `.restore()` manually, so when
`afterEach` runs it can attempt to restore an already-restored spy and throw,
causing unrelated tests to fail; guarding the restore calls prevents this
double-restore error. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ afterEach double-restore can break unrelated tests.
- ⚠️ CI flakiness and intermittent failures.
- ⚠️ Test-suite instability during spy cleanup.
```
</details>
```suggestion
if (spyDownloadAsImage && typeof spyDownloadAsImage.restore ===
'function') {
spyDownloadAsImage.restore();
}
if (spyExportChart && typeof spyExportChart.restore === 'function') {
spyExportChart.restore();
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the tests in this file
(superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx).
Several tests inside the "Export All Data" suite and other suites create
sinon spies in
beforeEach (e.g., spyDownloadAsImage = sinon.spy(downloadAsImage, 'default');
spyExportChart = sinon.spy(exploreUtils, 'exportChart');).
2. Some individual tests call spyExportChart.restore() or
spyExportChart.restore()
explicitly inside the test body (for example, tests labelled 'Should not
export to CSV if
canDownload=false' and 'Should export to CSV if canDownload=true' include
spyExportChart.restore()).
3. After a test that already restored the spy finishes, the file-level
afterEach() runs
and unconditionally calls spyExportChart.restore() and
spyDownloadAsImage.restore() again
(lines ~196–200). Restoring an already-restored sinon spy can throw
"TypeError:
spy.restore is not a function" or sinon's "already restored" error depending
on sinon
version.
4. The thrown error in afterEach would cause the test run to fail or abort
subsequent
tests, surfacing unrelated failures and flakiness.
5. Guarding the restore calls (check existence and function) prevents
double-restore and
makes cleanup idempotent.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
**Line:** 197:197
**Comment:**
*Possible Bug: The `afterEach` block unconditionally calls `.restore()`
on the Sinon spies, but several tests also call `.restore()` manually, so when
`afterEach` runs it can attempt to restore an already-restored spy and throw,
causing unrelated tests to fail; guarding the restore calls prevents this
double-restore error.
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]
