test(e2e): drop 5 dead marketplace tests and two stale skip comments (cherry-pick #30843 to 2.0) - #30912
Merged
Conversation
…removed routes), re-enable 2 that pass (#30843) * test(glossary): drop four status-filter tests that were never implemented 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> * test(e2e): re-enable four specs that pass on main today Each of these was written active and green, then skipped later when something broke. All four pass now on a real main stack built from this branch point (`revision 64df7a3`), so the markers are stale: | Test | Skipped since | Measured on main | |---|---|---| | `ActivityFeed › Mention notification shows correct user details` | #25894 (`test.fixme`) | 9.4s | | `CustomizeWidgets › KPI Widget` | `test.fixme` | 30.3s | | `Glossary › Term should stay approved when changes made by reviewer` | BE approval workflow | 1.0m | | `GlossaryStatusFilter › change filter while expanded updates visible root terms` | #25428 | 2.8s | `Term should stay approved` is the interesting one: its documented cause was the async approval workflow reverting a PATCHed entityStatus, and #29931 fixed exactly that machinery (approve/reject condition scheme, duplicate edges, stuck resolves) in July. main carries #29931; the test now passes. `change filter while expanded` never ran — it was born skipped in #25428 claiming "re-filtering expanded state isn't fully implemented". That shipped at some point and nobody revisited the marker. It is kept rather than deleted precisely because it passes: it is the one test in that file whose skip was stale rather than blocked on the missing backend support. Stale `eslint-disable playwright/no-skipped-test` directives removed with their skips, otherwise they become unused-directive errors. Prettier clean; eslint 0 errors. Co-Authored-By: Claude <noreply@anthropic.com> * Revert re-enabling KPI Widget and the ActivityFeed mention test Both pass on a main stack (30.3s and 9.4s), but they are out of scope for this PR and stay skipped. The two re-enables that remain — Term should stay approved and change filter while expanded — are unaffected. Co-Authored-By: Claude <noreply@anthropic.com> * test(marketplace): drop five navigation tests whose routes were removed These five differ from the four glossary tests removed earlier in this PR: they were born **active and green** in #26255 (`e90ab792f6`, 2026-04-06) and ran for ten days. #27377 (`3129a14b10`) then removed the `/data-marketplace/*` sub-routes they navigate to and skipped them in the same commit, noting "re-enable when the standalone marketplace shell is reintroduced". That shell has not been reintroduced in four months, so the skips are now a permanent fixture rather than a pending re-enable. Confirmed dead by running them un-skipped: all five fail at 1.0m on both a real 1.13 stack and a real main stack — the navigation target does not exist. Scope is the five dead navigation tests, **not** the three spec files. The marketplace page is alive — 11 marketplace components ship on every branch — and the files retain 8 passing tests covering rendering, data-product and domain creation via the drawer, the search empty state, the search popover, and the admin-vs-consumer permission split. Deleting the files would have taken that with them. If the standalone marketplace shell returns, these are better rewritten against the new routes than restored from a four-month-old skip. Prettier clean; eslint 0 errors. Co-Authored-By: Claude <noreply@anthropic.com> * Fix UI checkstyle: drop dead import and two stale skip comments `ui-checkstyle` failed on DataMarketplacePermissions.spec.ts. Removing the "Data consumer can search and view results" test left `searchMarketplace` imported and unused (0 references), which `organize-imports-cli` strips. CI's Playwright step is organize-imports -> eslint --fix -> prettier --write; the earlier local pass ran only eslint and prettier, and neither flags a dead import, which is why this got through locally but not in CI. Also removes two skip-reason comments left above tests re-enabled earlier in this PR, flagged in review — the source contradicted itself: - Glossary.spec.ts: "Need to fix the workflow from BE end, as it constantly failing in the AUT's" above the now-active `Term should stay approved` - GlossaryStatusFilterNestedTerms.spec.ts: "Skip: Requires re-filtering expanded state which isn't fully implemented" above the now-active `change filter while expanded` ActivityFeed.spec.ts's comment is left alone — that un-skip was reverted, so its note still matches a `test.fixme`. Verified by re-running the full CI sequence: idempotent, 0 eslint errors. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit aeafd62)
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! |
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-picks
aeafd62397(#30843, merged tomain) onto2.0.What was already on 2.0
2.0already carried the glossary half of #30843 —d4ab24372bis an ancestor oforigin/2.0, and that branch includes both "drop four status-filter tests" and "re-enable four specs". So the four never-run status-filter tests were already gone and the two stale skips already lifted.What was missing, and is what this PR brings over:
2.0searchMarketplaceimport inDataMarketplacePermissions.spec.ts, orphaned by removing "Data consumer can search and view results"2.0, so the source contradicted itself:Glossary.spec.ts— "Need to fix the workflow from BE end, as it constantly failing in the AUT's" above the activeTerm should stay approved when changes made by reviewerGlossaryStatusFilterNestedTerms.spec.ts— "Skip: Requires re-filtering expanded state which isn't fully implemented" above the activechange filter while expanded updates visible root termsTests removed
Five, all
test.skip, all navigating to/data-marketplace/*sub-routes that #27377 (3129a14b10) deleted while skipping them in the same commit, noting "re-enable when the standalone marketplace shell is reintroduced". That shell has not returned in four months.DataMarketplace.spec.tsSearch returns results and clicking navigates to entityDataMarketplace.spec.tsWidget card click navigates to entity detail pageDataMarketplace.spec.tsView All links navigate correctlyDataMarketplaceAnnouncements.spec.tsClicking announcement navigates to entity pageDataMarketplacePermissions.spec.tsData consumer can search and view resultsScope is the five dead tests, not the three spec files. The marketplace page is alive; the files keep their passing coverage of rendering, data-product and domain creation via the drawer, the search empty state, the search popover, and the admin-vs-consumer permission split.
Conflicts and how they were resolved
Two, both the stale-comment removals listed above. Resolved by taking the incoming side (deleting the comment) — the tests they annotate are not skipped on
2.0, so the comments were already wrong.One intentional divergence from main
Glossary.spec.tsis the only file that does not end up byte-identical tomain, and it differs by exactly one line — a genuine pre-existing2.0difference in the glossary assets-tab empty state, preserved rather than clobbered:The other four specs are byte-identical to
mainafter this cherry-pick.Verification
playwright/test.skip/test.fixmeleft in any of the five touched specssearchMarketplacedropped from the permissions spec's imports; still imported and used inDataMarketplace.spec.tsorganize-imports-cli→eslint --fix→prettier --write) run on all five files: 0 errors, and idempotent — no diff against the committed state. The three remaining eslint warnings are pre-existingbrowser.newPage()advisories on lines this PR does not touch.🤖 Generated with Claude Code