drivaspreset commented on code in PR #43004: URL: https://github.com/apache/superset/pull/43004#discussion_r3778643887
########## superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts: ########## @@ -0,0 +1,1061 @@ +/** + * 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. Review Comment: Test cases are now split - **File split**: 1061 lines → `global-async-query.spec.ts` (pipeline works for each consumer: cold cache, cache-hit shortcut, many charts, filter values) + `global-async-query-resilience.spec.ts` (where the machinery becomes observable: broken query, race, lost token, teardown mid-flight). Let me know if you meant to create specific spec files per each ########## superset-frontend/playwright/tests/dashboard/global-async-query.spec.ts: ########## @@ -0,0 +1,1061 @@ +/** + * 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 Review Comment: Reference removed: - **MD references**: removed — those design docs aren't checked in, so the pointers went nowhere. Each test's rationale now lives in its own comment. ########## superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts: ########## @@ -300,3 +308,169 @@ export async function createDashboardWithCharts( return { dashboardId, charts }; } + +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', + * dashboardTitlePrefix: 'gaq_tc1_cold_cache', + * chartSpecs: [{ viz_type: 'big_number_total', params: { metric: 'count' } }], + * }); + * 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 => + dashboard + .getChart(chart.id) + .locator('.superset-legacy-chart-big-number .header-line'), + ); + + await dashboard.gotoById(dashboardId); + await dashboard.waitForLoad(navigateOptions); + + return { dashboardId, charts, dashboard, valueLocators }; +} + +export interface ChartAsyncSignals { + /** Status of the chart-data POST matching `matchSliceId`, once observed. */ + submitStatus?: number; + /** Whether the client polled `/api/v1/async_event/` at least once. */ + sawAsyncEventPoll: boolean; + /** Whether the client fetched the final payload from `/api/v1/chart/data/<cache_key>`. */ + sawFinalCachedFetch: boolean; +} + +/** + * Attaches a `page.on('response', ...)` listener that records the three GAQ + * lifecycle signals for a single chart-data request: the submission's status, + * whether the client polled the async-event endpoint, and whether it fetched + * the final cached payload. + * + * `matchSliceId` distinguishes a chart's own chart-data request (a numeric + * slice id) from a native filter's value fetch, which carries no slice id at + * all (see `sliceIdFromChartDataUrl`) -- pass `undefined` to track a + * filter-value request instead of a chart's. + * + * Returns a single mutable object, rather than a tuple of `let` bindings, so + * callers can read the latest values from inside a `toPass` retry block + * without closing over stale variables. + */ +export function trackChartAsyncSignals( + page: Page, + matchSliceId: number | undefined, +): ChartAsyncSignals { + const signals: ChartAsyncSignals = { + submitStatus: undefined, + sawAsyncEventPoll: false, + 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) === matchSliceId + ) { + signals.submitStatus = response.status(); + return; + } + if (request.method() === 'GET' && url.includes('/api/v1/async_event/')) { + signals.sawAsyncEventPoll = true; + return; + } + if ( + request.method() === 'GET' && + /\/api\/v1\/chart\/data\/qc-/.test(url) + ) { + signals.sawFinalCachedFetch = true; + } + }); + + return signals; +} + +export interface MultiChartAsyncSignals { + /** Chart-data submit status keyed by slice id, for the charts in `chartIds`. */ + submitStatusBySliceId: Map<number, number>; + asyncEventPollCount: number; + finalFetchCount: number; +} + +/** + * Same signals as {@link trackChartAsyncSignals}, shaped for a dashboard with + * several charts in flight at once: each chart's own submit status is kept + * (keyed by slice id) instead of a single status, and poll/final-fetch events + * are counted instead of recorded as a single boolean, since they arrive from + * every chart concurrently. + */ +export function trackMultiChartAsyncSignals( Review Comment: Fixed: - **`trackMultiChartAsyncSignals` vs `trackChartAsyncSignals`**: collapsed into a single `trackGaqSignals` rather than having one call the other — two listeners would each see page-wide traffic and double-count polls. One listener, per-slice status map, counted poll/fetch events; single-chart callers read `submitStatusFor(id)`, the busy-dashboard test reads the counts. -- 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]
