EnxDev commented on code in PR #44272:
URL: https://github.com/apache/superset/pull/44272#discussion_r4074864552
##########
superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx:
##########
@@ -152,3 +158,23 @@ test('forceRefresh normalizes a negative config value to 0
(unstaggered)', async
expect.objectContaining({ interval: 0 }),
);
});
+
+test('a silent refresh reports only the affected chart ids to
startAutoRefresh, not the whole dashboard', async () => {
+ mockStartAutoRefresh.mockClear();
+ mockUseAutoRefreshTabPause.mockClear();
+ const { props } = renderHeaderAutoRefresh(
+ {},
+ { chartIds: [1, 2], timedRefreshImmuneSlices: [2] },
Review Comment:
Could we also cover a chart that's in `chartIds` but has an empty
`latestQueryFormData`, as an unvisited tab would? This case checks the
immune-slice filter, while the first-load fix also relies on the later filter
in `executeRefresh`. A third chart with no previous query data, asserting that
both `startAutoRefresh` and `onRefresh` receive only the eligible IDs, would
protect that part of the reported scenario. Non-blocking; the implementation
already handles it.
##########
superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx:
##########
@@ -152,3 +158,23 @@ test('forceRefresh normalizes a negative config value to 0
(unstaggered)', async
expect.objectContaining({ interval: 0 }),
);
});
+
+test('a silent refresh reports only the affected chart ids to
startAutoRefresh, not the whole dashboard', async () => {
+ mockStartAutoRefresh.mockClear();
+ mockUseAutoRefreshTabPause.mockClear();
+ const { props } = renderHeaderAutoRefresh(
+ {},
+ { chartIds: [1, 2], timedRefreshImmuneSlices: [2] },
+ );
+
+ const { onRefresh: handleTabVisibilityRefresh } =
+ mockUseAutoRefreshTabPause.mock.calls[0][0];
+
+ await act(async () => {
+ await handleTabVisibilityRefresh();
+ });
+
+ expect(props.onRefresh).toHaveBeenCalledTimes(1);
+ expect(mockStartAutoRefresh).toHaveBeenCalledWith([1]);
+ expect(mockStartAutoRefresh).not.toHaveBeenCalledWith([1, 2]);
+});
Review Comment:
I checked this at 2ab4222 with Node 24: the new test completes, and all five
tests in `useHeaderAutoRefresh.test.tsx` pass without a RAF mock. The Jest
jsdom environment enables `pretendToBeVisual`, which supplies
`requestAnimationFrame`; the custom environment inherits that setup. I don't
think we need an additional mock for this test.
--
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]
