Repository navigation
test(dashboard): migrate edit-mode component E2E tests to Playwright - #41438
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41438 +/- ##
========================================
Coverage 65.79% 65.79%
========================================
Files 2842 2835 -7
Lines 162106 161365 -741
Branches 37148 37055 -93
========================================
- Hits 106653 106167 -486
+ Misses 53388 53156 -232
+ Partials 2065 2042 -23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sadpandajoe
force-pushed
the
dashboard-pw-editmode
branch
from
June 26, 2026 19:27
47d9cce to
359f932
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
sadpandajoe
force-pushed
the
dashboard-pw-editmode
branch
2 times, most recently
from
July 7, 2026 14:11
27ff58a to
071b48d
Compare
sadpandajoe
force-pushed
the
dashboard-pw-editmode
branch
from
July 14, 2026 14:22
071b48d to
10e50dc
Compare
sadpandajoe
force-pushed
the
dashboard-pw-editmode
branch
from
July 22, 2026 18:00
10e50dc to
dac4db8
Compare
Port the Cypress dashboard edit-mode 'Components' tests (add chart, remove chart, add markdown) to Playwright as true E2E. The dashboard grid uses react-dnd with the HTML5 backend, so drags are driven by synthetic native drag events with a shared DataTransfer (new helpers/dnd.ts). Tests are hermetic: charts/dashboards are created from birth_names via the API and cleaned up by the testAssets fixture. The 21 skipped 'Color consistency' Cypress tests are intentionally not ported: they assert per-series colors via an .nv-legend-symbol SVG fill that no longer exists (ECharts renders to canvas, not DOM-inspectable); that color-precedence logic is covered by Jest/RTL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sadpandajoe
force-pushed
the
dashboard-pw-editmode
branch
from
July 25, 2026 04:06
dac4db8 to
8435a9c
Compare
Follow-up to the edit-mode component migration, applying a review of the new spec, page object and drag-and-drop helper. Correctness: - Resolve the dataset through getDatasetByName and create the chart through apiPostChart instead of calling page.request directly. Both wrap the request in the shared transient-retry helper, which exists because the dev server drops connections under concurrent load; the direct call had no such cover. - The resize assertion was guarded by `if (handleBox && boxBefore)` while only handleBox was asserted non-null, so a null bounding box silently skipped the assertion and the test still passed. DashboardPage.resizeComponent now throws on a missing box and returns the before/after heights for the spec to assert. - Replace the deprecated Locator.type() with pressSequentially(). Reuse over duplication: - Drop the local dashboard-creation helper in favour of createTestDashboard, which already handles both response shapes and cleanup registration; it gains an optional `published` flag. Chart ids come from extractIdFromResponse. - Generated names use the worker's parallel index, matching the convention in the sibling helpers, rather than a performance.now() suffix. Page object and readability: - Move the ace, resize-handle and title selectors out of the spec behind DashboardPage accessors. These stay locators rather than routing through the AceEditor component, whose setText/getText work through the ace API and so would bypass the keystroke path under test and lose web-first retries. - Name the react-dnd settle delay and the markdown retry budgets. - Make openBuilderTab private, drop the unused deleteChartHolder index, remove the unused GRID_CONTENT selector, correct the dropTarget docstring, type layout-element labels as a union, and prefix the locator getters with `get` to match the rest of the class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
waitForChartsToLoad only checked that no loading indicator was visible. Loader absence is also the state of a dashboard whose grid has not mounted yet, so calling it straight after waitForLoad — which resolves on the header container, rendered well before the grid — returned immediately and let callers assert against an empty page. Wait for at least one chart grid component to attach first, so the absence of loaders means "charts finished" rather than "charts have not begun". applyFiltersIfEnabled swallowed locator-resolution errors through `.catch(() => false)`, collapsing a missing Apply button into the same silent no-op as a disabled one. Wait for the button, then branch on enabled-ness only. Also switch waitForLoad, enterEditMode, openHeaderActionsMenu and openNativeFiltersConfigModal from page.click/page.waitForSelector to the locator idiom used everywhere else in the file, correct the blurToDashboardTitle docstring (clicking the title focuses its input in edit mode — a state change, not a no-op), note that dropTarget only resolves while the grid is empty, and inline the createDashboard passthrough in the edit-mode spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ards Hardening waitForChartsToLoad to require a chart grid component broke theme.spec, whose dashboard is created from dashboard_title/published/ theme_id alone — apiPostDashboard passes the payload straight through, so that dashboard has no position_json and its grid renders empty. No chart component ever attaches and the wait times out. Previously the loader-only check passed vacuously there, which is the same weakness the hardening removed, so restoring the old call is not an option. Add waitForGridToLoad for that case: the grid container renders whatever the grid holds, so it gives the "got past the header" evidence a chart component cannot. Waits for it attached rather than visible, since an empty grid collapses to zero height. Extract the shared loader-settle step into a private helper; waitForChartsToLoad keeps its behavior unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n commit Two review follow-ups on the edit-mode Playwright migration: - Delete cypress-base/cypress/e2e/dashboard/editmode.test.ts. The Playwright spec is a strict superset of its single remaining test, so leaving the Cypress file tracked duplicated coverage rather than reducing it. Its `drag` helper in cypress/utils/index.ts had no other callers and goes with it. - The markdown commit assertion could pass without the component leaving edit mode: the wrapper contains "Test resize" while ace is still mounted. Assert the ace document is gone first, which is what the preview swap in Markdown.tsx actually produces. Verified by mutation — removing the blur call now fails the test. Also corrects the file header, which described a "Color consistency" block that upstream had already dropped from the Cypress spec. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolves the DashboardPage conflict introduced by #42017, which extracted the native-filter-bar methods into a DashboardFilterBar component. That component already carries both refinements this branch made to openNativeFiltersConfigModal and applyFiltersIfEnabled, so master's side is taken for the whole filter-bar surface; the edit-mode helpers and the locator-based wait rework are kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three tests opened with the same nine-line preamble: create the empty published dashboard, construct the page object, navigate, wait, enter edit mode. Collapse it into openEmptyDashboardInEditMode, which absorbs the dashboard options that were hoisted into a module constant for the same reason. A helper rather than a beforeEach hook: a hook cannot return the page object, so the dashboard would have to become a file-scoped binding mutated between tests. Each test keeps its setup explicit and self-contained instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
waitForLoadersToSettle read getComputedStyle().display on each loader element itself. display does not inherit, so a spinner inside a display:none ancestor computes to its own display:block and was treated as visible, hanging the wait until the timeout rather than resolving. Use Playwright's :visible engine, which accounts for ancestors. Verified against a live browser: for a loader under a display:none ancestor the old check reports display:block/visible while :visible correctly matches zero elements. The already-settled case still resolves in ~2ms, so there is no timeout penalty. Drops the opacity === '0' branch as dead: the sole producer of aria-label="Loading" is the Loading component, which renders opacity 0.25 when muted and 1.0 otherwise, never 0. Also inlines builderTabs() into its only caller and drops a conditional spread that JSON serialization already handles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three dashboard specs each hand-rolled the same json_metadata envelope for a filtered dashboard, differing only in the filter's defaultDataMask. Extract buildSelectFilter/buildFilterJsonMetadata so the envelope has one definition. The generated metadata is byte-identical to the literals it replaces. Also drop the local findDatasetIdByName in the Mixed-chart spec: it issued a raw page.request.get, which bypasses the transient-retry wrapper every other API call in the suite goes through, and hand-rolled the rison filter. The shared getDatasetByName does both correctly. Route the two id reads in these specs through extractIdFromResponse so they tolerate either response shape.
theme.spec asserted twice on the same thing: the 403 array was populated under the same /api/v1/theme/ guard as the request array, so it could only be non-empty when the first assertion had already failed. Record the status alongside each theme request instead, keeping the 403 signal in the failure output with one assertion doing the work. delete-display-control logged four console lines that no assertion read, one under a name (goneAfterSave) that meant the opposite of the value it held. The screenshots at each step already tell that story. Name the deliberately non-fatal chart wait (settleCharts) rather than repeating a bare .catch three times, and record why the pause before the final assertion is load-bearing: toHaveCount(0) passes the moment the control is absent, so the wait is what gives a reappearance the window this regression test exists to catch.
sadpandajoe
marked this pull request as ready for review
July 31, 2026 21:11
sadpandajoe
requested review from
eschutho,
geido,
mistercrunch and
rusackas
as code owners
July 31, 2026 21:11
Contributor
Code Review Agent Run #7f09afActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Contributor
|
The flagged issue is correct. Because the resize-stop handler updates the layout asynchronously via React state, reading the bounding box immediately after In your test, you can replace the immediate measurement with a polling assertion or a wait condition that verifies the height has changed, ensuring the DOM has settled after the resize event. superset-frontend/playwright/pages/DashboardPage.ts |
Contributor
Code Review Agent Run #ee4dd1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
approved these changes
Aug 6, 2026
rusackas
left a comment
rusackas
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks for chasing down the race condition Codeant flagged in the resize helper. Approving!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Migrates the Cypress dashboard edit-mode tests to Playwright as part of the ongoing Cypress → Playwright E2E migration, and deletes the superseded
cypress-base/cypress/e2e/dashboard/editmode.test.tsso the change actually reduces Cypress coverage instead of duplicating it. Itsdraghelper incypress/utils/index.tshad no other callers and is removed with it.The Playwright spec is a strict superset of the deleted one, covering the drag-and-drop workflows that the upstream Cypress notes explicitly flagged as the one part of edit mode that genuinely needs E2E coverage ("Chart drag/drop functionality requires true E2E testing"):
Key design points:
dragstart/dragenter/dragover/dropevents rather than the mouse events Playwright's built-indragTo()produces. A newhelpers/dnd.tsdrives the drag with synthetic native events threaded through a single sharedDataTransferhandle (mirroring the deprecated Cypress synthetic-drag helper).birth_namesdataset via the API and torn down by the existingtestAssetsfixture — no reliance on seeded example dashboards.DashboardPagepage object (extended with edit-mode helpers), thetestWithAssetsfixture, and the API helpers.By the time of this migration the Cypress spec held a single
should add chartstest. ItsColor consistencyblock had already been dropped upstream in #40384 as permanently skipped — it read per-series colors off an.nv-legend-symbolSVGfillthat ECharts, rendering to<canvas>, no longer produces. That color-precedence logic is covered by Jest/RTL unit tests, so forcing it into E2E would be a component test posing as E2E.Why the
DashboardPagewait changes are herewaitForChartsToLoad()previously waited only for loading indicators to disappear, which passes instantly against a dashboard that has not begun rendering — the grid mounts its spinners after the header renders. It now waits for a chart grid component first, so loader absence means "charts finished" rather than "charts have not started". Building the edit-mode page-object helpers on top of a wait with that failure mode was not viable, so the fix rides along in the page object this PR already extends. A siblingwaitForGridToLoad()covers dashboards with no charts, which is whytheme.spec.tsswitches to it — the dashboard it creates has noposition_json, so its grid renders empty and there is no chart to wait for. Happy to split this into its own PR if reviewers would prefer it separate.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — test-only change.
TESTING INSTRUCTIONS
All three tests pass; verified stable under parallel load (
--workers=3 --repeat-each=3, 9/9).ADDITIONAL INFORMATION