From e9e489b5f2dfd3141005fbc4c38431199231bca5 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Mon, 13 Jul 2026 15:31:44 -0700 Subject: [PATCH] test(dashboard): strengthen chart render + per-chart query assertions in load spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address round-2 review feedback on the dashboard-load Playwright migration: - Render proof: wait on each expected `#chart-id-` (the ChartRenderer render marker) instead of snapshotting holder count after the first holder attaches. A chart that never renders now times out and fails the test rather than passing on a partial count. - Query proof: collect chart-data POSTs keyed by the slice_id encoded in the `form_data` query param, then assert every expected chart issued a 200 — not just that one chart-data request succeeded. - Reuse `extractIdFromResponse` for both chart and dashboard creation instead of re-implementing the `result?.id ?? id` normalization twice. - Extract the ROOT/GRID/ROW/CHART position_json scaffold into a typed `buildDashboardPositionJson` helper in dashboard-test-helpers.ts. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../playwright/pages/DashboardPage.ts | 38 +++----- .../tests/dashboard/dashboard-load.spec.ts | 97 ++++++++----------- .../tests/dashboard/dashboard-test-helpers.ts | 50 ++++++++++ 3 files changed, 108 insertions(+), 77 deletions(-) diff --git a/superset-frontend/playwright/pages/DashboardPage.ts b/superset-frontend/playwright/pages/DashboardPage.ts index 75f3f48cb01..381fd986735 100644 --- a/superset-frontend/playwright/pages/DashboardPage.ts +++ b/superset-frontend/playwright/pages/DashboardPage.ts @@ -145,36 +145,30 @@ export class DashboardPage { } /** - * Wait for every chart on the dashboard to finish rendering and return how - * many charts rendered. + * Wait for every expected chart on the dashboard to finish rendering. * - * A chart's `#chart-id-` element only becomes visible once the chart has - * fetched its data and rendered (the same signal the legacy Cypress - * `waitForChartLoad` helper relied on). The chart set is derived from the - * dashboard itself via the `[data-test="chart-grid-component"]` holders, so - * this adapts to any dashboard without hard-coding chart names or counts. + * A chart's `#chart-id-` element is rendered by `ChartRenderer` only in + * the non-loading, non-failed branch of `Chart` (a loading chart shows a + * spinner instead; a failed chart early-returns an error container), so its + * visibility is a genuine "this chart rendered" signal — the same one the + * legacy Cypress `waitForChartLoad` helper relied on. + * + * The expected chart IDs are passed in rather than discovered from the DOM: + * waiting on each specific `#chart-id-` means a chart that never renders + * makes this call time out and fail the test, and it avoids the partial-count + * race of snapshotting `holders.count()` after only the first holder attaches. */ - async waitForAllChartsRendered(options?: { - timeout?: number; - }): Promise { + async waitForAllChartsRendered( + expectedChartIds: number[], + options?: { timeout?: number }, + ): Promise { // Charts issue real backend queries; allow generous time for slow viz types. const timeout = options?.timeout ?? TIMEOUT.API_RESPONSE * 2; - const holders = this.page.locator('[data-test="chart-grid-component"]'); - await holders.first().waitFor({ state: 'attached', timeout }); - - const count = await holders.count(); - for (let i = 0; i < count; i += 1) { - const chartId = await holders.nth(i).getAttribute('data-test-chart-id'); - if (!chartId) { - throw new Error( - `Chart holder ${i} is missing its data-test-chart-id attribute`, - ); - } + for (const chartId of expectedChartIds) { await this.page .locator(`#chart-id-${chartId}`) .waitFor({ state: 'visible', timeout }); } - return count; } /** diff --git a/superset-frontend/playwright/tests/dashboard/dashboard-load.spec.ts b/superset-frontend/playwright/tests/dashboard/dashboard-load.spec.ts index 9d1534cbf6e..54e4a0fc036 100644 --- a/superset-frontend/playwright/tests/dashboard/dashboard-load.spec.ts +++ b/superset-frontend/playwright/tests/dashboard/dashboard-load.spec.ts @@ -37,8 +37,10 @@ import { testWithAssets, expect } from '../../helpers/fixtures'; import { apiPostChart, apiPutChart } from '../../helpers/api/chart'; import { apiPostDashboard } from '../../helpers/api/dashboard'; import { getDatasetByName } from '../../helpers/api/dataset'; +import { extractIdFromResponse } from '../../helpers/api/assertions'; import { TIMEOUT } from '../../utils/constants'; import { DashboardPage } from '../../pages/DashboardPage'; +import { buildDashboardPositionJson } from './dashboard-test-helpers'; const DATASET_NAME = 'birth_names'; @@ -101,51 +103,14 @@ testWithAssets( params: JSON.stringify(spec.params), }); expect(resp.ok()).toBe(true); - const body = await resp.json(); - const chartId: number = body.result?.id ?? body.id; - if (!chartId) { - throw new Error( - `Chart creation for ${spec.viz_type} returned no id: ${JSON.stringify(body)}`, - ); - } + const chartId = await extractIdFromResponse(resp); testAssets.trackChart(chartId); charts.push({ id: chartId, sliceName }); } const chartIds = charts.map(chart => chart.id); // Lay all charts out in a single row. - const chartKeys = chartIds.map(id => `CHART-${id}`); - const positionJson: Record = { - DASHBOARD_VERSION_KEY: 'v2', - ROOT_ID: { type: 'ROOT', id: 'ROOT_ID', children: ['GRID_ID'] }, - GRID_ID: { - type: 'GRID', - id: 'GRID_ID', - children: ['ROW-1'], - parents: ['ROOT_ID'], - }, - 'ROW-1': { - type: 'ROW', - id: 'ROW-1', - children: chartKeys, - parents: ['ROOT_ID', 'GRID_ID'], - meta: { background: 'BACKGROUND_TRANSPARENT' }, - }, - }; - chartIds.forEach((chartId, index) => { - positionJson[chartKeys[index]] = { - type: 'CHART', - id: chartKeys[index], - children: [], - parents: ['ROOT_ID', 'GRID_ID', 'ROW-1'], - meta: { - chartId, - width: 4, - height: 50, - sliceName: charts[index].sliceName, - }, - }; - }); + const positionJson = buildDashboardPositionJson(charts); const dashResp = await apiPostDashboard(page, { dashboard_title: `load_smoke_${uniqueSuffix}`, @@ -153,8 +118,7 @@ testWithAssets( position_json: JSON.stringify(positionJson), }); expect(dashResp.ok()).toBe(true); - const dashBody = await dashResp.json(); - const dashboardId: number = dashBody.result?.id ?? dashBody.id; + const dashboardId = await extractIdFromResponse(dashResp); testAssets.trackDashboard(dashboardId); // Associate every chart with the dashboard so they actually render. @@ -162,15 +126,31 @@ testWithAssets( await apiPutChart(page, chartId, { dashboards: [dashboardId] }); } - // Record the real chart-data round-trips the dashboard makes on load. - const chartDataStatuses: number[] = []; + // Record the real chart-data round-trips the dashboard makes on load, + // keyed by the chart each one queried for. The chart-data POST carries its + // slice id in the encoded `form_data={"slice_id":}` query param (see + // chartAction.ts), so parsing it lets us prove every chart queried — not + // just that some chart did. + const chartDataStatusBySliceId = new Map(); page.on('response', response => { const request = response.request(); if ( - request.method() === 'POST' && - response.url().includes('/api/v1/chart/data') + request.method() !== 'POST' || + !response.url().includes('/api/v1/chart/data') ) { - chartDataStatuses.push(response.status()); + return; + } + const formData = new URL(response.url()).searchParams.get('form_data'); + if (!formData) { + return; + } + try { + const sliceId = JSON.parse(formData).slice_id; + if (typeof sliceId === 'number') { + chartDataStatusBySliceId.set(sliceId, response.status()); + } + } catch { + // Not a slice-id form_data payload; ignore. } }); @@ -178,15 +158,22 @@ testWithAssets( await dashboard.gotoById(dashboardId); await dashboard.waitForLoad(); - // Every chart grid component must reach its rendered state. - const renderedCount = await dashboard.waitForAllChartsRendered(); - expect(renderedCount).toBe(chartIds.length); + // Each expected chart must reach its rendered state; a chart that never + // renders makes this time out and fail rather than passing silently. + await dashboard.waitForAllChartsRendered(chartIds); - // The render came from real backend queries, and all of them succeeded. - expect(chartDataStatuses.length).toBeGreaterThan(0); - expect( - chartDataStatuses.every(status => status === 200), - `all /api/v1/chart/data responses should be 200, got [${chartDataStatuses}]`, - ).toBe(true); + // The render came from real backend queries: every chart issued its own + // chart-data POST and each one succeeded. + for (const chartId of chartIds) { + const status = chartDataStatusBySliceId.get(chartId); + expect( + status, + `chart ${chartId} should have issued a /api/v1/chart/data POST`, + ).toBeDefined(); + expect( + status, + `chart ${chartId}'s /api/v1/chart/data response should be 200`, + ).toBe(200); + } }, ); diff --git a/superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts b/superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts index 9f45661342e..963a8662a45 100644 --- a/superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts +++ b/superset-frontend/playwright/tests/dashboard/dashboard-test-helpers.ts @@ -26,6 +26,56 @@ interface TestDashboardResult { name: string; } +/** A chart to place in a generated dashboard layout. */ +export interface DashboardChartLayout { + id: number; + sliceName: string; +} + +/** + * Build a v2 `position_json` that lays the given charts out in a single row. + * + * Centralizes the ROOT → GRID → ROW → CHART scaffold that every dashboard- + * building E2E test would otherwise hand-roll. + */ +export function buildDashboardPositionJson( + charts: DashboardChartLayout[], +): Record { + const chartKeys = charts.map(chart => `CHART-${chart.id}`); + const positionJson: Record = { + DASHBOARD_VERSION_KEY: 'v2', + ROOT_ID: { type: 'ROOT', id: 'ROOT_ID', children: ['GRID_ID'] }, + GRID_ID: { + type: 'GRID', + id: 'GRID_ID', + children: ['ROW-1'], + parents: ['ROOT_ID'], + }, + 'ROW-1': { + type: 'ROW', + id: 'ROW-1', + children: chartKeys, + parents: ['ROOT_ID', 'GRID_ID'], + meta: { background: 'BACKGROUND_TRANSPARENT' }, + }, + }; + charts.forEach((chart, index) => { + positionJson[chartKeys[index]] = { + type: 'CHART', + id: chartKeys[index], + children: [], + parents: ['ROOT_ID', 'GRID_ID', 'ROW-1'], + meta: { + chartId: chart.id, + width: 4, + height: 50, + sliceName: chart.sliceName, + }, + }; + }); + return positionJson; +} + interface CreateTestDashboardOptions { /** Prefix for generated name (default: 'test_dashboard') */ prefix?: string;