codeant-ai-for-open-source[bot] commented on code in PR #38307:
URL: https://github.com/apache/superset/pull/38307#discussion_r2868622813
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx:
##########
@@ -370,6 +370,74 @@ test('should fallback to formData state when runtime state
not available', () =>
expect(getByTestId('chart-container')).toBeInTheDocument();
});
+test('chart height accounts for description height when expanded', () => {
+ const DESCRIPTION_HEIGHT = 60;
+ const CHART_HEIGHT = 300;
+
+ const loadingChartState = {
+ charts: {
+ ...defaultState.charts,
+ [queryId]: {
+ ...defaultState.charts[queryId],
+ // ChartOverlay renders with an inline height style when loading —
+ // this is the observable proxy for getChartHeight() without real
layout
+ chartStatus: 'loading',
+ },
+ },
+ };
+
+ // Stabilise getHeaderHeight(): emotion injects margin-bottom CSS during
+ // React's commit phase, so getComputedStyle returns different values in
+ // initial renders vs re-renders. Mock it to always return empty so
+ // getHeaderHeight() consistently falls back to DEFAULT_HEADER_HEIGHT.
+ jest.spyOn(window, 'getComputedStyle').mockReturnValue({
+ getPropertyValue: () => '',
+ } as unknown as CSSStyleDeclaration);
+
+ // JSDOM doesn't compute layout, so mock offsetHeight to simulate a real
+ // description element with height
+ jest
+ .spyOn(HTMLElement.prototype, 'offsetHeight', 'get')
+ .mockImplementation(function (this: HTMLElement) {
+ return this.classList.contains('slice_description')
+ ? DESCRIPTION_HEIGHT
+ : 0;
+ });
+
+ const { container: collapsedContainer } = setup(
+ { height: CHART_HEIGHT },
+ { ...loadingChartState },
+ );
+ const heightCollapsed = parseInt(
+ collapsedContainer.querySelector<HTMLDivElement>(
+ '.dashboard-chart > div[style]',
+ )!.style.height,
+ 10,
+ );
+
+ const { container: expandedContainer } = setup(
+ { height: CHART_HEIGHT },
+ {
+ ...loadingChartState,
+ dashboardState: {
+ ...defaultState.dashboardState,
+ expandedSlices: { [queryId]: true },
+ },
+ },
+ );
+ const heightExpanded = parseInt(
+ expandedContainer.querySelector<HTMLDivElement>(
+ '.dashboard-chart > div[style]',
+ )!.style.height,
+ 10,
+ );
+
+ // The chart should shrink by exactly the description's height
+ expect(heightCollapsed - heightExpanded).toBe(DESCRIPTION_HEIGHT);
+
+ jest.restoreAllMocks();
Review Comment:
**Suggestion:** Using `jest.restoreAllMocks()` at the end of this test
restores every spy in the Jest environment, including the
`redux.bindActionCreators` spy created in `beforeAll`, so subsequent tests will
unexpectedly stop using the mocked action creators and may behave differently
or fail; instead, restore only the spies created in this test. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Later Chart tests use real Redux action creators unexpectedly.
- ⚠️ Shared Jest spies from beforeAll not applied consistently.
- ⚠️ Future tests may flake depending on test execution order.
```
</details>
```suggestion
const getComputedStyleSpy = jest
.spyOn(window, 'getComputedStyle')
.mockReturnValue({
getPropertyValue: () => '',
} as unknown as CSSStyleDeclaration);
// JSDOM doesn't compute layout, so mock offsetHeight to simulate a real
// description element with height
const offsetHeightSpy = jest
.spyOn(HTMLElement.prototype, 'offsetHeight', 'get')
.mockImplementation(function (this: HTMLElement) {
return this.classList.contains('slice_description')
? DESCRIPTION_HEIGHT
: 0;
});
const { container: collapsedContainer } = setup(
{ height: CHART_HEIGHT },
{ ...loadingChartState },
);
const heightCollapsed = parseInt(
collapsedContainer.querySelector<HTMLDivElement>(
'.dashboard-chart > div[style]',
)!.style.height,
10,
);
const { container: expandedContainer } = setup(
{ height: CHART_HEIGHT },
{
...loadingChartState,
dashboardState: {
...defaultState.dashboardState,
expandedSlices: { [queryId]: true },
},
},
);
const heightExpanded = parseInt(
expandedContainer.querySelector<HTMLDivElement>(
'.dashboard-chart > div[style]',
)!.style.height,
10,
);
// The chart should shrink by exactly the description's height
expect(heightCollapsed - heightExpanded).toBe(DESCRIPTION_HEIGHT);
getComputedStyleSpy.mockRestore();
offsetHeightSpy.mockRestore();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the Jest test file `Chart.test.tsx` (e.g. `npm test --
Chart.test.tsx`); in its
`beforeAll` block near the top of the file, a spy is created on
`redux.bindActionCreators`
with `jest.spyOn(redux, 'bindActionCreators').mockImplementation(...)`, so
that all
rendered `Chart` components receive the shared mock action creators
(`refreshChart`,
`logEvent`, etc.).
2. Note that the global `afterEach` in `Chart.test.tsx` only calls
`jest.clearAllMocks()`,
which clears call history but intentionally leaves the `bindActionCreators`
spy and its
mock implementation active for all tests in this file.
3. During the execution of the new test `chart height accounts for
description height when
expanded` (lines 373–439 in `Chart.test.tsx`), the test finishes by calling
`jest.restoreAllMocks()` (line 438), which restores every Jest spy in this
environment,
including the `redux.bindActionCreators` spy created in `beforeAll`,
reverting it to the
real implementation.
4. All subsequently defined tests in the same file (e.g. `should not show a
close button
on chart error banners` and the chart state converter tests that follow this
block) now
run with the original, unmocked `redux.bindActionCreators`; any new tests
added after this
one that expect the shared mock action creators from `beforeAll` would
instead receive
real Redux-bound functions, leading to unexpected behavior or failing
assertions.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx
**Line:** 393:438
**Comment:**
*Logic Error: Using `jest.restoreAllMocks()` at the end of this test
restores every spy in the Jest environment, including the
`redux.bindActionCreators` spy created in `beforeAll`, so subsequent tests will
unexpectedly stop using the mocked action creators and may behave differently
or fail; instead, restore only the spies created in this test.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38307&comment_hash=2aa0ca9751eba2e8d7d10343dc7c1fc4c01279ec1b8515da83851c3109d8497f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38307&comment_hash=2aa0ca9751eba2e8d7d10343dc7c1fc4c01279ec1b8515da83851c3109d8497f&reaction=dislike'>👎</a>
--
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]
