drivaspreset commented on code in PR #43004: URL: https://github.com/apache/superset/pull/43004#discussion_r3767973183
########## superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts: ########## @@ -0,0 +1,1213 @@ +/** + * 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. + */ + +/** + * GAQ (Global Async Queries) dashboard coverage -- TC1 through TC8. + * See ../../GAQ_Architecture.md and ../../gaq-test-cases.md for the design + * each test below implements. + * + * TC9 (SQL Lab smoke test) lives separately in + * tests/sqllab/global-async-query-sqllab.spec.ts, since it must run under + * the `chromium-sqllab` project rather than this directory's default one. + * + * Precondition (all tests): `GLOBAL_ASYNC_QUERIES` feature flag enabled, + * plus Redis and a running Celery worker -- EXCEPT TC2 (cache-hit reload), + * which is served synchronously and never touches the async channel, so it + * needs only the feature flag. See each test's own "Precondition" note below + * for specifics, and ../../gaq-test-cases.md's "Environment prerequisites" + * for how to stand up Redis/Celery locally. + */ +import type { Page } from '@playwright/test'; +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiGetChart, apiPutChart } from '../../helpers/api/chart'; +import { apiPost, apiPut } from '../../helpers/api/requests'; +import { + apiPostDashboard, + buildSingleRowDashboardLayout, +} from '../../helpers/api/dashboard'; +import { + apiPostVirtualDataset, + getDatasetByName, +} from '../../helpers/api/dataset'; +import { getDatabaseByName } from '../../helpers/api/database'; +import { extractIdFromResponse } from '../../helpers/api/assertions'; +import { DashboardPage } from '../../pages/DashboardPage'; +import { TIMEOUT } from '../../utils/constants'; +import { + createDashboardWithCharts, + sliceIdFromChartDataUrl, +} from './dashboard-test-helpers'; + +// --------------------------------------------------------------------------- +// TC1 -- Normal load, happy path (cold cache). +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled, plus Redis and a running +// Celery worker. Without a worker, the chart-data POST below still returns +// 202, but no job ever executes and this test times out waiting for the +// chart to render. +// +// A freshly created chart's query could still coincidentally share a cache +// key with an identical query cached by another test (e.g. another suite +// also querying birth_names/count/big_number_total), so this test forces a +// refresh rather than relying on a plain first load -- a forced request +// always takes the async path regardless of cache state, which is the +// deterministic way to guarantee we're exercising the real cycle and not +// silently hitting the cache-hit shortcut (see TC2). +// +// CI green => the forced chart-data request was accepted (202), at least one +// /api/v1/async_event/ poll occurred, the real payload was +// fetched from /api/v1/chart/data/<cache_key>, and the chart +// rendered its queried value. +// CI red => any of the above didn't happen (e.g. flag disabled, no worker +// running, or the async pipeline broke). +// --------------------------------------------------------------------------- +testWithAssets( + 'forced dashboard refresh goes through the GAQ 202 -> poll -> done cycle', + async ({ page, testAssets }) => { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc1_cold_cache', + dashboardTitlePrefix: 'gaq_tc1_cold_cache', + chartSpecs: [ + { + viz_type: 'big_number_total', + params: { metric: 'count' }, + }, + ], + }, + ); + const [chart] = charts; + + const dashboard = new DashboardPage(page); + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + + const value = dashboard + .getChart(chart.id) + .locator('.superset-legacy-chart-big-number .header-line'); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + // Only start recording once the initial load has settled, so these + // signals reflect the forced refresh below rather than the first load. + let chartDataSubmitStatus: number | undefined; + let sawAsyncEventPoll = false; + let sawFinalCachedFetch = false; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if ( + request.method() === 'POST' && + url.includes('/api/v1/chart/data') && + sliceIdFromChartDataUrl(url) === chart.id + ) { + chartDataSubmitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + sawAsyncEventPoll = true; + return; + } + if ( + request.method() === 'GET' && + /\/api\/v1\/chart\/data\/qc-/.test(url) + ) { + sawFinalCachedFetch = true; + } + }); Review Comment: Resolved ✅ ########## superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts: ########## @@ -0,0 +1,1213 @@ +/** + * 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. + */ + +/** + * GAQ (Global Async Queries) dashboard coverage -- TC1 through TC8. + * See ../../GAQ_Architecture.md and ../../gaq-test-cases.md for the design + * each test below implements. + * + * TC9 (SQL Lab smoke test) lives separately in + * tests/sqllab/global-async-query-sqllab.spec.ts, since it must run under + * the `chromium-sqllab` project rather than this directory's default one. + * + * Precondition (all tests): `GLOBAL_ASYNC_QUERIES` feature flag enabled, + * plus Redis and a running Celery worker -- EXCEPT TC2 (cache-hit reload), + * which is served synchronously and never touches the async channel, so it + * needs only the feature flag. See each test's own "Precondition" note below + * for specifics, and ../../gaq-test-cases.md's "Environment prerequisites" + * for how to stand up Redis/Celery locally. + */ +import type { Page } from '@playwright/test'; +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiGetChart, apiPutChart } from '../../helpers/api/chart'; +import { apiPost, apiPut } from '../../helpers/api/requests'; +import { + apiPostDashboard, + buildSingleRowDashboardLayout, +} from '../../helpers/api/dashboard'; +import { + apiPostVirtualDataset, + getDatasetByName, +} from '../../helpers/api/dataset'; +import { getDatabaseByName } from '../../helpers/api/database'; +import { extractIdFromResponse } from '../../helpers/api/assertions'; +import { DashboardPage } from '../../pages/DashboardPage'; +import { TIMEOUT } from '../../utils/constants'; +import { + createDashboardWithCharts, + sliceIdFromChartDataUrl, +} from './dashboard-test-helpers'; + +// --------------------------------------------------------------------------- +// TC1 -- Normal load, happy path (cold cache). +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled, plus Redis and a running +// Celery worker. Without a worker, the chart-data POST below still returns +// 202, but no job ever executes and this test times out waiting for the +// chart to render. +// +// A freshly created chart's query could still coincidentally share a cache +// key with an identical query cached by another test (e.g. another suite +// also querying birth_names/count/big_number_total), so this test forces a +// refresh rather than relying on a plain first load -- a forced request +// always takes the async path regardless of cache state, which is the +// deterministic way to guarantee we're exercising the real cycle and not +// silently hitting the cache-hit shortcut (see TC2). +// +// CI green => the forced chart-data request was accepted (202), at least one +// /api/v1/async_event/ poll occurred, the real payload was +// fetched from /api/v1/chart/data/<cache_key>, and the chart +// rendered its queried value. +// CI red => any of the above didn't happen (e.g. flag disabled, no worker +// running, or the async pipeline broke). +// --------------------------------------------------------------------------- +testWithAssets( + 'forced dashboard refresh goes through the GAQ 202 -> poll -> done cycle', + async ({ page, testAssets }) => { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc1_cold_cache', + dashboardTitlePrefix: 'gaq_tc1_cold_cache', + chartSpecs: [ + { + viz_type: 'big_number_total', + params: { metric: 'count' }, + }, + ], + }, + ); + const [chart] = charts; + + const dashboard = new DashboardPage(page); + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + + const value = dashboard + .getChart(chart.id) + .locator('.superset-legacy-chart-big-number .header-line'); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + // Only start recording once the initial load has settled, so these + // signals reflect the forced refresh below rather than the first load. + let chartDataSubmitStatus: number | undefined; + let sawAsyncEventPoll = false; + let sawFinalCachedFetch = false; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if ( + request.method() === 'POST' && + url.includes('/api/v1/chart/data') && + sliceIdFromChartDataUrl(url) === chart.id + ) { + chartDataSubmitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + sawAsyncEventPoll = true; + return; + } + if ( + request.method() === 'GET' && + /\/api\/v1\/chart\/data\/qc-/.test(url) + ) { + sawFinalCachedFetch = true; + } + }); + + await dashboard.forceRefresh(); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + await expect(value).toHaveText(/\d/); + + await expect(() => { + expect( + chartDataSubmitStatus, + 'forced chart-data submission should be accepted (202) onto the async path', + ).toBe(202); + expect( + sawAsyncEventPoll, + 'the client should have polled /api/v1/async_event/ while the job ran', + ).toBe(true); + expect( + sawFinalCachedFetch, + 'once done, the client should fetch the real payload from /api/v1/chart/data/<cache_key>', + ).toBe(true); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + }, +); + +// --------------------------------------------------------------------------- +// TC2 -- Cache-hit fast reload. +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled. Unlike every other GAQ test +// case, this one does NOT depend on Redis/Celery: a cache hit is served +// synchronously from the chart-data endpoint itself +// (`force_cached`/`force=false`), so it never touches the async channel. +// +// The dashboard is loaded once first to warm the cache for this chart's +// exact query context, then reloaded via a plain page reload -- no filter +// change, no "Refresh dashboard" -- which is what should hit the cache-hit +// shortcut rather than re-entering the 202 -> poll -> done cycle exercised +// by TC1. +// +// CI green => on reload, the chart-data request for this chart resolved +// with a synchronous 200 (never a 202), no +// /api/v1/async_event/ poll occurred, and no separate +// /api/v1/chart/data/<cache_key> fetch occurred -- the reload +// got its data straight from the initial POST response. +// CI red => any of the above didn't hold (e.g. the cache-hit shortcut +// regressed into taking the async path). +// --------------------------------------------------------------------------- +testWithAssets( + 'reloading an already-cached dashboard serves the chart synchronously, without the async cycle', + async ({ page, testAssets }) => { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc2_cache_hit', + dashboardTitlePrefix: 'gaq_tc2_cache_hit', + chartSpecs: [ + { + viz_type: 'big_number_total', + params: { metric: 'count' }, + }, + ], + }, + ); + const [chart] = charts; + + const dashboard = new DashboardPage(page); + const value = dashboard + .getChart(chart.id) + .locator('.superset-legacy-chart-big-number .header-line'); + + // First load warms the cache for this chart's exact query context -- + // not the request under test, so no listeners attached yet. + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + let chartDataSubmitStatus: number | undefined; + let sawAsyncEventPoll = false; + let sawCachedFetch = false; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if ( + request.method() === 'POST' && + url.includes('/api/v1/chart/data') && + sliceIdFromChartDataUrl(url) === chart.id + ) { + chartDataSubmitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + sawAsyncEventPoll = true; + return; + } + if ( + request.method() === 'GET' && + /\/api\/v1\/chart\/data\/qc-/.test(url) + ) { + sawCachedFetch = true; + } + }); + + // Plain reload -- same filters, no "Refresh dashboard" -- is what should + // take the cache-hit shortcut rather than 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( + chartDataSubmitStatus, + 'a cache-hit reload should resolve chart-data synchronously (200), not queue onto the async path (202)', + ).toBe(200); + expect( + sawAsyncEventPoll, + 'a cache hit should never need to poll /api/v1/async_event/', + ).toBe(false); + expect( + sawCachedFetch, + 'a cache hit should never need the follow-up /api/v1/chart/data/<cache_key> fetch -- the data comes back on the initial POST', + ).toBe(false); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + }, +); + +// --------------------------------------------------------------------------- +// TC3 -- Broken chart yields a clean error, not a hang. +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled, plus Redis and a running +// Celery worker. The broken chart still queues and runs a real job under GAQ -- the query +// just fails once Postgres executes it -- so this exercises the async error +// path (STATUS_ERROR over the async channel), not a client-side validation +// failure that would never reach the async pipeline at all. +// +// The chart's metric is a custom SQL expression referencing a column that +// doesn't exist, guaranteeing a real backend query error regardless of +// dataset contents. +// +// CI green => the forced chart-data request was accepted (202), at least one +// /api/v1/async_event/ poll occurred, and the chart surfaced a +// legible "Data error" alert (not an indefinite spinner) with +// the real Postgres error naming the bad column. Then, after +// fixing the chart's metric via the API and forcing another +// refresh, the chart renders real data and the error clears. +// CI red => any of the above didn't happen (e.g. flag disabled, no worker +// running, the error never surfaced, or it hung instead). +// --------------------------------------------------------------------------- +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 in between exceed the + // default 30s test timeout on a loaded runner. + testWithAssets.setTimeout(TIMEOUT.SLOW_TEST); + + const BAD_COLUMN = 'this_column_does_not_exist_gaq_test'; + + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc3_broken_chart', + dashboardTitlePrefix: '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 dashboard = new DashboardPage(page); + const chartLocator = dashboard.getChart(chart.id); + const errorAlert = chartLocator.locator('.ant-alert-error'); + const value = chartLocator.locator( + '.superset-legacy-chart-big-number .header-line', + ); + + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + + // Let the initial (also broken) load settle before recording signals, so + // they reflect the forced refresh below rather than the first load. + await expect(errorAlert).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + let chartDataSubmitStatus: number | undefined; + let sawAsyncEventPoll = false; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if ( + request.method() === 'POST' && + url.includes('/api/v1/chart/data') && + sliceIdFromChartDataUrl(url) === chart.id + ) { + chartDataSubmitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + sawAsyncEventPoll = true; + } + }); + + // Force-refresh guarantees this exercises the async path + // deterministically, the same way TC1 does for the happy path. + 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( + chartDataSubmitStatus, + 'forced chart-data submission for the broken chart should still be accepted (202) onto the async path', + ).toBe(202); + expect( + sawAsyncEventPoll, + 'the client should have polled /api/v1/async_event/ while the broken query ran', + ).toBe(true); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + + // Fix the underlying config and confirm the chart recovers normally, + // rather than staying stuck in an error state. + 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); + + // "Refresh dashboard" re-submits whatever form_data the dashboard already + // has loaded client-side -- it doesn't pick up a chart config change made + // out-of-band via the API. A fresh navigation re-fetches chart metadata + // (including the fixed metric) from the backend. + 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(); + }, +); + +// --------------------------------------------------------------------------- +// TC4 -- Filter change mid-load shows no stale data. +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled, plus Redis and a running +// Celery worker. +// +// The prepopulated example queries here resolve in ~1-2s, too fast to +// reliably race in real time, so the first filter selection's response is +// artificially delayed via page.route() -- this guarantees it's still +// in-flight when the second selection fires, which is what actually +// exercises the client's stale-query guard (chartAction.ts dispatches +// CHART_UPDATE_STOPPED to abort a superseded query's controller when a new +// one starts for the same chart) rather than two selections that happen to +// resolve one after another with no overlap. +// +// The filter is single-select (multiSelect: false) specifically so each +// click replaces the prior selection instead of accumulating -- the failure +// mode from an earlier manual pass against a multi-select filter, per +// gaq-test-cases.md's automation notes for this test case. +// +// CI green => after rapidly selecting "boy" (delayed) then "girl" +// (undelayed) with no wait in between, the chart settles on +// girl's count and never flips to boy's count even after +// waiting well past the artificial delay -- proving the +// superseded "boy" job's late result never clobbers the screen. +// CI red => the chart shows boy's count at any point after girl's +// selection was applied (stale data winning the race), or the +// two counts are indistinguishable (test can't actually tell). +// --------------------------------------------------------------------------- +async function selectGenderFilterOption( + page: Page, + genderValue: string, +): Promise<void> { + const filterCombobox = page + .locator('[data-test="form-item-value"]') + .first() + .locator('[role="combobox"]'); Review Comment: Resolved ✅ ########## superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts: ########## @@ -0,0 +1,1213 @@ +/** + * 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. + */ + +/** + * GAQ (Global Async Queries) dashboard coverage -- TC1 through TC8. + * See ../../GAQ_Architecture.md and ../../gaq-test-cases.md for the design + * each test below implements. + * + * TC9 (SQL Lab smoke test) lives separately in + * tests/sqllab/global-async-query-sqllab.spec.ts, since it must run under + * the `chromium-sqllab` project rather than this directory's default one. + * + * Precondition (all tests): `GLOBAL_ASYNC_QUERIES` feature flag enabled, + * plus Redis and a running Celery worker -- EXCEPT TC2 (cache-hit reload), + * which is served synchronously and never touches the async channel, so it + * needs only the feature flag. See each test's own "Precondition" note below + * for specifics, and ../../gaq-test-cases.md's "Environment prerequisites" + * for how to stand up Redis/Celery locally. + */ +import type { Page } from '@playwright/test'; +import { testWithAssets, expect } from '../../helpers/fixtures'; +import { apiGetChart, apiPutChart } from '../../helpers/api/chart'; +import { apiPost, apiPut } from '../../helpers/api/requests'; +import { + apiPostDashboard, + buildSingleRowDashboardLayout, +} from '../../helpers/api/dashboard'; +import { + apiPostVirtualDataset, + getDatasetByName, +} from '../../helpers/api/dataset'; +import { getDatabaseByName } from '../../helpers/api/database'; +import { extractIdFromResponse } from '../../helpers/api/assertions'; +import { DashboardPage } from '../../pages/DashboardPage'; +import { TIMEOUT } from '../../utils/constants'; +import { + createDashboardWithCharts, + sliceIdFromChartDataUrl, +} from './dashboard-test-helpers'; + +// --------------------------------------------------------------------------- +// TC1 -- Normal load, happy path (cold cache). +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled, plus Redis and a running +// Celery worker. Without a worker, the chart-data POST below still returns +// 202, but no job ever executes and this test times out waiting for the +// chart to render. +// +// A freshly created chart's query could still coincidentally share a cache +// key with an identical query cached by another test (e.g. another suite +// also querying birth_names/count/big_number_total), so this test forces a +// refresh rather than relying on a plain first load -- a forced request +// always takes the async path regardless of cache state, which is the +// deterministic way to guarantee we're exercising the real cycle and not +// silently hitting the cache-hit shortcut (see TC2). +// +// CI green => the forced chart-data request was accepted (202), at least one +// /api/v1/async_event/ poll occurred, the real payload was +// fetched from /api/v1/chart/data/<cache_key>, and the chart +// rendered its queried value. +// CI red => any of the above didn't happen (e.g. flag disabled, no worker +// running, or the async pipeline broke). +// --------------------------------------------------------------------------- +testWithAssets( + 'forced dashboard refresh goes through the GAQ 202 -> poll -> done cycle', + async ({ page, testAssets }) => { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc1_cold_cache', + dashboardTitlePrefix: 'gaq_tc1_cold_cache', + chartSpecs: [ + { + viz_type: 'big_number_total', + params: { metric: 'count' }, + }, + ], + }, + ); + const [chart] = charts; + + const dashboard = new DashboardPage(page); + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + + const value = dashboard + .getChart(chart.id) + .locator('.superset-legacy-chart-big-number .header-line'); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + // Only start recording once the initial load has settled, so these + // signals reflect the forced refresh below rather than the first load. + let chartDataSubmitStatus: number | undefined; + let sawAsyncEventPoll = false; + let sawFinalCachedFetch = false; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if ( + request.method() === 'POST' && + url.includes('/api/v1/chart/data') && + sliceIdFromChartDataUrl(url) === chart.id + ) { + chartDataSubmitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + sawAsyncEventPoll = true; + return; + } + if ( + request.method() === 'GET' && + /\/api\/v1\/chart\/data\/qc-/.test(url) + ) { + sawFinalCachedFetch = true; + } + }); + + await dashboard.forceRefresh(); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + await expect(value).toHaveText(/\d/); + + await expect(() => { + expect( + chartDataSubmitStatus, + 'forced chart-data submission should be accepted (202) onto the async path', + ).toBe(202); + expect( + sawAsyncEventPoll, + 'the client should have polled /api/v1/async_event/ while the job ran', + ).toBe(true); + expect( + sawFinalCachedFetch, + 'once done, the client should fetch the real payload from /api/v1/chart/data/<cache_key>', + ).toBe(true); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + }, +); + +// --------------------------------------------------------------------------- +// TC2 -- Cache-hit fast reload. +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled. Unlike every other GAQ test +// case, this one does NOT depend on Redis/Celery: a cache hit is served +// synchronously from the chart-data endpoint itself +// (`force_cached`/`force=false`), so it never touches the async channel. +// +// The dashboard is loaded once first to warm the cache for this chart's +// exact query context, then reloaded via a plain page reload -- no filter +// change, no "Refresh dashboard" -- which is what should hit the cache-hit +// shortcut rather than re-entering the 202 -> poll -> done cycle exercised +// by TC1. +// +// CI green => on reload, the chart-data request for this chart resolved +// with a synchronous 200 (never a 202), no +// /api/v1/async_event/ poll occurred, and no separate +// /api/v1/chart/data/<cache_key> fetch occurred -- the reload +// got its data straight from the initial POST response. +// CI red => any of the above didn't hold (e.g. the cache-hit shortcut +// regressed into taking the async path). +// --------------------------------------------------------------------------- +testWithAssets( + 'reloading an already-cached dashboard serves the chart synchronously, without the async cycle', + async ({ page, testAssets }) => { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc2_cache_hit', + dashboardTitlePrefix: 'gaq_tc2_cache_hit', + chartSpecs: [ + { + viz_type: 'big_number_total', + params: { metric: 'count' }, + }, + ], + }, + ); + const [chart] = charts; + + const dashboard = new DashboardPage(page); + const value = dashboard + .getChart(chart.id) + .locator('.superset-legacy-chart-big-number .header-line'); + + // First load warms the cache for this chart's exact query context -- + // not the request under test, so no listeners attached yet. + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + await expect(value).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + let chartDataSubmitStatus: number | undefined; + let sawAsyncEventPoll = false; + let sawCachedFetch = false; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if ( + request.method() === 'POST' && + url.includes('/api/v1/chart/data') && + sliceIdFromChartDataUrl(url) === chart.id + ) { + chartDataSubmitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + sawAsyncEventPoll = true; + return; + } + if ( + request.method() === 'GET' && + /\/api\/v1\/chart\/data\/qc-/.test(url) + ) { + sawCachedFetch = true; + } + }); + + // Plain reload -- same filters, no "Refresh dashboard" -- is what should + // take the cache-hit shortcut rather than 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( + chartDataSubmitStatus, + 'a cache-hit reload should resolve chart-data synchronously (200), not queue onto the async path (202)', + ).toBe(200); + expect( + sawAsyncEventPoll, + 'a cache hit should never need to poll /api/v1/async_event/', + ).toBe(false); + expect( + sawCachedFetch, + 'a cache hit should never need the follow-up /api/v1/chart/data/<cache_key> fetch -- the data comes back on the initial POST', + ).toBe(false); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + }, +); + +// --------------------------------------------------------------------------- +// TC3 -- Broken chart yields a clean error, not a hang. +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled, plus Redis and a running +// Celery worker. The broken chart still queues and runs a real job under GAQ -- the query +// just fails once Postgres executes it -- so this exercises the async error +// path (STATUS_ERROR over the async channel), not a client-side validation +// failure that would never reach the async pipeline at all. +// +// The chart's metric is a custom SQL expression referencing a column that +// doesn't exist, guaranteeing a real backend query error regardless of +// dataset contents. +// +// CI green => the forced chart-data request was accepted (202), at least one +// /api/v1/async_event/ poll occurred, and the chart surfaced a +// legible "Data error" alert (not an indefinite spinner) with +// the real Postgres error naming the bad column. Then, after +// fixing the chart's metric via the API and forcing another +// refresh, the chart renders real data and the error clears. +// CI red => any of the above didn't happen (e.g. flag disabled, no worker +// running, the error never surfaced, or it hung instead). +// --------------------------------------------------------------------------- +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 in between exceed the + // default 30s test timeout on a loaded runner. + testWithAssets.setTimeout(TIMEOUT.SLOW_TEST); + + const BAD_COLUMN = 'this_column_does_not_exist_gaq_test'; + + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testWithAssets.info(), + { + datasetName: 'birth_names', + chartNamePrefix: 'gaq_tc3_broken_chart', + dashboardTitlePrefix: '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 dashboard = new DashboardPage(page); + const chartLocator = dashboard.getChart(chart.id); + const errorAlert = chartLocator.locator('.ant-alert-error'); + const value = chartLocator.locator( + '.superset-legacy-chart-big-number .header-line', + ); + + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(); + + // Let the initial (also broken) load settle before recording signals, so + // they reflect the forced refresh below rather than the first load. + await expect(errorAlert).toBeVisible({ timeout: TIMEOUT.CHART_RENDER }); + + let chartDataSubmitStatus: number | undefined; + let sawAsyncEventPoll = false; + + page.on('response', response => { + const request = response.request(); + const url = response.url(); + + if ( + request.method() === 'POST' && + url.includes('/api/v1/chart/data') && + sliceIdFromChartDataUrl(url) === chart.id + ) { + chartDataSubmitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + sawAsyncEventPoll = true; + } + }); + + // Force-refresh guarantees this exercises the async path + // deterministically, the same way TC1 does for the happy path. + 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( + chartDataSubmitStatus, + 'forced chart-data submission for the broken chart should still be accepted (202) onto the async path', + ).toBe(202); + expect( + sawAsyncEventPoll, + 'the client should have polled /api/v1/async_event/ while the broken query ran', + ).toBe(true); + }).toPass({ timeout: TIMEOUT.CHART_RENDER }); + + // Fix the underlying config and confirm the chart recovers normally, + // rather than staying stuck in an error state. + 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); + + // "Refresh dashboard" re-submits whatever form_data the dashboard already + // has loaded client-side -- it doesn't pick up a chart config change made + // out-of-band via the API. A fresh navigation re-fetches chart metadata + // (including the fixed metric) from the backend. + 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(); + }, +); + +// --------------------------------------------------------------------------- +// TC4 -- Filter change mid-load shows no stale data. +// +// Precondition: GLOBAL_ASYNC_QUERIES enabled, plus Redis and a running +// Celery worker. +// +// The prepopulated example queries here resolve in ~1-2s, too fast to +// reliably race in real time, so the first filter selection's response is +// artificially delayed via page.route() -- this guarantees it's still +// in-flight when the second selection fires, which is what actually +// exercises the client's stale-query guard (chartAction.ts dispatches +// CHART_UPDATE_STOPPED to abort a superseded query's controller when a new +// one starts for the same chart) rather than two selections that happen to +// resolve one after another with no overlap. +// +// The filter is single-select (multiSelect: false) specifically so each +// click replaces the prior selection instead of accumulating -- the failure +// mode from an earlier manual pass against a multi-select filter, per +// gaq-test-cases.md's automation notes for this test case. +// +// CI green => after rapidly selecting "boy" (delayed) then "girl" +// (undelayed) with no wait in between, the chart settles on +// girl's count and never flips to boy's count even after +// waiting well past the artificial delay -- proving the +// superseded "boy" job's late result never clobbers the screen. +// CI red => the chart shows boy's count at any point after girl's +// selection was applied (stale data winning the race), or the +// two counts are indistinguishable (test can't actually tell). +// --------------------------------------------------------------------------- +async function selectGenderFilterOption( + page: Page, + genderValue: string, +): Promise<void> { + const filterCombobox = page + .locator('[data-test="form-item-value"]') + .first() + .locator('[role="combobox"]'); + await filterCombobox.click(); + await page + .locator('.ant-select-item-option', { + hasText: new RegExp(`^${genderValue}$`), + }) + .first() + .click(); + await page.keyboard.press('Escape'); +} + +async function clickApplyFilters(page: Page): Promise<void> { + const applyBtn = page.locator( + '[data-test="filter-bar__apply-button"], [data-test="filterbar-action-buttons"] button[type="submit"]', + ); Review Comment: Resolved ✅ -- 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]
