sadpandajoe commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4383010062
##########
superset-frontend/src/components/Chart/ChartRenderer.refresh.test.tsx:
##########
@@ -164,7 +230,735 @@ test('shows the new data once the auto-refresh query
returns', async () => {
/>,
);
- expect(await screen.findByText('value=2')).toBeInTheDocument();
+ expect(await screen.findByText(/value=2/)).toBeInTheDocument();
+});
+
+test('keeps a dashboard chart on screen while a manual refresh query is in
flight', async () => {
+ const { rerender } = render(
+ <ChartRenderer {...props({ chartStatus: 'rendered' })} />,
+ );
+ await screen.findByTestId('refresh-test-chart');
+
+ rerender(<ChartRenderer {...props({ chartStatus: 'loading' })} />);
+
+ expect(await screen.findByTestId('refresh-test-chart')).toHaveTextContent(
+ 'value=1',
+ );
+ expect(screen.queryByText(NO_RESULTS)).not.toBeInTheDocument();
+
+ rerender(
+ <ChartRenderer
+ {...props({
+ chartStatus: 'success',
+ queriesResponse: queriesResponse(2),
+ })}
+ />,
+ );
+
+ expect(await screen.findByText(/value=2/)).toBeInTheDocument();
+});
+
+// A dashboard filter change, such as a Dynamic Group By, updates formData as
+// soon as the new query starts. The previous rows don't match it. Chart tracks
+// which form data the response in the store was queried with.
+const labelledFormData = (label: string) =>
+ ({
+ datasource: '1__table',
+ viz_type: VIZ_TYPE,
+ label,
+ }) as ChartRendererProps['formData'];
+
+const dashboardChartProps = {
+ chartId: 1,
+ dashboardId: 17,
+ width: 600,
+ height: 400,
+ vizType: VIZ_TYPE,
+ setControlValue: jest.fn(),
+ actions: {
+ chartRenderingSucceeded: jest.fn(),
+ chartRenderingFailed: jest.fn(),
+ logEvent: jest.fn(),
+ postChartFormData: jest.fn(),
+ } as unknown as Actions,
+};
+
+test('draws each previous response with the form data it was queried with',
async () => {
+ // The store keeps the same response object until the new one lands, and
+ // latestQueryFormData is the form data of the request that produced it.
+ const first = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={labelledFormData('first')}
+ latestQueryFormData={labelledFormData('first')}
+ queriesResponse={first}
+ />,
+ );
+ expect(await screen.findByTestId('refresh-test-chart')).toHaveTextContent(
+ 'label=first',
+ );
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('second')}
+ latestQueryFormData={labelledFormData('second')}
+ queriesResponse={first}
+ />,
+ );
+ expect(await screen.findByTestId('refresh-test-chart')).toHaveTextContent(
+ 'value=1 label=first',
+ );
+
+ const second = queriesResponse(2);
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="success"
+ formData={labelledFormData('second')}
+ latestQueryFormData={labelledFormData('second')}
+ queriesResponse={second}
+ />,
+ );
+ expect(await screen.findByText('value=2 label=second')).toBeInTheDocument();
+
+ // The second refresh draws the second response with its own form data, not
+ // with the first query's.
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('third')}
+ latestQueryFormData={labelledFormData('third')}
+ queriesResponse={second}
+ />,
+ );
+ expect(await screen.findByTestId('refresh-test-chart')).toHaveTextContent(
+ 'value=2 label=second',
+ );
+});
+
+test('pairs a response that lands while the chart is off screen with the
request that produced it', async () => {
+ // With deferred data, a chart off screen starts no new query, but a query
+ // already in flight keeps running and its response lands against the newer
+ // form data.
+ const previousFlags = window.featureFlags;
+ window.featureFlags = {
+ ...previousFlags,
+ [FeatureFlag.DashboardVirtualization]: true,
+ [FeatureFlag.DashboardVirtualizationDeferData]: true,
+ };
+ try {
+ const first = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={labelledFormData('country')}
+ latestQueryFormData={labelledFormData('country')}
+ queriesResponse={first}
+ />,
+ );
+ await screen.findByTestId('refresh-test-chart');
+
+ // A country refresh is in flight when the chart scrolls out of view.
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ isInView={false}
+ chartStatus="loading"
+ formData={labelledFormData('country')}
+ latestQueryFormData={labelledFormData('country')}
+ queriesResponse={first}
+ />,
+ );
+ // Dynamic Group By moves to region. The region query is deferred, so the
+ // request's form data stays country.
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ isInView={false}
+ chartStatus="loading"
+ formData={labelledFormData('region')}
+ latestQueryFormData={labelledFormData('country')}
+ queriesResponse={first}
+ />,
+ );
+ // The country response lands.
+ const country = queriesResponse(2);
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ isInView={false}
+ chartStatus="success"
+ formData={labelledFormData('region')}
+ latestQueryFormData={labelledFormData('country')}
+ queriesResponse={country}
+ />,
+ );
+ // Back in view, the region query starts and the country rows are drawn
+ // with the country form data until it lands.
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('region')}
+ latestQueryFormData={labelledFormData('region')}
+ queriesResponse={country}
+ />,
+ );
+ expect(await screen.findByTestId('refresh-test-chart')).toHaveTextContent(
+ 'value=2 label=country',
+ );
+ } finally {
+ window.featureFlags = previousFlags;
+ }
+});
+
+test('builds the context menu from the form data the chart is drawn with while
it refreshes', async () => {
+ // A drill from the context menu has to match the rows on screen, which
+ // keep the previous grouping until the new response lands.
+ const previousFlags = window.featureFlags;
+ window.featureFlags = {
+ ...previousFlags,
+ [FeatureFlag.DrillToDetail]: true,
+ };
+ try {
+ const first = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={labelledFormData('country')}
+ latestQueryFormData={labelledFormData('country')}
+ queriesResponse={first}
+ />,
+ );
+ await screen.findByTestId('refresh-test-chart');
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('region')}
+ latestQueryFormData={labelledFormData('region')}
+ queriesResponse={first}
+ />,
+ );
+ expect(screen.getByTestId('chart-context-menu')).toHaveAttribute(
+ 'data-label',
+ 'country',
+ );
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="success"
+ formData={labelledFormData('region')}
+ latestQueryFormData={labelledFormData('region')}
+ queriesResponse={queriesResponse(2)}
+ />,
+ );
+ expect(await screen.findByText('value=2
label=region')).toBeInTheDocument();
+ expect(screen.getByTestId('chart-context-menu')).toHaveAttribute(
+ 'data-label',
+ 'region',
+ );
+ } finally {
+ window.featureFlags = previousFlags;
+ }
+});
+
+test('draws the previous data with the values the chart set on itself', async
() => {
+ // A chart sets controls on itself through setControlValue, which a dashboard
+ // merges into the form data without a new query. A refresh then re-queries
+ // with the form data of the previous request.
+ const previous = queriesResponse(1);
+ const withDisplay = (stack: boolean, zoom: number) =>
+ ({
+ ...labelledFormData('before'),
+ stack,
+ viewport: { zoom },
+ }) as ChartRendererProps['formData'];
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={withDisplay(false, 1)}
+ latestQueryFormData={withDisplay(false, 1)}
+ queriesResponse={previous}
+ />,
+ );
+ expect(
+ await screen.findByTestId('refresh-test-chart-display'),
+ ).toHaveTextContent('stack=false zoom=1');
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={withDisplay(true, 4)}
+ latestQueryFormData={withDisplay(false, 1)}
+ queriesResponse={previous}
+ />,
+ );
+ expect(
+ await screen.findByTestId('refresh-test-chart-display'),
+ ).toHaveTextContent('stack=true zoom=4');
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={withDisplay(true, 4)}
+ latestQueryFormData={withDisplay(false, 1)}
+ queriesResponse={previous}
+ />,
+ );
+ expect(screen.getByTestId('refresh-test-chart-display')).toHaveTextContent(
+ 'stack=true zoom=4',
+ );
+ expect(screen.getByTestId('refresh-test-chart')).toHaveTextContent(
+ 'value=1 label=before',
+ );
+});
+
+test('draws the previous data with its own form data when a virtualized chart
scrolls back into view', async () => {
+ const previousFlags = window.featureFlags;
+ window.featureFlags = {
+ ...previousFlags,
+ [FeatureFlag.DashboardVirtualization]: true,
+ };
+ try {
+ const previous = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={labelledFormData('before')}
+ queriesResponse={previous}
+ />,
+ );
+ await screen.findByTestId('refresh-test-chart');
+
+ // Out of view, ChartRenderer unmounts while the filter change starts the
+ // new query.
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ isInView={false}
+ chartStatus="loading"
+ formData={labelledFormData('after')}
+ queriesResponse={previous}
+ />,
+ );
+ expect(screen.queryByTestId('refresh-test-chart')).not.toBeInTheDocument();
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('after')}
+ queriesResponse={previous}
+ />,
+ );
+ expect(await screen.findByTestId('refresh-test-chart')).toHaveTextContent(
+ 'value=1 label=before',
+ );
+ } finally {
+ window.featureFlags = previousFlags;
+ }
+});
+
+test('draws the previous data with the current dashboard colour scheme', async
() => {
+ // Changing the dashboard colour scheme doesn't run a new query, so formData
+ // moves on while the response in the store stays the same.
+ const previous = queriesResponse(1);
+ const withScheme = (colorScheme: string) =>
+ ({
+ ...labelledFormData('before'),
+ color_scheme: colorScheme,
+ }) as ChartRendererProps['formData'];
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={withScheme('supersetColors')}
+ queriesResponse={previous}
+ />,
+ );
+ expect(
+ await screen.findByTestId('refresh-test-chart-scheme'),
+ ).toHaveTextContent('supersetColors');
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={withScheme('bnbColors')}
+ queriesResponse={previous}
+ />,
+ );
+ expect(
+ await screen.findByTestId('refresh-test-chart-scheme'),
+ ).toHaveTextContent('bnbColors');
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={withScheme('bnbColors')}
+ queriesResponse={previous}
+ />,
+ );
+ expect(screen.getByTestId('refresh-test-chart-scheme')).toHaveTextContent(
+ 'bnbColors',
+ );
+});
+
+test('draws the previous data without a colour setting cleared on the
dashboard', async () => {
+ // The response was queried with a colour scheme that the dashboard then
+ // drops without a new query.
+ const previous = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={
+ {
+ ...labelledFormData('before'),
+ color_scheme: 'supersetColors',
+ } as ChartRendererProps['formData']
+ }
+ queriesResponse={previous}
+ />,
+ );
+ expect(
+ await screen.findByTestId('refresh-test-chart-scheme'),
+ ).toHaveTextContent('supersetColors');
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={labelledFormData('before')}
+ queriesResponse={previous}
+ />,
+ );
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('before')}
+ queriesResponse={previous}
+ />,
+ );
+ await waitForChartLoad();
+
+
expect(screen.getByTestId('refresh-test-chart')).toHaveTextContent('value=1');
+
expect(screen.getByTestId('refresh-test-chart-scheme')).toBeEmptyDOMElement();
+});
+
+test('shows the regular spinner for a dashboard chart mounted while its query
is in flight', async () => {
+ // Nothing tells this chart which form data the response in the store was
+ // queried with, so it can't draw that response.
+ render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('after')}
+ queriesResponse={queriesResponse(1)}
+ />,
+ );
+ await waitForChartLoad();
+
+ expect(screen.queryByTestId('refresh-test-chart')).not.toBeInTheDocument();
+ expect(screen.getByText('Waiting on database...')).toBeInTheDocument();
+});
+
+test('does not redraw a dashboard chart under the spinner when a refresh
starts with nothing changed', async () => {
+ // A refresh sends latestQueryFormData again, so the store keeps both form
+ // data objects as they are. The form data the chart is drawn with is built
+ // from them while it refreshes, and an ECharts chart replays its entrance
+ // animation each time the plugin transforms it again. The dashboard also
+ // passes its filter callbacks memoized.
+ const chartProps = {
+ ...dashboardChartProps,
+ addFilter: jest.fn(),
+ onFilterMenuOpen: jest.fn(),
+ onFilterMenuClose: jest.fn(),
+ };
+ const formData = labelledFormData('first');
+ const latestQueryFormData = labelledFormData('first');
+ const response = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...chartProps}
+ chartStatus="rendered"
+ formData={formData}
+ latestQueryFormData={latestQueryFormData}
+ queriesResponse={response}
+ />,
+ );
+ await screen.findByTestId('refresh-test-chart');
+ const transformsBeforeRefresh = testChartTransformed.mock.calls.length;
+
+ rerender(
+ <Chart
+ {...chartProps}
+ chartStatus="loading"
+ formData={formData}
+ latestQueryFormData={latestQueryFormData}
+ queriesResponse={response}
+ />,
+ );
+ await waitForChartLoad();
+
+ expect(screen.getByTestId('refresh-test-chart')).toHaveTextContent(
+ 'value=1 label=first',
+ );
+ expect(screen.getByRole('status')).toBeInTheDocument();
+ expect(testChartTransformed).toHaveBeenCalledTimes(transformsBeforeRefresh);
+
+ const second = queriesResponse(2);
+ rerender(
+ <Chart
+ {...chartProps}
+ chartStatus="success"
+ formData={formData}
+ latestQueryFormData={latestQueryFormData}
+ queriesResponse={second}
+ />,
+ );
+ expect(await screen.findByText('value=2 label=first')).toBeInTheDocument();
+
+ // A filter change: the request starts with the new form data, and the chart
+ // is drawn with it once its response lands.
+ const filteredFormData = labelledFormData('second');
+ const filteredQueryFormData = labelledFormData('second');
+ const third = queriesResponse(3);
+ rerender(
+ <Chart
+ {...chartProps}
+ chartStatus="loading"
+ formData={filteredFormData}
+ latestQueryFormData={filteredQueryFormData}
+ queriesResponse={second}
+ />,
+ );
+ rerender(
+ <Chart
+ {...chartProps}
+ chartStatus="success"
+ formData={filteredFormData}
+ latestQueryFormData={filteredQueryFormData}
+ queriesResponse={third}
+ />,
+ );
+ expect(await screen.findByText('value=3 label=second')).toBeInTheDocument();
+
+ // The next refresh with nothing changed compares against the form data the
+ // chart was last drawn with, not the one it was first drawn with.
+ const transformsBeforeSecondRefresh = testChartTransformed.mock.calls.length;
+ rerender(
+ <Chart
+ {...chartProps}
+ chartStatus="loading"
+ formData={filteredFormData}
+ latestQueryFormData={filteredQueryFormData}
+ queriesResponse={third}
+ />,
+ );
+ await waitForChartLoad();
+
+ expect(screen.getByRole('status')).toBeInTheDocument();
+ expect(testChartTransformed).toHaveBeenCalledTimes(
+ transformsBeforeSecondRefresh,
+ );
+});
+
+test('does not redraw a dashboard chart under the spinner when its query
records new form data objects', async () => {
+ // A query started from the chart itself sends formData, and the store
+ // records it as latestQueryFormData: a different object from the one the
+ // response was queried with, with the same values.
+ const chartProps = {
+ ...dashboardChartProps,
+ addFilter: jest.fn(),
+ onFilterMenuOpen: jest.fn(),
+ onFilterMenuClose: jest.fn(),
+ };
+ const formData = labelledFormData('first');
+ const response = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...chartProps}
+ chartStatus="rendered"
+ formData={formData}
+ latestQueryFormData={labelledFormData('first')}
+ queriesResponse={response}
+ />,
+ );
+ await screen.findByTestId('refresh-test-chart');
+ const transformsBeforeRefresh = testChartTransformed.mock.calls.length;
+
+ rerender(
+ <Chart
+ {...chartProps}
+ chartStatus="loading"
+ formData={formData}
+ latestQueryFormData={formData}
+ queriesResponse={response}
+ />,
+ );
+ await waitForChartLoad();
+
+ expect(screen.getByRole('status')).toBeInTheDocument();
+ expect(testChartTransformed).toHaveBeenCalledTimes(transformsBeforeRefresh);
+});
+
+const selfFetchingChartProps = {
+ ...dashboardChartProps,
+ vizType: SELF_FETCHING_VIZ_TYPE,
+ formData: {
+ datasource: '1__table',
+ viz_type: SELF_FETCHING_VIZ_TYPE,
+ } as ChartRendererProps['formData'],
+};
+
+test('replaces a self-fetching dashboard chart with the spinner through a
manual refresh', async () => {
+ // Its own query lands first and its data reloads after that, so kept on
+ // screen it would show the previous data with no spinner while it reloads.
+ const { rerender } = render(
+ <Chart
+ {...selfFetchingChartProps}
+ chartStatus="rendered"
+ queriesResponse={[]}
+ />,
+ );
+ await screen.findByTestId('self-fetching-test-chart');
+
+ rerender(
+ <Chart
+ {...selfFetchingChartProps}
+ chartStatus="loading"
+ queriesResponse={[]}
+ />,
+ );
+ expect(
+ screen.queryByTestId('self-fetching-test-chart'),
+ ).not.toBeInTheDocument();
+ expect(screen.getByText('Waiting on database...')).toBeInTheDocument();
+
+ rerender(
+ <Chart
+ {...selfFetchingChartProps}
+ chartStatus="success"
+ queriesResponse={[]}
+ />,
+ );
+ expect(
+ await screen.findByTestId('self-fetching-test-chart'),
+ ).toBeInTheDocument();
+ // Mounting again is what reloads its data.
+ expect(selfFetchingChartMounted).toHaveBeenCalledTimes(2);
+});
+
+test('keeps a self-fetching dashboard chart mounted through an auto-refresh',
async () => {
+ const { rerender } = render(
+ <Chart
+ {...selfFetchingChartProps}
+ chartStatus="rendered"
+ queriesResponse={[]}
+ />,
+ );
+ await screen.findByTestId('self-fetching-test-chart');
+
+ rerender(
+ <Chart
+ {...selfFetchingChartProps}
+ chartStatus="loading"
+ queriesResponse={[]}
+ suppressLoadingSpinner
+ />,
+ );
+ expect(screen.getByTestId('self-fetching-test-chart')).toBeInTheDocument();
+
+ rerender(
+ <Chart
+ {...selfFetchingChartProps}
+ chartStatus="success"
+ queriesResponse={[]}
+ suppressLoadingSpinner
+ />,
+ );
+ expect(screen.getByTestId('self-fetching-test-chart')).toBeInTheDocument();
+ expect(selfFetchingChartMounted).toHaveBeenCalledTimes(1);
+});
+
+// A plugin that throws while rendering leaves its response in the store with
+// no query error, and only the chart status and alert record the failure.
+test.each([
+ ['a manual refresh', false],
+ ['an auto-refresh', true],
+])(
+ 'does not draw a response that failed to render again during %s',
+ async (_, suppressLoadingSpinner) => {
+ const previous = queriesResponse(1);
+ const { rerender } = render(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="rendered"
+ formData={labelledFormData('before')}
+ queriesResponse={previous}
+ />,
+ );
+ await screen.findByTestId('refresh-test-chart');
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="failed"
+ chartAlert="An error occurred while rendering the visualization"
+ formData={labelledFormData('before')}
+ queriesResponse={previous}
+ />,
+ );
+ expect(screen.queryByTestId('refresh-test-chart')).not.toBeInTheDocument();
+
+ rerender(
+ <Chart
+ {...dashboardChartProps}
+ chartStatus="loading"
+ formData={labelledFormData('before')}
+ queriesResponse={previous}
+ suppressLoadingSpinner={suppressLoadingSpinner}
+ />,
+ );
+ await waitForChartLoad();
+
+ expect(screen.queryByTestId('refresh-test-chart')).not.toBeInTheDocument();
Review Comment:
This test only asserts the old chart is gone after a render failure, so it
would stay green if `showSpinner` in `Chart.tsx` stopped covering a chart whose
`responseFormData` was dropped (for example `responseFormData !== undefined &&
!canDrawPreviousResponse`). An auto-refresh after a failed render would then
leave an empty box until the new response lands, which is the case the fallback
was added for. The Matrixify auto-refresh test in `Chart.test.tsx` doesn't
reach it because there the response form data is still known. Could this also
assert that the loading status (`Waiting on database...`) is shown after the
final rerender, at least for the auto-refresh case?
--
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]
