sadpandajoe commented on code in PR #43004: URL: https://github.com/apache/superset/pull/43004#discussion_r4129426247
########## 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: This compares each chart's post-refresh text to its own pre-refresh value, so it passes whether the refresh genuinely re-rendered fresh data or the DOM simply never updated at all — `birth_names` is static, so a correct refresh reproduces the same number as before. The signal-based block above already proves every chart completed its 202→200 network round trip; nothing here proves the DOM actually re-painted with that response. Could this assert against something that must change on a correct refresh, to actually prove the render happened? ########## superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts: ########## @@ -337,3 +388,253 @@ export async function createDashboardWithCharts( return { dashboardId, charts }; } + +/** The rendered value of a big-number chart. */ +export function bigNumberValueLocator( + dashboard: DashboardPage, + chartId: number, +): Locator { + return dashboard + .getChart(chartId) + .locator('.superset-legacy-chart-big-number .header-line'); +} + +interface SetupDashboardWithChartsResult { + dashboardId: number; + charts: DashboardLayoutChart[]; + dashboard: DashboardPage; + /** Big-number value locator per chart, in the same order as `charts`. */ + valueLocators: Locator[]; +} + +/** + * Combines {@link createDashboardWithCharts} with navigating to the result and + * waiting for it to load -- the setup every GAQ test case that renders a plain + * big-number dashboard needs before it starts recording its own signals or + * assertions. Callers still assert on `valueLocators` themselves (a happy-path + * test wants them visible; a broken-chart test wants an error alert instead), + * so this only removes the identical creation/navigation boilerplate, not the + * per-test assertions layered on top of it. + * + * @example + * const { charts, dashboard, valueLocators } = + * await setupDashboardWithBigNumberCharts(page, testAssets, testInfo, { + * 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 }); + */ +export async function setupDashboardWithBigNumberCharts( + page: Page, + testAssets: TestAssets, + testInfo: TestInfo, + options: CreateDashboardWithChartsOptions, + navigateOptions?: { timeout?: number }, +): Promise<SetupDashboardWithChartsResult> { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testInfo, + options, + ); + const dashboard = new DashboardPage(page); + const valueLocators = charts.map(chart => + bigNumberValueLocator(dashboard, chart.id), + ); + + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(navigateOptions); + + return { dashboardId, charts, dashboard, valueLocators }; +} + +export interface GaqSignals { + /** + * Every chart-data response status seen for a slice, in order. Under the + * Global Task Framework an async chart-data request produces *two* responses + * for the same URL: the 202 that hands the work to GTF, then the 200 the + * client gets when it re-issues the request and is served from the cache the + * tasks populated. A single value per slice would hide one of them. + * + * A native filter's value fetch hits the same endpoint without a `slice_id`, + * so it is keyed under `undefined` (see {@link sliceIdFromChartDataUrl}). + */ + submitStatusesFor(sliceId?: number): readonly number[]; + /** First status seen for a slice; `undefined` if it has not responded yet. */ + submitStatusFor(sliceId?: number): number | undefined; + /** Poll/fetch events are counted, not flagged: on a busy dashboard they arrive per chart. */ + readonly taskStatusPollCount: number; + /** Chart-data re-requests that were served synchronously (200) after a 202. */ + readonly cachedRereadCount: number; + readonly sawTaskStatusPoll: boolean; + /** True once some slice went 202 -> 200: a full async round trip completed. */ + readonly sawAsyncRoundTrip: boolean; +} + +/** + * Records the GAQ lifecycle signals seen from now on. + * + * Under GTF the cycle is: `POST /api/v1/chart/data` with `async_mode` returns + * **202** with task ids; the client observes completion via + * `GET /api/v1/task/status_changes` (the poll transport, which is what runs + * unless `WEBSOCKET_ENABLE` is on); it then **re-issues the same POST**, which + * returns **200** from the per-query cache the tasks warmed. There is no + * separate result-fetch endpoint any more -- the old `/chart/data/qc-<hash>` + * replay route was removed with the GTF migration. + * + * Attach only once the traffic you care about is the *next* thing to happen -- + * an initial dashboard load fires the same signals, so tracking from before it + * would attribute that load's cycle to whatever you trigger after. + * + * Reads are live getters rather than a snapshot, so callers can poll them from + * inside an `expect(...).toPass()` retry block. + */ +export function trackGaqSignals(page: Page): GaqSignals { + const submitStatuses = new Map<number | undefined, number[]>(); + let taskStatusPollCount = 0; + let cachedRereadCount = 0; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if (request.method() === 'POST' && url.includes('/api/v1/chart/data')) { + const sliceId = sliceIdFromChartDataUrl(url); + const seen = submitStatuses.get(sliceId) ?? []; + // A 200 following a 202 for the same slice is the re-request being served + // from the warmed cache -- the completion half of the round trip. + if (response.status() === 200 && seen.includes(202)) { + cachedRereadCount += 1; + } + submitStatuses.set(sliceId, [...seen, response.status()]); + return; + } + if ( + request.method() === 'GET' && + url.includes(GAQ.TASK_STATUS_CHANGES_PATH) + ) { + taskStatusPollCount += 1; + } + }); + + return { + submitStatusesFor: sliceId => submitStatuses.get(sliceId) ?? [], + submitStatusFor: sliceId => submitStatuses.get(sliceId)?.[0], + get taskStatusPollCount() { + return taskStatusPollCount; + }, + get cachedRereadCount() { + return cachedRereadCount; + }, + get sawTaskStatusPoll() { + return taskStatusPollCount > 0; + }, + get sawAsyncRoundTrip() { + return cachedRereadCount > 0; + }, + }; +} + +interface SetupFilteredDashboardOptions { + /** Dataset backing both the chart and the filter's value lookup -- see {@link CreateDashboardWithChartsOptions}. */ + datasetName?: string; + datasetId?: number; + /** Prefix for the generated chart and dashboard names. */ + namePrefix: string; + /** Column the native filter targets. */ + filterColumn: string; + /** Label shown in the filter bar (default: the column name). */ + filterName?: string; +} + +interface SetupFilteredDashboardResult { + dashboardId: number; + chartId: number; + dashboard: DashboardPage; + filterBar: DashboardFilterBar; + /** Big-number value locator for the dashboard's single chart. */ + value: Locator; +} + +/** + * Builds a dashboard with one big-number chart plus a single-select native + * filter scoped to it. Does NOT navigate: some callers must attach network + * listeners before the first load (the filter's value fetch fires during the + * filter panel's own initialization). + */ +export async function setupDashboardWithSelectFilter( + page: Page, + testAssets: TestAssets, + testInfo: TestInfo, + options: SetupFilteredDashboardOptions, +): Promise<SetupFilteredDashboardResult> { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testInfo, + { + datasetName: options.datasetName, + datasetId: options.datasetId, + chartNamePrefix: options.namePrefix, + chartSpecs: [BIG_NUMBER_COUNT_SPEC], Review Comment: `setupDashboardWithSelectFilter()` always builds its chart from `BIG_NUMBER_COUNT_SPEC` (the saved `count` metric), but its only caller with a virtual dataset — the filter-dropdown test in `global-async-query.spec.ts` — creates that dataset via `apiPostVirtualDataset`, which carries no saved metrics. The chart-data query for that dashboard would fail ("Metric 'count' does not exist"), and the test wouldn't catch it since it only asserts on the filter dropdown's async signals and options, never on the chart itself. Should this take an explicit chart spec (or an ad-hoc metric, like the cold-first-load test does) instead of hardcoding `BIG_NUMBER_COUNT_SPEC`? ########## superset-frontend/playwright/tests/dashboard/global-async-query-resilience.spec.ts: ########## @@ -0,0 +1,366 @@ +/** + * 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) under stress: a query that fails, a superseded + * query racing a newer one, a lost channel token, and a page torn down + * mid-flight. + * + * GAQ's happy path is visually identical to a synchronous load, so these are + * the cases where its machinery actually becomes observable -- or where it + * must stay invisible. The happy paths live in global-async-query.spec.ts. + * + * Requires the `GLOBAL_ASYNC_QUERIES` feature flag, Redis, and a running + * Celery worker. + */ +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiGetChart, apiPutChart } from '../../helpers/api/chart'; +import { TIMEOUT } from '../../utils/constants'; +import { apiPost } from '../../helpers/api/requests'; +import { + BIG_NUMBER_COUNT_SPEC, + setupDashboardWithBigNumberCharts, + setupDashboardWithSelectFilter, + sliceIdFromChartDataUrl, + trackGaqSignals, +} from './dashboard-test-helpers'; +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( + 'broken chart surfaces a clean error under GAQ instead of hanging, and recovers once fixed', + async ({ page, testAssets }) => { + // Two forced refreshes plus an API round-trip between them exceed the + // default timeout on a loaded runner. + testWithAssets.setTimeout(TIMEOUT.SLOW_TEST); + + const BAD_COLUMN = 'this_column_does_not_exist_gaq_test'; + + // A custom SQL metric on a nonexistent column fails in Postgres, not in + // client-side validation -- so the job really is queued and run, and this + // exercises the async error path rather than a request that never ships. + const { dashboardId, dashboard, charts, valueLocators } = + await setupDashboardWithBigNumberCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc3_broken_chart', + chartSpecs: [ + { + viz_type: 'big_number_total', + params: { + metric: { + expressionType: 'SQL', + sqlExpression: `SUM(${BAD_COLUMN})`, + label: 'broken_metric', + hasCustomLabel: true, + }, + }, + }, + ], + }, + ); + const [chart] = charts; + const [value] = valueLocators; + const errorAlert = dashboard.getChart(chart.id).locator('.ant-alert-error'); + + // Let the initial (also broken) load settle before tracking. + await expect(errorAlert).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + const signals = trackGaqSignals(page); + + await dashboard.forceRefresh(); + + await expect(errorAlert).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + await expect(errorAlert).toContainText('Data error'); + await expect(errorAlert).toContainText(BAD_COLUMN); + + await expect(() => { + expect( + signals.submitStatusFor(chart.id), + 'forced chart-data submission for the broken chart should still be accepted (202) onto the async path', + ).toBe(202); + expect( + signals.sawTaskStatusPoll, + 'the client should have polled /api/v1/task/status_changes while the broken query ran', + ).toBe(true); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + + // Fix the config and confirm the chart recovers, rather than staying stuck. + const chartResp = await apiGetChart(page, chart.id); + expect(chartResp.ok()).toBe(true); + const { result } = await chartResp.json(); + const fixedParams = { ...JSON.parse(result.params), metric: 'count' }; + + const updateResp = await apiPutChart(page, chart.id, { + params: JSON.stringify(fixedParams), + }); + expect(updateResp.ok()).toBe(true); + + // Refreshing re-submits the form_data already loaded client-side, so it + // would not pick up an out-of-band config change. Re-navigating refetches + // the chart's metadata. + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + await expect(value).toHaveText(/\d/); + await expect(errorAlert).not.toBeAttached(); + }, +); + +testWithAssets( + 'rapidly switching a filter value never lets the superseded selection clobber the screen', + async ({ page, testAssets }) => { + testWithAssets.setTimeout(TIMEOUT.SLOW_TEST); + + const RACE_DELAY_MS = 3000; + + const { chartId, dashboardId, dashboard, filterBar, value } = + await setupDashboardWithSelectFilter( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + namePrefix: 'gaq_tc4_filter_race', + filterColumn: 'gender', + filterName: 'Gender', + }, + ); + + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad({ timeout: TIMEOUT.SLOW_TEST }); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + // Learn "girl"'s real count first, so the race can assert on that specific + // number. The unfiltered total already contains a digit, so a bare /\d/ + // would pass even if the request never completed. + await filterBar.selectOption('girl'); + await filterBar.apply(); + await expect(value).toHaveText(/\d/, { timeout: TIMEOUT.CHART_RENDER }); + const expectedGirlText = await value.textContent(); Review Comment: This can capture the *pre-filter* value as `expectedGirlText`: the chart already shows a digit (the previous/unfiltered value) while girl's query is still in flight, and `toHaveText(/\d/)` — unlike the signal-based checks added later in this same test — doesn't wait for girl's request to actually resolve before the text is read. If girl's query is slow here, this baseline ends up wrong, and the race assertion below is then checked against it. Should this wait for a network signal (as the rest of the test does) before capturing the baseline? ########## 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. Review Comment: This says the SQL Lab smoke check needs the `chromium-sqllab` project, but `chromium-sqllab`'s `testIgnore` deliberately excludes it (see playwright.config.ts) — it only runs under `chromium-gaq`, same as these dashboard specs. A maintainer following this comment to run the smoke test with `--project=chromium-sqllab` would collect zero tests. Can this reference `chromium-gaq` instead? ########## .github/workflows/superset-e2e.yml: ########## @@ -452,6 +452,174 @@ jobs: fi echo "cypress-matrix result: $RESULT (changes: $CHANGES)" + # GAQ runs in its own job rather than as a step in playwright-tests above. + # A step with no explicit `if:` implicitly inherits `if: success()`, so + # when GAQ was a step after the other suites in that job, a failure in + # either of those unrelated suites skipped GAQ entirely rather than failing it + # -- silently leaving that commit with zero GAQ coverage instead of a visible + # red check. A separate job can't share that fate: it either runs and reports + # for itself, or it doesn't start (e.g. the environment itself never came up), + # which is the only case where "no GAQ result" is actually the right outcome. + playwright-tests-gaq: + needs: changes + if: needs.changes.outputs.python == 'true' || needs.changes.outputs.frontend == 'true' + runs-on: ubuntu-26.04 + timeout-minutes: 30 + continue-on-error: true + permissions: + contents: read + pull-requests: read + strategy: + fail-fast: false + matrix: + browser: ["chromium"] + app_root: ["", "/app/prefix"] + env: + SUPERSET_ENV: development + SUPERSET_CONFIG: tests.integration_tests.superset_test_config_gaq + SUPERSET__SQLALCHEMY_DATABASE_URI: postgresql+psycopg2://superset:[email protected]:15432/superset + PYTHONPATH: ${{ github.workspace }} + REDIS_PORT: 16379 + GITHUB_TOKEN: ${{ github.token }} + services: + postgres: + image: postgres:17-alpine Review Comment: This job pulls `postgres:17-alpine` and `redis:7-alpine` (line 493) straight from Docker Hub, but the sibling `playwright-tests` job in this same file uses the GHCR mirror instead (`ghcr.io/apache/superset/ci/postgres:17-alpine` / `.../redis:7-alpine`) — that switch (#40882) was specifically to avoid Docker Hub's anonymous-pull rate limit hitting shared runner IPs. This job runs on every PR, not just merges, `continue-on-error: true`, and isn't part of the required check, so a rate-limit hit here fails both matrix legs at container init silently, with no signal to anyone. Should this use the same GHCR-mirrored images as the rest of the workflow? -- 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]
