sadpandajoe commented on code in PR #41437:
URL: https://github.com/apache/superset/pull/41437#discussion_r3786536086
##########
superset-frontend/playwright/pages/DashboardPage.ts:
##########
@@ -454,4 +455,113 @@ export class DashboardPage {
return { heightBefore: boxBefore.height, heightAfter: boxAfter.height };
}
+
+ //
---------------------------------------------------------------------------
+ // Drill to detail
+ //
+ // Charts that implement the DRILL_TO_DETAIL behavior expose two entry
points:
+ // the chart's "More Options" header menu, and a right-click context menu on
+ // the chart body (a cell, the big-number value, or a canvas data point).
Both
+ // open the same DrillDetailModal, which renders the underlying sample rows
for
+ // the (optionally filtered) chart by calling the `/datasource/samples` API.
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Open the "Drill to detail" item from a chart's "More Options" header menu.
+ * This is the whole-chart entry point (no row-level filters applied).
+ */
+ async openDrillToDetailFromMenu(chartId: number): Promise<void> {
+ const moreOptions = new Button(
+ this.page,
+ this.getChart(chartId).getByLabel('More Options', { exact: true }),
+ );
+ await moreOptions.click();
+ await this.page
+ .getByRole('menuitem', { name: 'Drill to detail', exact: true })
+ .click();
+ }
+
+ /**
+ * The DrillDetailModal dialog (titled "Drill to detail: <chart name>").
+ */
+ drillModal(): DrillDetailModal {
+ return new DrillDetailModal(this.page);
+ }
+
+ /**
+ * Click the plain "Drill to detail" item in an open chart context menu
+ * (whole chart, no row-level filter).
+ */
+ async contextMenuDrillToDetail(): Promise<void> {
+ await this.page
+ .getByRole('menuitem', { name: 'Drill to detail', exact: true })
+ .click();
+ }
+
+ /**
+ * The "Drill to detail by" submenu parent (title) in an open context menu.
+ * Targeted by its submenu-title element rather than role+name because antd
+ * appends the arrow-icon name ("right") to the accessible name, and the leaf
+ * items ("Drill to detail by boy") would otherwise match a role+name lookup.
+ */
+ drillBySubmenuTitle(): Locator {
+ return this.page.locator('.ant-dropdown-menu-submenu-title', {
+ hasText: 'Drill to detail by',
+ });
+ }
+
+ /**
+ * The chart context menu's Menu component, scoped to the open context
+ * menu's root. Used to open the "Drill to detail by" submenu robustly:
+ * plain hover is not reliably picked up by Ant Design's submenu trigger in
+ * headless Chromium, so this falls back to keyboard and dispatchEvent - see
+ * {@link Menu.openSubmenu}.
+ */
+ private contextMenu(): Menu {
+ return new Menu(this.page, '[data-test="chart-context-menu"]');
+ }
+
+ /**
+ * Opens the "Drill to detail by" submenu and returns its popup, containing
+ * the leaf value items (e.g. "Drill to detail by boy").
+ */
+ private openDrillBySubmenu(): Promise<Locator> {
+ return this.contextMenu().openSubmenu('Drill to detail by', {
+ popupSelector: '.chart-context-submenu',
+ });
+ }
+
+ /**
+ * From an open chart context menu, open the "Drill to detail by" submenu and
+ * click the entry for a specific value (e.g. "boy", "1965", "all").
+ */
+ async contextMenuDrillToDetailBy(value: string): Promise<void> {
+ const popup = await this.openDrillBySubmenu();
+ // Use dispatchEvent instead of click to bypass viewport and pointer
+ // interception issues - see Menu.selectSubmenuItem.
+ await popup
+ .getByRole('menuitem', {
+ name: `Drill to detail by ${value}`,
+ exact: true,
+ })
+ .dispatchEvent('click');
Review Comment:
Confirmed real — `aria-label` carries the raw `formattedVal`, the visible
menu text goes through `<StyledFilter stripHTML>` (see
`useDrillDetailMenuItems/index.tsx`), so they diverge for HTML-formatted
values. It's narrower than it looks though: the returned string here is used
both to drive the click (needs the accessible-name form) and to assert against
the modal's *displayed* filter chip (needs the stripped form) — a single string
can't serve both when markup is present, and the `aria-label` actually lives on
a `TruncatedMenuLabel` descendant rather than the `[role="menuitem"]" itself,
so reading it directly off the menuitem wouldn't have worked either. Every
value this suite currently exercises (birth_names gender/state/temporal, 'all')
is markup-free, so it isn't reachable today; documented the limitation in
`f0a34b8839` rather than reworking the round-trip, since a correct fix means
decoupling the click value from the assertion value — out of scope for this
migration P
R.
##########
superset-frontend/playwright/tests/dashboard/dashboard-drill-to-detail.spec.ts:
##########
@@ -0,0 +1,747 @@
+/**
+ * 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, ''));
Review Comment:
Verified this can't occur in this codebase: `RowCountLabel` formats via
`getNumberFormatter()`, which resolves to the single, fixed `DEFAULT_D3_FORMAT`
(`decimal: '.'`, `thousands: ','`) — confirmed via a repo-wide search for
`formatLocale` overrides, there are none. Superset doesn't localize number
formatting at runtime, so a comma-decimal row-count string like '75,7k' is not
something `parseRowCount` will ever see, regardless of UI language.
--
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]
