test(e2e): 1.13 skip-list cleanup — re-enable 5 specs that pass, drop 4 that never ran - #30839
Conversation
Backport of dd73bd9 to 1.13. Prerequisite for re-enabling GlossaryNavigation's empty-state test, which 1.13 skips with the note "selectActiveGlossary not selecting the correct glossary". The old selector was `page.getByRole('menuitem', { name }).first()` — unscoped and non-exact, so it matched any menuitem on the page whose accessible name merely *contained* the target, and `.first()` then silently picked whichever came first in the DOM. With two glossaries sharing a name prefix the test drove the wrong one. Scope it to the `glossary-left-panel` sidebar, require an exact label match, and wait for the item rather than racing the panel's render. The label the sidebar renders is displayName, so the UI create helper now fills display-name and the call sites pass displayName. Verified every selectActiveGlossary call site on 1.13 already passes a displayName, so the stricter match has no other call site to break. (cherry picked from commit dd73bd9) Co-Authored-By: Claude <noreply@anthropic.com>
…ermission specs on 1.13 Backports the un-skips from #30451 (64c33fc) and #30699 (507caf7), which main and 2.0 have both carried since end of July. These four tests are the whole 1.13-only half of the nightly skip list — every other entry is skipped on all three branches. - EntityRenameConsolidation "Domain - rename then update description" and "Domain - multiple rename + update cycles": pure un-skip, no body change, same as main. - GlossaryNavigation "should show empty state when glossary has no terms": the assertion targeted the "No Glossary Term found" row, which only a non-matching status filter renders — an empty glossary renders a placeholder instead. main asserts its EmptyPlaceholder testid; 1.13 still routes through ErrorPlaceHolder and picks NO_DATA over CREATE (glossaryTermStatus is null for a glossary, not Approved), so assert no-data-placeholder here. Depends on the preceding selectActiveGlossary backport. - EntityPermissions "EditTier, EditOwners, EditCertification allowed but EditAll denied": the body contradicted its own title, asserting the edit buttons were visible. getPrioritizedEditPermission is byte-identical on 1.13 and main, so the corrected not.toBeVisible() assertions hold here too. Co-Authored-By: Claude <noreply@anthropic.com>
❌ 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 |
|
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! |
|
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! |
…nted These four arrived skipped in #25428 (`12d85f310f`), the same commit that created the file, and have never executed once in five months. They are not regressions and not flakes — they are a specification for behaviour the backend does not have. Each carried its own reason inline from birth: "Requires backend to return nested terms as flat results when filtered". That is accurate. The glossary term listing sends `directChildrenOf` and `entityStatus` on the same request (`glossaryAPI.ts` `getFirstLevelGlossaryTermsPaginated`), and the backend ANDs them (`GlossaryTermResource` `.addQueryParam("directChildrenOf", …).addQueryParam("entityStatus", …)`), so a nested term is eliminated by the hierarchy filter before its status is ever considered. No status value can surface a grandchild as a flat result. Confirmed by running them: on a real 1.13 stack all four fail at ~17.5s (the locator timeout — the term genuinely never renders), while every sibling test that asserts only root-level terms passes. `glossaryAPI.ts` and `GlossaryTermResource` are byte-identical across 1.13, main and 2.0, so the outcome is the same everywhere. Removed rather than left skipped: five bare `test.skip`s surface on every nightly triage list as if they were failures, and cost review attention each time someone asks why the glossary suite is red. If flat subtree filtering is built, these are better rewritten against the real API than resurrected from a five-month-old skip. No fixtures are orphaned — `basicChild`, `multiChild`, `deepTerms` and the rest are still used by the surviving tests. Prettier and eslint clean. Co-Authored-By: Claude <noreply@anthropic.com> (cherry picked from commit 4a7aa34) (cherry picked from commit f099c7c)
Verified passing at 2.8s on a real 1.13 stack built from this branch. Born skipped in #25428 claiming "re-filtering expanded state isn't fully implemented" — that shipped since and nobody revisited the marker. It is the one stale skip in this file, as opposed to the four genuinely backend-blocked ones removed in the preceding commit. It only surfaced on the nightly skip list as a separate entry because the file is `test.describe.configure({ mode: 'serial' })`: when `filter by child status` fails, Playwright reports every later test in the file as skipped without executing it. With those four gone the cascade cannot occur, so this test now actually runs. Co-Authored-By: Claude <noreply@anthropic.com>
…to backport/e2e-unskips-to-1.13
fb2837e landed `headerPermTest(` with five leading spaces instead of four, which fails `prettier --check` and would block the UI Checkstyle job. The assertion change in that commit is correct and identical to what this branch already carried — verified passing on a real 1.13 stack (3.0s). Only the indentation is corrected here. Co-Authored-By: Claude <noreply@anthropic.com>
|
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
changed the title
Closes the last gap between this branch and #30843, so 1.13 lands the same glossary state as main and 2.0 in a single merge — no post-merge cherry-pick of #30843 onto 1.13 is needed, and none should be attempted: the other two pieces of #30843 (the four removals and the change-filter re-enable) are already on this branch and would conflict. Measured passing on a real 1.13 stack (1.9m). Caveat worth carrying into review: **#29931 is not on 1.13**. That PR fixed the approve/reject condition scheme — duplicate edges and stuck resolves in the approval workflow — which is the documented cause behind this test's original skip ("BE approval-workflow reverts it"). `APPROVE_CONDITION` and `getExpectedResultForActiveTask` are both absent here, so 1.13 passes this test without the root fix. On main it passed 3/3 (1.0m, 58.6s, 58.3s) with the fix; on 1.13 it has one measured pass without it, and the failure mode is a load-dependent race that shows up under CI concurrency rather than on an idle machine. If it flakes in nightlies, backport #29931 rather than re-skipping. Note that is not a casual cherry-pick — it touches 21 files including bootstrap/sql/migrations/native/2.0.0/{mysql,postgres}/schemaChanges.sql, and a trial cherry-pick onto 1.13 conflicts in 14 of them. Co-Authored-By: Claude <noreply@anthropic.com>
|
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! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @@ -512,8 +464,7 @@ test.describe('Glossary Status Filter - Nested Terms', () => { | |||
| }); | |||
|
|
|||
| // Skip: Requires re-filtering expanded state which isn't fully implemented | |||
There was a problem hiding this comment.
💡 Quality: Stale "Skip:" comment left above re-enabled test
The comment // Skip: Requires re-filtering expanded state which isn't fully implemented still sits directly above the now-enabled change filter while expanded updates visible root terms test. It is now misleading — a reader would think the test is skipped/broken. Remove the stale comment along with the marker it described.
Delete the stale Skip comment on line 466.:
test('change filter while expanded updates visible root terms', async ({
page,
}) => {
Was this helpful? React with 👍 / 👎
| @@ -1904,8 +1904,7 @@ test.describe('Glossary tests', () => { | |||
| }); | |||
|
|
|||
| // Need to fix the workflow from BE end, as it constantly failing in the AUT's | |||
There was a problem hiding this comment.
💡 Quality: Stale skip-reason comment left above re-enabled approval test
The comment // Need to fix the workflow from BE end, as it constantly failing in the AUT's remains above the re-enabled Term should stay approved when changes made by reviewer test. Since the test.skip was removed, this comment now falsely implies the test is disabled/failing. Remove or update it; note the PR summary itself flags that the BE fix (#29931) is NOT on 1.13, so a brief note about the load-dependent flake risk would be more useful than the stale text.
Delete the stale backend-workflow skip comment on line 1906.:
test('Term should stay approved when changes made by reviewer', async ({
browser,
}) => {
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 2 findingsRe-enables five passing E2E specs and drops four unexecuted tests for the 1.13 skip-list cleanup. Consider removing the stale skip-reason comments left above the newly enabled tests. 💡 Quality: Stale "Skip:" comment left above re-enabled testThe comment Delete the stale Skip comment on line 466.💡 Quality: Stale skip-reason comment left above re-enabled approval test📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts:1906 The comment Delete the stale backend-workflow skip comment on line 1906.🤖 Prompt for agentsOptionsDisplay: 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 |
1 participant
Single PR covering the whole
1.13skip list. Every verdict is a measurement on a real 1.13 stack built from this branch (/api/v1/system/versionreportedrevision 53adda644b, matching HEAD — not stale-image results).Re-enabled — 5 specs, all measured green on 1.13
EntityRenameConsolidation › Domain - rename then update descriptionEntityRenameConsolidation › Domain - multiple rename + update cyclesGlossaryNavigation › should show empty state when glossary has no termsEntityPermissions › EditTier, EditOwners, EditCertification allowed but EditAll deniedGlossaryStatusFilter › change filter while expanded updates visible root termsThese are the entire 1.13-only half of the nightly skip list —
mainand2.0have run the first four since end of July (#30451, #30699).Two needed more than removing a marker:
mainasserts anEmptyPlaceholdertestid that does not exist on 1.13. 1.13 still routes throughErrorPlaceHolderand picksNO_DATAoverCREATE(glossaryTermStatusisnullfor a glossary, notApproved), so this assertsno-data-placeholder. Negative control: restoring main's assertion fails here withelement(s) not found — waiting for getByTestId('create-error-placeholder-Glossary Term'). A straight cherry-pick would have shipped a red test.page.getByRole('menuitem', { name }).first()— unscoped and non-exact, matching any menuitem merely containing the target and letting.first()pick whichever came first in the DOM. Now scoped toglossary-left-panelwith an exact label match. Verified everyselectActiveGlossarycall site on 1.13 already passes adisplayName, so the stricter match breaks nothing.Regression sweep for that shared util: all 251 tests in the 24 spec files that touch the glossary utils — 242 passed, 8 failed. All 8 A/B tested against the same stack: they pass in isolation with the backport (10/10), and one of them also fails on pristine
origin/1.13, so the failure mode predates this change. They are load-induced flakes at thetest.slow()boundary under 251 tests / 3 workers on one local stack; five are in files that only importselectActiveGlossaryTerm, which this PR does not touch.2. Glossary skip-list cleanup
Single PR covering the whole
1.13skip list. Every verdict is a measurement on a real 1.13 stack built from this branch (/api/v1/system/versionreportedrevision 53adda644b, matching HEAD — not stale-image results).Re-enabled — 5 specs, all measured green on 1.13
EntityRenameConsolidation › Domain - rename then update descriptionEntityRenameConsolidation › Domain - multiple rename + update cyclesGlossaryNavigation › should show empty state when glossary has no termsEntityPermissions › EditTier, EditOwners, EditCertification allowed but EditAll deniedGlossaryStatusFilter › change filter while expanded updates visible root termsThese are the entire 1.13-only half of the nightly skip list —
mainand2.0have run the first four since end of July (#30451, #30699).Two needed more than removing a marker:
mainasserts anEmptyPlaceholdertestid that does not exist on 1.13. 1.13 still routes throughErrorPlaceHolderand picksNO_DATAoverCREATE(glossaryTermStatusisnullfor a glossary, notApproved), so this assertsno-data-placeholder. Negative control: restoring main's assertion fails here withelement(s) not found — waiting for getByTestId(create-error-placeholder-Glossary Term). A straight cherry-pick would have shipped a red test.page.getByRole(menuitem, { name }).first()— unscoped and non-exact, matching any menuitem merely containing the target and letting.first()pick whichever came first in the DOM. Now scoped toglossary-left-panelwith an exact label match. Verified everyselectActiveGlossarycall site on 1.13 already passes adisplayName, so the stricter match breaks nothing.Regression sweep for that shared util: all 251 tests in the 24 spec files that touch the glossary utils — 242 passed, 8 failed. All 8 A/B tested against the same stack: they pass in isolation with the backport (10/10), and one of them also fails on pristine
origin/1.13, so the failure mode predates this change. They are load-induced flakes at thetest.slow()boundary under 251 tests / 3 workers on one local stack; five are in files that only importselectActiveGlossaryTerm, which this PR does not touch.2. Glossary skip-list cleanup
The rule applied
A test is deleted only if it was skipped in the very commit that introduced it — it has never run and never protected anything. Anything that was once green stays, even if it is red today, because that redness records a regression rather than a test that never worked.
Every verdict below is a measurement on a real local stack, not inspection.
Removed — 4 tests, skipped at inception, never executed
GlossaryStatusFilterNestedTerms:filter by child status,filter by middle level status,filter by leaf level status,deeply nested term (5 levels).All four arrived skipped in #25428 (
12d85f310f, 2026-03-05) — the commit that created the file — each carrying "Requires backend to return nested terms as flat results when filtered". Five months, zero executions. The six later commits to this file never touched the skips.They cannot pass, by construction.
getFirstLevelGlossaryTermsPaginatedsends the hierarchy and status filters on one request:and
GlossaryTermResourceANDs them:directChildrenOfmeans "first level/immediate children", so a nested term is eliminated before its status is considered. Measured: all four fail at ~17.5–17.9s (locator timeout — the term never renders) on both a 1.13 stack and a main stack, while every sibling asserting only root-level terms passes.glossaryAPI.tsandGlossaryTermResource.javaare byte-identical across 1.13, main and 2.0.Re-enabled — 2 tests that pass today
Glossary › Term should stay approved when changes made by reviewerentityStatusGlossaryStatusFilter › change filter while expanded updates visible root terms#29931 fixed the approval machinery in July (approve/reject condition scheme, duplicate edges, stuck resolves) — exactly the cause behind the first test. Re-run three times to guard against its known load-dependent race; green every time.
The second was born skipped like the four removed ones, but for a different reason, and unlike them it is green. It is re-enabled rather than deleted precisely because deleting a passing test loses coverage. It only appeared on the nightly skip list at all because the file is
test.describe.configure({ mode: 'serial' })— whenfilter by child statusfails, Playwright reports every later test in the file as skipped without running it. With those four gone the cascade cannot occur.Deliberately left skipped
The five
DataMarketplacenavigation tests. They were born active and green in #26255 and ran for ten days before #27377 removed the/data-marketplace/*sub-routes and skipped them with "re-enable when the standalone marketplace shell is reintroduced". They fail at 1.0m today on both main and 1.13, but under the rule above they are not deletion candidates — they record a withdrawn capability. The marketplace page itself is alive (11 components on every branch) and 8 sibling tests covering rendering, data-product/domain creation, search empty state and the admin-vs-consumer permission split all still pass, so the spec files stay.CustomizeWidgets › KPI WidgetandActivityFeed › Mention notificationalso pass on main (30.3s / 9.4s) but are out of scope here and remain skipped.Verification
basicChild,multiChild,deepTerms,multiChildrenParentall still used by surviving tests.eslint-disable playwright/no-skipped-testdirectives removed with their skips, otherwise they become unused-directive errors.prettier --checkclean;eslint0 errors.🤖 Generated with Claude Code
1.13-specific notes
EntityRenameConsolidationis tagged@basic, which thechromiumproject grep-inverts — re-runs need--project=Basicor Playwright reports "No tests found" while testing nothing.Glossary › Term should stay approvedis re-enabled here on the strength of a local pass, but note fix(workflows,tasks): approve/reject condition scheme, Task V2 migration & resolve, v1122 workflow-handler init, TagUpdate version bump #29931 — the approval-workflow fix — is NOT on 1.13 (APPROVE_CONDITIONandgetExpectedResultForActiveTaskare both absent). It passes locally without that fix, but its documented failure mode is a load-dependent race. If it flakes in nightlies, backport fix(workflows,tasks): approve/reject condition scheme, Task V2 migration & resolve, v1122 workflow-handler init, TagUpdate version bump #29931 rather than re-skipping.GlossaryStatusFilterNestedTermsstill uses pre-testid selectors inapplyStatusFilterwhere main has moved toglossary-status-option-*. Pre-existing, in a shared helper, untouched here.