drivaspreset commented on code in PR #43004: URL: https://github.com/apache/superset/pull/43004#discussion_r4135004923
########## superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts: ########## @@ -0,0 +1,394 @@ +/** + * 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. + */ + +/** + * Global Async Queries (GAQ): the pipeline works for each of its consumers -- + * a cold first load, a forced refresh, the cache-hit shortcut that bypasses the + * cycle entirely, many charts at once, and native filter value lookups. + * + * Failure and edge-case behavior lives in global-async-query-resilience.spec.ts. + * SQL Lab's smoke check lives in tests/sqllab/, which needs the + * `chromium-sqllab` project rather than this directory's default one. + * + * Requires the `GLOBAL_ASYNC_QUERIES` feature flag, Redis, and a running + * Celery worker -- without a worker, submissions still return 202 but no job + * ever executes and these tests time out. The cache-hit test is the exception: + * it is served synchronously and needs only the flag. + */ +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { TIMEOUT } from '../../utils/constants'; +import { + BIG_NUMBER_COUNT_SPEC, + bigNumberValueLocator, + createCacheColdVirtualDataset, + createDashboardWithCharts, + setupDashboardWithBigNumberCharts, + setupDashboardWithSelectFilter, + trackGaqSignals, +} from './dashboard-test-helpers'; +import { DashboardPage } from '../../pages/DashboardPage'; +import { isFeatureEnabled } from '../../helpers/featureFlags'; + +testWithAssets.beforeEach(async ({ page }) => { + await page.goto('chart/list/'); + testWithAssets.skip( + !(await isFeatureEnabled(page, 'GLOBAL_ASYNC_QUERIES')), + 'GLOBAL_ASYNC_QUERIES is not enabled on this instance', + ); +}); + +testWithAssets( + 'forced dashboard refresh goes through the GAQ 202 -> poll -> done cycle', + async ({ page, testAssets }) => { + const { dashboard, charts, valueLocators } = + await setupDashboardWithBigNumberCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc1_cold_cache', + chartSpecs: [BIG_NUMBER_COUNT_SPEC], + }, + ); + const [chart] = charts; + const [value] = valueLocators; + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + // Track only after the initial load settles, so these signals describe the + // forced refresh rather than the load that preceded it. + const signals = trackGaqSignals(page); + + // A fresh chart's query can still collide with an identical one another + // suite already cached, so force the refresh: forced requests take the + // async path regardless of cache state. + await dashboard.forceRefresh(); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + await expect(value).toHaveText(/\d/); + + await expect(() => { + expect( + signals.submitStatusFor(chart.id), + 'forced chart-data submission should be accepted (202) onto the async path', + ).toBe(202); + expect( + signals.sawTaskStatusPoll, + 'the client should have polled /api/v1/task/status_changes while the tasks ran', + ).toBe(true); + expect( + signals.submitStatusesFor(chart.id), + 'the client should re-issue chart-data once the tasks finish and be served 200 from the warmed cache', + ).toEqual([202, 200]); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + }, +); + +testWithAssets( + 'a cold first load resolves through the GAQ cycle with no manual refresh', + async ({ page, testAssets }) => { + testWithAssets.setTimeout(TIMEOUT.SLOW_TEST); + + // The test above forces a refresh, because a fresh chart over a shared + // physical table can collide with a query another suite already cached and + // a forced request takes the async path regardless of cache state. That + // leaves the unforced first load -- the one a real user gets -- unasserted. + // + // A cache-cold dataset removes the need to force anything, so the async + // cycle can be asserted on the initial render itself. + const { datasetId } = await createCacheColdVirtualDataset( + page, + testAssets, + testWithAssets.info(), + { namePrefix: 'gaq_cold_first_load' }, + ); + + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetId, + chartNamePrefix: 'gaq_cold_first_load', + chartSpecs: [ + { + viz_type: 'big_number_total', + // An adhoc metric, not the saved `count` the physical example + // datasets ship with: a dataset created through the API carries no + // metrics at all, so a saved-metric reference would not resolve. + params: { + metric: { + expressionType: 'SIMPLE', + column: { column_name: 'name' }, + aggregate: 'COUNT', + label: 'COUNT(name)', + }, + }, + }, + ], + }, + ); + const [chart] = charts; + const dashboard = new DashboardPage(page); + const value = bigNumberValueLocator(dashboard, chart.id); + + // Track before navigating: the request under test is the one the first page + // load fires, so there is no later point at which to start listening. + const signals = trackGaqSignals(page); + + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad({ timeout: TIMEOUT.SLOW_TEST }); + + // The regression this guards: the chart resolves on the first load. An async + // handoff that never completes leaves a spinner here and the user has to + // refresh to get a number -- which the forced-refresh test cannot catch, + // because refreshing is the very thing it does. + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + await expect(value).toHaveText(/\d/); + + await expect(() => { + expect( + signals.submitStatusFor(chart.id), + 'a cold first load should be accepted (202) onto the async path', + ).toBe(202); + expect( + signals.sawTaskStatusPoll, + 'the client should have polled /api/v1/task/status_changes while the task ran', + ).toBe(true); + expect( + signals.submitStatusesFor(chart.id), + 'the first load alone should complete the round trip -- 202, then 200 from the warmed cache -- without any user-initiated refresh', + ).toEqual([202, 200]); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + }, +); + +testWithAssets( + 'reloading an already-cached dashboard serves the chart synchronously, without the async cycle', + async ({ page, testAssets }) => { + // This first load is what warms the cache -- not the request under test. + const { dashboard, charts, valueLocators } = + await setupDashboardWithBigNumberCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc2_cache_hit', + chartSpecs: [BIG_NUMBER_COUNT_SPEC], + }, + ); + const [chart] = charts; + const [value] = valueLocators; + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + const signals = trackGaqSignals(page); + + // A plain reload -- same filters, no forced refresh -- is what should take + // the cache-hit shortcut instead of re-entering the async cycle. + await page.reload(); + await dashboard.waitForLoad(); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + await expect(value).toHaveText(/\d/); + + await expect(() => { + expect( + signals.submitStatusesFor(chart.id), + 'a cache-hit reload should resolve chart-data synchronously (200) on the first request, never queueing onto the async path (202) or needing a re-request', + ).toEqual([200]); + expect( + signals.sawTaskStatusPoll, + 'a cache hit should never need to poll /api/v1/task/status_changes', + ).toBe(false); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + }, +); + +testWithAssets( + 'refreshing a dashboard with many charts resolves every chart independently and correctly', + async ({ page, testAssets }) => { + testWithAssets.setTimeout(TIMEOUT.SLOW_TEST); + + // Distinct names, so a misrouted event (one chart rendering another's + // result) is actually detectable -- identical queries would hide it. + const NAMES = [ + 'John', + 'Mary', + 'James', + 'Linda', + 'Robert', + 'Patricia', + 'Michael', + 'Barbara', + ]; + + const { dashboard, charts, valueLocators } = + await setupDashboardWithBigNumberCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc5_busy_dashboard', + chartSpecs: NAMES.map(name => ({ + viz_type: 'big_number_total', + params: { + metric: 'count', + adhoc_filters: [ + { + clause: 'WHERE', + expressionType: 'SIMPLE', + subject: 'name', + operator: '==', + comparator: name, + }, + ], + }, + })), + // 8 charts at the default width (4) would exceed the 12-column grid. + chartWidth: 1, + }, + { timeout: TIMEOUT.SLOW_TEST }, + ); + + await Promise.all( + valueLocators.map(locator => + expect(locator).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }), + ), + ); + + // Each chart's own filter is baked into its query, so its pre-refresh count + // is per-chart ground truth. "Values aren't all identical" would not catch + // two charts swapping results; "chart N still shows chart N's count" does. + const expectedValues = await Promise.all( + valueLocators.map(locator => locator.textContent()), + ); + + const signals = trackGaqSignals(page); + + await dashboard.forceRefresh(); + + await Promise.all( + valueLocators.map(locator => + expect(locator).toHaveText(/\d/, { timeout: TIMEOUT.CHART_RENDER }), + ), + ); + + await expect(() => { + for (const chart of charts) { + expect( + signals.submitStatusesFor(chart.id), + `chart ${chart.id} (${chart.sliceName}) should have gone 202 onto the async path, then 200 on the re-request`, + ).toEqual([202, 200]); + } + expect( + signals.taskStatusPollCount, + 'the client should have polled /api/v1/task/status_changes while the concurrent tasks ran', + ).toBeGreaterThan(0); + expect( + signals.cachedRereadCount, + 'every chart should have completed its own 202 -> 200 round trip', + ).toBeGreaterThanOrEqual(charts.length); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + + // If these names didn't produce distinct counts, the per-chart assertion + // below would pass no matter how badly results were shuffled. + expect( + new Set(expectedValues).size, + 'each chart filters on a different name, so their pre-refresh counts should not all collapse to the same number', + ).toBeGreaterThan(1); + + const displayedValues = await Promise.all( + valueLocators.map(locator => locator.textContent()), + ); + for (const [index, chart] of charts.entries()) { + expect( + displayedValues[index], + `chart ${chart.id} (${chart.sliceName}) should show its own count (${expectedValues[index]}) after the refresh, not another chart's result`, Review Comment: Agreed — as written that assertion could not distinguish a real re-render from a DOM that never updated, since `birth_names` is static and a correct refresh reproduces the same number. Fixed in d56f007ffc by giving the test something that *must* change. The charts now sit on a per-run virtual dataset that stamps each query with the server clock: ```sql SELECT name, CAST(EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000 AS BIGINT) AS queried_at_ms FROM birth_names ``` A ninth chart shows `MAX(queried_at_ms)`, and the refresh must make it strictly increase: ```ts await expect .poll(readClock, { timeout: TIMEOUT.CHART_RENDER }) .toBeGreaterThan(clockBefore); ``` That is the render-level proof the network block can't give: a stale DOM fails it. The chart carries `y_axis_format: ',d'` because the default SMART_NUMBER rounds two stamps seconds apart to the same `1.76T`. The eight per-name identity checks stay exactly as they were — with the repaint now established independently, "each chart still shows its own count" becomes a real anti-shuffling check rather than something that passes by default. Passing on the current run (test 8, both legs). -- 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]
