rusackas commented on code in PR #42530: URL: https://github.com/apache/superset/pull/42530#discussion_r3664595503
########## superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx: ########## @@ -0,0 +1,186 @@ +/** + * 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 { act, render } from '@testing-library/react'; +import { + CategoricalColorNamespace, + getLabelsColorMap, +} from '@superset-ui/core'; +import { supersetTheme, ThemeProvider } from '@apache-superset/core/theme'; +import EchartsRose from '../../src/Rose/EchartsRose'; +import { RoseChartTransformedProps, RosePeriod } from '../../src/Rose/types'; +import Echart from '../../src/components/Echart'; + +jest.mock('../../src/components/Echart', () => ({ + __esModule: true, + default: jest.fn(() => null), +})); + +const mockedEchart = jest.mocked(Echart); + +// Entries are NOT in seriesNames order -- East/West/North -- deliberately, +// to prove colors come from priming the scale in seriesNames order rather +// than from whatever order the drilled pie happens to sort slices into. +const SERIES_NAMES = ['East', 'West', 'North']; +const PERIODS: RosePeriod[] = [ + { + time: 1, + label: '2021', + entries: [ + { seriesName: 'West', value: 5, increment: 0.1 }, + { seriesName: 'North', value: 30, increment: 0.5 }, + { seriesName: 'East', value: 10, increment: 0.3 }, + ], + }, + { + time: 2, + label: '2022', + entries: [ + { seriesName: 'East', value: 7, increment: 0.2 }, + { seriesName: 'West', value: 3, increment: 0.1 }, + { seriesName: 'North', value: 1, increment: 0.05 }, + ], + }, +]; + +const ROSE_ECHART_OPTIONS = { + legend: { data: SERIES_NAMES }, + series: [{ id: 'rose', type: 'bar' }], +}; + +const baseProps: RoseChartTransformedProps = { + height: 400, + width: 800, + echartOptions: ROSE_ECHART_OPTIONS as any, + refs: {}, + formData: { vizType: 'rose' } as any, + periods: PERIODS, + seriesNames: SERIES_NAMES, + numberFormat: 'SMART_NUMBER', + sliceId: 1, + colorScheme: '', + groupby: [], + labelMap: {}, + setDataMask: jest.fn(), + selectedValues: {}, + emitCrossFilters: false, +}; + +function lastEchartCall() { + return mockedEchart.mock.calls[mockedEchart.mock.calls.length - 1][0] as any; +} + +function renderRose(props: RoseChartTransformedProps = baseProps) { + return render( + <ThemeProvider theme={supersetTheme}> + <EchartsRose {...props} /> + </ThemeProvider>, + ); +} + +beforeEach(() => { + mockedEchart.mockClear(); + // getColor() remembers a label's color for a given sliceId in a + // process-wide singleton (so a dashboard's colors stay consistent across + // charts); without resetting it here, whichever test runs first would + // permanently decide these labels' colors for every later test in this + // file, masking a priming-order regression instead of catching it. + getLabelsColorMap().reset(); +}); + +test('renders the rose view unchanged until a period is clicked', () => { + renderRose(); + + expect(lastEchartCall().echartOptions).toBe(ROSE_ECHART_OPTIONS); +}); + +test('clicking a period drills into a pie of that period, sorted largest-first', () => { + renderRose(); + + act(() => { + lastEchartCall().eventHandlers.click({ dataIndex: 0 }); + }); + + const { echartOptions } = lastEchartCall(); + expect(echartOptions.title.text).toContain('2021'); + expect(echartOptions.series[0].data.map((d: any) => d.name)).toEqual([ + 'North', + 'East', + 'West', + ]); + expect(echartOptions.series[0].data.map((d: any) => d.value)).toEqual([ + 30, 10, 5, + ]); +}); + +test('drilled slice colors match the rose colors, primed in seriesNames order', () => { + // Compute the expected mapping first, from a clean slate, priming a scale + // in seriesNames order exactly as the rose view does -- independent of + // the pie's own value-sorted order (North/East/West by value here, vs. + // East/West/North in seriesNames). + const expectedColorFn = CategoricalColorNamespace.getScale(''); + const expectedColors: Record<string, string> = {}; + SERIES_NAMES.forEach(name => { + expectedColors[name] = expectedColorFn(name, baseProps.sliceId); + }); + + // Reset before rendering so the component primes its own scale from the + // same clean slate, rather than inheriting the mapping just computed + // above (colors are remembered per sliceId in a process-wide singleton, + // so without this reset the comparison below would be tautological). + getLabelsColorMap().reset(); + renderRose(); + act(() => { + lastEchartCall().eventHandlers.click({ dataIndex: 0 }); + }); + const { echartOptions } = lastEchartCall(); + + echartOptions.series[0].data.forEach((slice: any) => { + expect(slice.itemStyle.color).toBe(expectedColors[slice.name]); + }); +}); + +test('clicking again while drilled returns to the rose view', () => { + renderRose(); + + act(() => { + lastEchartCall().eventHandlers.click({ dataIndex: 1 }); + }); + expect(lastEchartCall().echartOptions).not.toBe(ROSE_ECHART_OPTIONS); + + act(() => { + lastEchartCall().eventHandlers.click({ dataIndex: 0 }); + }); + expect(lastEchartCall().echartOptions).toBe(ROSE_ECHART_OPTIONS); +}); + +test('clicking a different period while drilled switches to that period', () => { Review Comment: Fixed, renamed the test to match what it actually asserts (returns to the rose view, not to a second period). ########## superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.test.tsx: ########## @@ -148,346 +164,291 @@ const renderWithProviders = (component: React.ReactElement) => describe('DeckMulti Autozoom Functionality', () => { beforeEach(() => { jest.clearAllMocks(); - (SupersetClient.post as jest.Mock).mockResolvedValue({ - json: { - result: [{ data: [] }], - }, - }); - (SupersetClient.get as jest.Mock).mockResolvedValue({ - json: { - data: { - features: [], - }, - }, - }); + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); }); - test('should NOT apply autozoom when autozoom is false', () => { + test('should NOT apply autozoom when autozoom is false', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); + featuresByVizType = { deck_scatter: [{ position: [1, 1] }] }; const props = { ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: false, - }, + formData: { ...baseMockProps.formData, autozoom: false }, }; renderWithProviders(<DeckMulti {...props} />); - // fitViewport should not be called when autozoom is false + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalled()); expect(fitViewportSpy).not.toHaveBeenCalled(); fitViewportSpy.mockRestore(); }); - test('should apply autozoom when autozoom is true', () => { + test('should apply autozoom to points fetched from each layer when autozoom is true', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); fitViewportSpy.mockReturnValue({ longitude: -122.4, latitude: 37.8, zoom: 10, }); + featuresByVizType = { + deck_scatter: [{ position: [1, 1] }, { position: [2, 2] }], + deck_polygon: [ + { + polygon: [ + [3, 3], + [4, 4], + ], + }, + ], + }; const props = { ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, + formData: { ...baseMockProps.formData, autozoom: true }, }; renderWithProviders(<DeckMulti {...props} />); - // fitViewport should be called with the points from all layers - expect(fitViewportSpy).toHaveBeenCalledWith( - expect.objectContaining({ - longitude: 0, - latitude: 0, - zoom: 1, - }), - expect.objectContaining({ - width: 800, - height: 600, - points: expect.any(Array), - }), + await waitFor(() => { + expect(fitViewportSpy).toHaveBeenCalledWith( + expect.objectContaining({ longitude: 0, latitude: 0, zoom: 1 }), + expect.objectContaining({ + width: 800, + height: 600, + points: expect.any(Array), + }), + ); + }); + // Points from both layers should have been collected by the time the + // second (or later) refit happens -- this exercises the async, per-layer + // accumulation added to fix the "autozoom dead in the v1 path" bug. + const callWithBothLayers = fitViewportSpy.mock.calls.find( + call => call[1].points.length >= 4, ); + expect(callWithBothLayers).toBeDefined(); fitViewportSpy.mockRestore(); }); - test('should use adjusted viewport when autozoom is enabled', async () => { + test('should refit as each layer arrives, even when they resolve out of order', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const adjustedViewport = { - longitude: -122.4, - latitude: 37.8, - zoom: 12, - }; - fitViewportSpy.mockReturnValue(adjustedViewport); - - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, + fitViewportSpy.mockReturnValue({ longitude: 0, latitude: 0, zoom: 8 }); + + // The polygon layer (slice 2) resolves before the scatter layer (slice 1), + // the opposite of deck_slices order -- accumulation must not assume + // layers arrive in request order. + let resolveScatter: (value: unknown) => void = () => {}; + (SupersetClient.post as jest.Mock).mockImplementation( + ({ + jsonPayload, + }: { + jsonPayload: { form_data: { viz_type: string } }; + }) => { + if (jsonPayload.form_data.viz_type === 'deck_scatter') { + return new Promise(resolve => { + resolveScatter = resolve; + }); + } + return Promise.resolve({ + json: { + result: [ + { + data: [ + { + polygon: [ + [3, 3], + [4, 4], + ], + }, + ], + }, + ], + }, + }); }, - }; + ); - renderWithProviders(<DeckMulti {...props} />); + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: true }} + />, + ); + // Only the polygon layer's points have arrived so far. await waitFor(() => { - const container = screen.getByTestId('deckgl-container'); - const viewportData = JSON.parse( - container.getAttribute('data-viewport') || '{}', - ); + const call = fitViewportSpy.mock.calls.at(-1); + expect(call?.[1].points.length).toBe(2); + }); + + resolveScatter({ + json: { + result: [{ data: [{ position: [1, 1] }, { position: [2, 2] }] }], + }, + }); - expect(viewportData.longitude).toBe(adjustedViewport.longitude); - expect(viewportData.latitude).toBe(adjustedViewport.latitude); - expect(viewportData.zoom).toBe(adjustedViewport.zoom); + // Once the scatter layer resolves, its points are added to the same + // accumulator rather than replacing the polygon layer's. + await waitFor(() => { + const call = fitViewportSpy.mock.calls.at(-1); + expect(call?.[1].points.length).toBe(4); }); fitViewportSpy.mockRestore(); }); - test('should set zoom to 0 when calculated zoom is negative', async () => { + test('should set zoom to 0 when the fitted zoom is negative', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - fitViewportSpy.mockReturnValue({ - longitude: 0, - latitude: 0, - zoom: -5, // negative zoom - }); - - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, - }; - - renderWithProviders(<DeckMulti {...props} />); + fitViewportSpy.mockReturnValue({ longitude: 0, latitude: 0, zoom: -5 }); + featuresByVizType = { deck_scatter: [{ position: [1, 1] }] }; + + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: true }} + />, + ); await waitFor(() => { const container = screen.getByTestId('deckgl-container'); const viewportData = JSON.parse( container.getAttribute('data-viewport') || '{}', ); - - // Zoom should be 0, not negative expect(viewportData.zoom).toBe(0); }); fitViewportSpy.mockRestore(); }); - test('should handle empty features gracefully when autozoom is enabled', () => { + test('should not refit when a layer resolves with no features', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, - payload: { - ...baseMockProps.payload, - data: { - ...baseMockProps.payload.data, - features: { - deck_scatter: [], - deck_polygon: [], - deck_path: [], - deck_grid: [], - deck_contour: [], - deck_heatmap: [], - deck_hex: [], - deck_arc: [], - deck_geojson: [], - deck_screengrid: [], - }, - }, - }, - }; - - renderWithProviders(<DeckMulti {...props} />); + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: true }} + />, + ); - // fitViewport should not be called when there are no points + await waitFor(() => expect(SupersetClient.post).toHaveBeenCalledTimes(2)); expect(fitViewportSpy).not.toHaveBeenCalled(); fitViewportSpy.mockRestore(); }); - test('should collect points from all layer types when autozoom is enabled', () => { + test('should use the original viewport when autozoom is disabled', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - fitViewportSpy.mockReturnValue({ - longitude: 0, - latitude: 0, - zoom: 10, - }); - - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: true, - }, - payload: { - ...baseMockProps.payload, - data: { - ...baseMockProps.payload.data, - features: { - deck_scatter: [{ position: [1, 1] }, { position: [2, 2] }], - deck_polygon: [ - { - polygon: [ - [3, 3], - [4, 4], - ], - }, - ], - deck_arc: [{ sourcePosition: [5, 5], targetPosition: [6, 6] }], - deck_path: [], - deck_grid: [], - deck_contour: [], - deck_heatmap: [], - deck_hex: [], - deck_geojson: [], - deck_screengrid: [], - }, - }, - }, - }; - - renderWithProviders(<DeckMulti {...props} />); - - expect(fitViewportSpy).toHaveBeenCalled(); - const callArgs = fitViewportSpy.mock.calls[0]; - const { points } = callArgs[1]; - - // Should have points from scatter (2), polygon (2), and arc (2) = 6 points total - expect(points.length).toBeGreaterThan(0); - - fitViewportSpy.mockRestore(); - }); - - test('should use original viewport when autozoom is disabled', async () => { - const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const originalViewport = { longitude: -100, latitude: 40, zoom: 5 }; - const props = { - ...baseMockProps, - viewport: originalViewport, - formData: { - ...baseMockProps.formData, - autozoom: false, - }, - }; - renderWithProviders(<DeckMulti {...props} />); + renderWithProviders( + <DeckMulti + {...baseMockProps} + viewport={originalViewport} + formData={{ ...baseMockProps.formData, autozoom: false }} + />, + ); await waitFor(() => { const container = screen.getByTestId('deckgl-container'); const viewportData = JSON.parse( container.getAttribute('data-viewport') || '{}', ); - - // Should use original viewport without modification - expect(viewportData.longitude).toBe(originalViewport.longitude); - expect(viewportData.latitude).toBe(originalViewport.latitude); - expect(viewportData.zoom).toBe(originalViewport.zoom); + expect(viewportData).toMatchObject(originalViewport); }); - - // fitViewport should not have been called expect(fitViewportSpy).not.toHaveBeenCalled(); fitViewportSpy.mockRestore(); }); - test('should apply autozoom when autozoom is undefined (backward compatibility)', () => { + test('should apply autozoom when autozoom is undefined (backward compatibility)', async () => { const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); fitViewportSpy.mockReturnValue({ longitude: -122.4, latitude: 37.8, zoom: 10, }); + featuresByVizType = { deck_scatter: [{ position: [1, 1] }] }; - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: undefined, // Simulating existing charts created before this feature - }, - }; - - renderWithProviders(<DeckMulti {...props} />); - - // fitViewport should be called for backward compatibility with existing charts - expect(fitViewportSpy).toHaveBeenCalledWith( - expect.objectContaining({ - longitude: 0, - latitude: 0, - zoom: 1, - }), - expect.objectContaining({ - width: 800, - height: 600, - points: expect.any(Array), - }), + renderWithProviders( + <DeckMulti + {...baseMockProps} + formData={{ ...baseMockProps.formData, autozoom: undefined }} + />, ); + await waitFor(() => expect(fitViewportSpy).toHaveBeenCalled()); + fitViewportSpy.mockRestore(); }); +}); - test('should use adjusted viewport when autozoom is undefined', async () => { - const fitViewportSpy = jest.spyOn(fitViewportModule, 'default'); - const adjustedViewport = { - longitude: -122.4, - latitude: 37.8, - zoom: 12, - }; - fitViewportSpy.mockReturnValue(adjustedViewport); +describe('DeckMulti stale-response guard', () => { + beforeEach(() => { + jest.clearAllMocks(); + featuresByVizType = {}; + mockFetchesFor(SUBSLICES); + }); - const props = { - ...baseMockProps, - formData: { - ...baseMockProps.formData, - autozoom: undefined, // Simulating existing charts - }, - }; + test('ignores a layer response that resolves after deck_slices has already changed again', async () => { + let resolveFirstLoad: (value: unknown) => void = () => {}; + (SupersetClient.post as jest.Mock) + .mockImplementationOnce( + () => + new Promise(resolve => { + resolveFirstLoad = resolve; + }), + ) + .mockImplementationOnce( + () => + new Promise(resolve => { + resolveFirstLoad = resolve; + }), + ) Review Comment: Good catch, `resolveFirstLoad` was getting clobbered by the second mock so slice-1 never actually resolved. Split into separate resolvers for slice 1 and 2 and resolve both. -- 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]
