Cherry-pick #30451 to 2.0: re-enable Domains, Data Product & domain-rename E2E suites - #30777
Merged
Conversation
…30451) * test(ui): re-enable Domains, Data Product & domain-rename E2E suites 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> * fix(e2e): repair three specs that broke against current main Domains "Create DataProducts and add remove assets": the assets empty-state CTA lost its data-testid in #30489 — CreatePlaceholder maps the action's id onto the button's DOM id — so select it by id instead. GlossaryNavigation empty state: #30057 replaced the ErrorPlaceHolder text ("...there are no Glossary Terms defined...") with an EmptyPlaceholder, and message.no-glossary-term is now unused. Assert the placeholder testid, matching CustomizeDetailPage.spec.ts. Glossary "Request description task for Glossary Term": restore { navigate: true } so performAdminLogin returns an authenticated page rather than taking the API-only path, and replace the one-shot textContent read with a web-first assertion — it read an empty container before the description rendered, mirroring the sibling glossary-level test. Co-Authored-By: Claude <noreply@anthropic.com> * ci(playwright): track the 25m shard wrapper in the perf gate #30689 raised the shard step's `timeout ... 21m` wrapper to 25m and the job `timeout-minutes` 30 -> 35, but left the blocking performance target at `maxExecutionSeconds <= 1260`. That opened a 21-25 minute band where a shard completes well inside its timeout and every test passes, yet evaluate_playwright_performance.py fails the run and playwright-summary reports it as a CI/reporting failure with zero test failures. Merge-queue run 30590319979 for #30451 landed in exactly that band: all 34 shards green, slowest execution 1396s, blocked solely by this target. Move the target to 1500s so it tracks the wrapper, and rename it to match. * ci(playwright): give shards 30m and keep the gates tracking the wrapper 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. * ci(playwright): restore real weights for re-enabled specs in the timing 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. * Revert "ci(playwright): restore real weights for re-enabled specs in 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. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 64c33fc)
siddhant1
added
UI
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 |
Code Review ✅ ApprovedCherry-picks the commit to re-enable Domains, Data Product, and domain-rename E2E suites by removing skip and fixme markers across five spec files. 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }); | ||
|
|
||
| test.describe.fixme('Domains', () => { | ||
| test.describe('Domains', () => { |
Contributor
There was a problem hiding this comment.
When the re-enabled Domains suite runs fully parallel, the domain-owner test deletes the module-level domain and user created by beforeAll, causing concurrent or later tests that use those shared fixtures to fail with missing-entity or authentication errors.
Contributor
❌ UI Checkstyle Failed❌ Antd + Less Deprecation GuardA new Affected filesat Function._resolveFilename (node:internal/modules/cjs/loader:1401:15) Fix locally (fast - only checks files changed in this branch): make ui-checkstyle-changed |
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
1 participant
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.
Cherry-pick of #30451 onto
2.0.Applied cleanly with no conflicts. See #30451 for the full write-up.
What it carries
Re-enables E2E coverage that was disabled on this branch — 10
test.describe.fixmesuites turned back on and 3test.skips removed across 5 spec files (+133/−166):Pages/Domains.spec.tsfixmes Domains, Domain Rename Comprehensive, Domains Rbac, Data Consumer Domain Ownership, hasDomain()/noDomain() Rule, Domain Tree View, asset dryRunPages/Glossary.spec.tsFeatures/LandingPageWidgets/DomainDataProductsWidgets.spec.tsfixmes Domain and Data Product Asset CountsFeatures/EntityRenameConsolidation.spec.tsFeatures/Glossary/GlossaryNavigation.spec.tsThe substantive selector fix it carries:
CreatePlaceholderapplies its action'sdata-assets-add-buttonas the button's DOM id, not a testid, so the CTA is selected withpage.locator('#data-assets-add-button')instead ofgetByTestId(...).Backport notes
The applied diff is byte-identical to
main's except for one context line —Domains.spec.tsasserts the assets empty state differently on each branch:main:page.getByTestId('empty-placeholder')2.0:page.getByText('No assets linked yet')That divergence is pre-existing (
mainchanged it in a later, unrelated commit) and untouched here. Verified valid on2.0: the string resolves tono-assets-linked-yetin this branch'sen-us.json.Verification
-/+lines identical to the original commit.CreatePlaceholder.tsxandEmptyPlaceholder.utils.tsare identical between2.0andmain, so the DOM-id selector change is valid here —resolveSingleAction's contract ("id, when provided, is applied to the generated button as its DOM id") holds on this branch.2.0source:approve-button,asset-selection-modal,delete-all-button,request-description,viewer-container,table-data-card_,data-assets-add-button.Runtime behaviour of the re-enabled suites on
2.0is not verified locally — that is what this PR's Playwright run is for.Greptile Summary
Re-enables domain, data-product, glossary, and rename Playwright coverage while updating selectors and stabilizing asynchronous asset-removal assertions.
Confidence Score: 4/5
The shared-fixture lifecycle in the re-enabled Domains suite should be fixed before merging because parallel tests can lose their domain or user while still running.
Activating the Domains describe block exposes an individual test that deletes resources owned by the suite-level setup, allowing fully parallel tests to observe missing shared state.
Files Needing Attention: openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Domains.spec.ts
Important Files Changed
Reviews (1): Last reviewed commit: "test(ui): re-enable Domains, Data Produc..." | Re-trigger Greptile