sadpandajoe commented on code in PR #41437: URL: https://github.com/apache/superset/pull/41437#discussion_r3779924312
########## superset-frontend/playwright/components/modals/DrillDetailModal.ts: ########## @@ -0,0 +1,114 @@ +/** + * 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. + */ + +import { Locator, Page } from '@playwright/test'; +import { Modal } from '../core'; + +/** + * The "Drill to detail" modal (`DrillDetailModal.tsx`), opened from a chart's + * "More Options" menu or its right-click context menu. Renders the chart's + * underlying sample rows, optionally scoped to a drilled-by value, via the + * `/datasource/samples` API. + */ +export class DrillDetailModal extends Modal { + private static readonly SELECTORS = { + ROW_COUNT_LABEL: '[data-test="row-count-label"]', + METADATA_BAR: '[data-test="metadata-bar"]', + FILTER_COLUMN: '[data-test="filter-col"]', + FILTER_VALUE: '[data-test="filter-val"]', + PAGE_ITEM: '.ant-pagination-item', + ACTIVE_PAGE_ITEM: '.ant-pagination-item-active', + GRID_CELL: '.virtual-table-cell', + } as const; + + private readonly specificLocator: Locator; + + constructor(page: Page) { + super(page); + this.specificLocator = page.getByRole('dialog', { + name: /^Drill to detail:/, + }); + } + + override get element(): Locator { + return this.specificLocator; + } + + /** + * The applied-filter value tags (`<col>=<val>`). Empty when the drill was + * whole-chart (no row/point-level filter applied). + */ + get filterValues(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.FILTER_VALUE); + } + + /** The applied-filter chip(s); each is closable via its own "Close" icon. */ + get filterColumns(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.FILTER_COLUMN); + } + + /** Row-count label above the results grid, e.g. "1-50 of 500 rows". */ + get rowCountLabel(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.ROW_COUNT_LABEL); + } + + /** The metadata bar (column/row summary) shown once samples have loaded. */ + get metadataBar(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.METADATA_BAR); + } + + /** Pagination page-number items below the results grid. */ + get pageItems(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.PAGE_ITEM); + } + + /** The currently active pagination page-number item. */ + get activePageItem(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.ACTIVE_PAGE_ITEM); + } + + /** Cells of the virtualized results grid. */ + get gridCells(): Locator { + return this.element.locator(DrillDetailModal.SELECTORS.GRID_CELL); + } + + /** + * Removes the first applied filter by clicking its chip's Close icon, + * re-fetching the unfiltered samples. + */ + async clearFirstFilter(): Promise<void> { + await this.filterColumns.first().getByLabel('Close').click(); + } + + /** Navigates to the given 1-indexed pagination page. */ + async goToPage(pageNumber: number): Promise<void> { + await this.pageItems.nth(pageNumber - 1).click(); + } + + /** Re-fetches the current samples query, resetting pagination to page 1. */ + async reload(): Promise<void> { + await this.element.getByRole('button', { name: 'Reload' }).click(); + } + + /** Closes the modal via its footer Close button. */ + async close(): Promise<void> { + await this.clickFooterButton('Close'); + await this.waitForHidden(); + } Review Comment: Fixed in 0ea1398651 — `close()` now targets `[data-test="close-drilltodetail-modal"]` instead of the visible "Close" text, matching `DeleteConfirmationModal`'s existing pattern for the same i18n concern. ########## superset-frontend/playwright/tests/dashboard/dashboard-drill-to-detail.spec.ts: ########## @@ -0,0 +1,740 @@ +/** + * 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. + */ + +/** + * E2E migration of the Cypress "Drill to detail modal" suite + * (dashboard/drilltodetail.test.ts). + * + * Drill to detail lets a viewer open a modal of the underlying sample rows for a + * chart — optionally filtered to a single data point — by either the chart's + * "More Options" header menu or a right-click context menu on the chart body. + * The modal calls the real `/datasource/samples` API, so this is genuinely + * end-to-end: each test API-builds a hermetic dashboard from the `birth_names` + * dataset, renders it in the browser, drives the real menus, and asserts the + * resulting backend round-trip (the samples POST and the filter the modal + * applies). + * + * Why the original suite was fully `describe.skip`: + * "it has issues with autoscrolling and the locked title flakes intricately + * when the rightClick is obstructed by the title." + * That failure mode is Cypress-specific — Cypress auto-scrolls the target under + * the sticky chart header before every action. Playwright scrolls once and the + * target stays put, so the entry points are portable here. + * + * What is migrated, and how it is kept deterministic: + * - Modal mechanics (open from header menu, pagination, reload-resets-page) + * and the no-filter big-number drill use stable DOM elements. + * - Table and Pivot drills right-click real DOM cells (no canvas pixels). + * - Canvas (echarts) charts — Pie, Line, Scatter, generic/smooth/step + * time-series, Mixed, Box plot, Funnel, Gauge, Treemap — DID rely on + * hard-coded pixel coordinates in Cypress to land on a specific slice/point. + * Instead of reproducing those brittle pixels, these tests scan a stable + * region of the canvas (see `rightClickCanvasDatum`), read whichever value + * the drill submenu actually offers for the point under the cursor, drill by + * that value, and assert the SAME value round-trips into the modal filter. + * This exercises the full canvas → contextmenu → datum → samples pipeline + * while staying independent of exact geometry. `Big Number with Trendline` + * drills the whole chart (no datum filter), like `Big Number`. + * + * Excluded (kept out, matching the original's own `describe.skip`s): Bar, Area, + * World Map, Radar — skipped upstream for chart-specific reasons. + */ +import { + testWithAssets, + expect, + type TestAssets, +} from '../../helpers/fixtures'; +import type { Page, TestInfo } from '@playwright/test'; +import { TIMEOUT } from '../../utils/constants'; +import { DashboardPage } from '../../pages/DashboardPage'; +import { createDashboardWithCharts } from './dashboard-test-helpers'; + +const DATASET_NAME = 'birth_names'; + +/** + * Parse a RowCountLabel value ("75.7k rows", "1,234 rows") into a number so + * tests can assert the *invariant* (filtered < unfiltered) without hard-coding + * the dataset-specific totals the original Cypress suite baked in. + */ +function parseRowCount(text: string): number { + const m = text.match(/([\d.,]+)\s*([kKmM]?)/); + if (!m) return NaN; + let n = parseFloat(m[1].replace(/,/g, '')); + const suffix = m[2].toLowerCase(); + if (suffix === 'k') n *= 1e3; + if (suffix === 'm') n *= 1e6; + return n; +} + +interface ChartSpec { + vizType: string; + chartNamePrefix: string; + params: Record<string, unknown>; +} + +/** + * API-build a hermetic single-chart dashboard from birth_names and return its + * dashboard and chart ids. Thin single-chart wrapper around + * `createDashboardWithCharts`, the build helper shared by the other migrated + * dashboard specs — reused here rather than hand-rolling position-json and id + * extraction again. + */ +async function buildSingleChartDashboard( + page: Page, + testAssets: TestAssets, + testInfo: TestInfo, + spec: ChartSpec, +): Promise<{ dashboardId: number; chartId: number }> { + const { dashboardId, charts } = await createDashboardWithCharts( + page, + testAssets, + testInfo, + { + datasetName: DATASET_NAME, + chartNamePrefix: spec.chartNamePrefix, + dashboardTitlePrefix: spec.chartNamePrefix, + chartSpecs: [{ viz_type: spec.vizType, params: spec.params }], + }, + ); + return { dashboardId, chartId: charts[0].id }; +} + +/** + * Right-click an echarts canvas until a data point is hit — i.e. until the + * context menu offers an *enabled* "Drill to detail by" submenu (a miss renders + * that item disabled, as a plain menu item rather than a submenu title). + * + * echarts renders to a single canvas, so there is no per-datum DOM element to + * target and the exact pixel of a mark depends on chart geometry (donut hole, + * legend size, axis padding). Rather than hard-code Cypress's brittle pixel + * coordinates, this scans a small set of candidate points — a radial ring for + * pie/radial charts, a grid for cartesian charts — and stops at the first that + * lands on a mark. The drill value is then whatever that mark represents, so the + * caller asserts a value round-trip rather than a specific geometry. + */ +async function rightClickCanvasDatum( + page: Page, + dashboard: DashboardPage, + canvas: ReturnType<Page['locator']>, + pattern: 'ring' | 'grid' | 'dense', +): Promise<void> { + const box = await canvas.boundingBox(); + if (!box) throw new Error('canvas has no bounding box'); + + const ringPoints = (): Array<{ x: number; y: number }> => { + const pts: Array<{ x: number; y: number }> = []; + const cx = box.width / 2; + const cy = box.height / 2; + const minSide = Math.min(box.width, box.height); + for (const rf of [0.3, 0.22, 0.38]) { + for (let a = 0; a < 360; a += 45) { + const rad = (a * Math.PI) / 180; + pts.push({ + x: cx + Math.cos(rad) * minSide * rf, + y: cy + Math.sin(rad) * minSide * rf, + }); + } + } + return pts; + }; + const gridPoints = (): Array<{ x: number; y: number }> => { + const pts: Array<{ x: number; y: number }> = []; + for (const yf of [0.5, 0.4, 0.6, 0.3, 0.7]) { + for (const xf of [0.3, 0.45, 0.6, 0.2, 0.75]) { + pts.push({ x: box.width * xf, y: box.height * yf }); + } + } + return pts; + }; + + // 'dense' merges both scans for radial/stacked shapes (gauge, funnel, box + // plot) whose drillable marks don't fall neatly on a single ring or grid. + let candidates: Array<{ x: number; y: number }>; + if (pattern === 'ring') candidates = ringPoints(); + else if (pattern === 'grid') candidates = gridPoints(); + else candidates = [...gridPoints(), ...ringPoints()]; + + // The submenu *title* element only exists when "Drill to detail by" is an + // enabled submenu (a real datum was hit); a miss renders a disabled item. + const enabledDrillBy = dashboard.drillBySubmenuTitle(); + + for (const pt of candidates) { + await canvas.click({ button: 'right', position: pt }); + const hit = await enabledDrillBy + .waitFor({ state: 'visible', timeout: 400 }) + .then(() => true) + .catch(() => false); + if (hit) return; + await page.keyboard.press('Escape'); + } Review Comment: Fixed in 0ea1398651 — after Escape, the scan loop now waits (up to 400ms, best-effort) for the `[data-test="chart-context-menu"]` portal to actually leave the DOM before the next right-click, instead of assuming the Escape took effect immediately. -- 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]
