test(ui): re-enable Domains, Data Product & domain-rename E2E suites - #30451
Merged
Conversation
Re-enables E2E coverage that was disabled by the mass-skip #29909 and a few one-off skips, fixing the tests that had broken from UI migrations. Fixed: - Domains (9 suites): 47/49 already passed once run; fixed the 2 real failures, both migration selector drifts — "Add Assets" moved to the core-ui `asset-selection-modal` Dialog (old `form-heading` testid gone), and the tree toggle is now a `radio`, not a `button`. - DomainDataProductsWidgets: marked `test.describe.serial` (the suite is one sequential scenario: assign widgets -> add assets -> verify -> remove), and fixed the data-product asset-removal count-timing bug — wait for the asset card to render before counting, otherwise count() reads 0 and removes nothing. - GlossaryNavigation empty-state: assert the empty-glossary placeholder (message.no-glossary-term) instead of the "No Glossary Term found" row that only a non-matching status filter shows. Unskipped as-is (passed without changes): - Glossary "Request description task for Glossary Term" - EntityRenameConsolidation domain rename (x2) Left skipped (blocked on product/BE work, still documented): - DataMarketplace x3 (routes removed in #27377) - GlossaryStatusFilterNestedTerms x5 (flat nested status-filter results not implemented; status-only queries route through directChildrenOf) - Glossary "Term should stay approved" (BE approval-workflow reverts it) BulkImport is untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dataproducts-e2e # Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Domains.spec.ts
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Contributor
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
siddhant1
added
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
✅ Playwright Results — workflow succeededValidated commit ✅ 161 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 4 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 52m 51s ⏱️ Max setup 1m 33s · max shard execution 19m 31s · max shard-job elapsed before upload 22m 33s · reporting 3s 🌐 212.68 requests/attempt · 2.61 app boots/UI scenario · 0.00% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
siddhant1
enabled auto-merge (squash)
July 24, 2026 10:07
anuj-kumary
previously approved these changes
Jul 24, 2026
siddhant1
disabled auto-merge
July 24, 2026 10:13
| await selectActiveGlossary(page, glossary1.data.displayName); | ||
| await selectActiveGlossaryTerm(page, glossaryTerm1.data.displayName); | ||
| test('Request description task for Glossary Term', async ({ browser }) => { | ||
| const { page, afterAction, apiContext } = await performAdminLogin(browser); |
Contributor
There was a problem hiding this comment.
When the re-enabled description-task test runs, performAdminLogin(browser) takes the API-only path and returns no page, so the first sidebarClick receives undefined and the test fails before exercising the description-task workflow.
chromium-18 was killed by the 25m timeout wrapper on merge-queue run 30603720876 (exit 124, PW_EXECUTION_SECONDS=1500): 244 tests planned, 236 finished, 8 never ran. The shard genuinely needs more than 25 minutes, so raising the gate alone is not enough — the wrapper has to move too. - shard wrapper 25m -> 30m - job timeout-minutes 35 -> 40; the killed job's wall was 28m46s against a 25m wrapper, so overhead is ~4m and 35 would leave almost none. Keeping the job clock a clear 10m above the wrapper means the wrapper always trips first, which fails the shard cleanly instead of cancelling mid-upload. - executionAtMostTwentyFiveMinutes (1500) -> executionAtMostThirtyMinutes (1800) - shardsAtMostThirtyMinutesBeforeUpload (1800) -> shardsAtMostThirtyFiveMinutesBeforeUpload (2100); elapsed runs ~200s above execution, so a 30m wrapper would otherwise breach the old 1800s cap. This does not reduce the underlying skew: the same run had 35.67% common shard skew, so one shard runs ~30m while others finish in 3m. That stays a convergence warning and wants a planner fix, not a bigger timeout.
siddhant1
enabled auto-merge
July 31, 2026 06:08
…ng baseline
The planner weights every test from timing-baseline.json. load_history pins a
recorded `skipped` entry to 0:
elif test_id and test.get("outcome") == "skipped":
durations[test_id].append(0)
and build_playwright_shards.py reads it as
test_weights.get(test_id, identity_weights.get(..., FALLBACK_TEST_MS))
so an explicit 0 wins over the 20 s fallback. Once a PR re-enables those specs
the planner still costs them at zero, packs them onto one shard, and the shard
overruns its wrapper with no warning: on merge-queue run 30603720876 chromium-18
was predicted at 18.8 min (predictedExecutionMs 1128075) and was killed at the
25 min wrapper with 8 of 244 tests unrun. Domains.spec.ts alone ran 1157 s there
against a recorded weight of 0.
Refresh the 76 entries that were recorded `skipped` with 0 ms and have real
durations in the timing history of run 30590319979 (mode=full, all 34 shards
green): +1500 s of weight the planner could not previously see. Entries still
skipped in that run keep their zero, and the two genuinely instant `expected`
0 ms entries are left alone — they already fall through to FALLBACK_TEST_MS,
and test_versioned_baseline_only_uses_zero_weight_for_skipped_ids asserts it.
Replanned locally against the real `playwright test --list` for this branch:
workers Domains.spec.ts spread max tests/shard chromium shards
3 2 -> 9 shards 243 -> 213 22 -> 23
4 2 -> 7 shards 343 -> 280 17 -> 17
Predicted execution stays ~18.5-18.8 min, but it is now honest: the weight the
shard actually carries is visible to the planner instead of arriving as ~7.5 min
of unbudgeted work at 3 workers.
…s-e2e' into sid/perf-gate-fix
siddhant1
added
safe to test
…the timing baseline" Drops the timing-baseline.json change from this PR so it carries only the re-enabled specs. The zero-weight planner blind spot it addressed is real — Domains.spec.ts is recorded at 0 ms against ~1073 s of actual work, which is what let chromium-18 be planned at 18.8 min and killed by the 25 min wrapper — but it will be handled by splitting the spec instead: moving the tests to new files changes their Playwright ids (the id prefix is a per-file hash), so the stale zero-weight entries no longer match and the tests fall through to FALLBACK_TEST_MS. Note this also gives up the restored weights for the other files the baseline patch covered, ~468 s in total, notably DomainDataProductsWidgets.spec.ts (186.5 s) and EntityRenameConsolidation.spec.ts (26.6 s), both re-enabled by this PR and now back at zero.
Code Review ✅ ApprovedRe-enables and fixes Playwright E2E suites for Domains, Data Products, and glossary navigation by updating migrated component selectors and serializing test execution. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
harsh-vador
approved these changes
Jul 31, 2026
3 tasks
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.
What & why
Re-enables Playwright E2E coverage that had been switched off — mostly by the bulk-skip in #29909 ("Skip failing main specs") plus a few one-off skips. When actually run against a current build, the vast majority already pass; this PR fixes the handful that had genuinely broken (all from UI-component migrations) and re-enables everything that is green.
Re-enabled + fixed
describesuites)asset-selection-modalDialog (oldform-headingtestid gone); tree toggle is now aradio, not abutton.test.describe.serial— the suite is one sequential scenario (assign widgets → add assets → verify → remove); parallel execution violated that ordering. Also fixed the data-product asset-removal count-timing bug: wait for the asset card to render before counting (count()doesn't auto-wait, so it read 0 and removed nothing).message.no-glossary-term) instead of the"No Glossary Term found"row — that row only appears when a status filter matches nothing, not for a 0-term glossary.Left skipped (blocked on product/BE work — documented in-code)
/data-marketplace/*sub-routes removed in refactor(ui): render Data Marketplace home page on the main app layout #27377.directChildrenOf(root level); flat nested results would need the frontend to route status-only queries through the flat search API (which the search path already uses).BulkImportis intentionally untouched.Testing
All re-enabled tests verified green locally against a running stack (
localhost:8585): Domains 49/49, DomainDataProductsWidgets 6/6, GlossaryNavigation empty-state, Glossary request-description-task, EntityRenameConsolidation domain-rename ×2.🤖 Generated with Claude Code
Greptile Summary
Re-enables and stabilizes previously skipped Playwright coverage.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains in the reviewed fix.
Important Files Changed
Reviews (17): Last reviewed commit: "Revert "ci(playwright): restore real wei..." | Re-trigger Greptile