test(ui): make glossary listing tests resilient to active-glossary drift#28988
test(ui): make glossary listing tests resilient to active-glossary drift#28988siddhant1 wants to merge 4 commits into
Conversation
The glossary listing page falls back to the first glossary (glossaries[0]) when the URL's glossary isn't present in the freshly loaded sidebar page. Under parallel test load the sidebar list churns, so the in-memory active glossary can drift to another worker's glossary even though the URL stays correct (the sidebar highlight is URL-driven, so a re-click is a no-op), leaving the terms table showing the wrong glossary. This flaked tests such as "Approve and reject glossary term from Glossary Listing" at the term-row visibility assertion. Recover by reloading -- which re-derives the active glossary from the URL FQN -- in selectActiveGlossary and validateGlossaryTerm. Co-Authored-By: Claude Opus 4.8 (1M context) <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! |
siddhant1
added
UI
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Jest test CoverageUI tests summary
|
🔴 Playwright Results — 1 failure(s), 16 flaky✅ 4283 passed · ❌ 1 failed · 🟡 16 flaky · ⏭️ 88 skipped
Genuine Failures (failed on all attempts)❌
|
Code Review ✅ Approved 1 resolved / 1 findingsAdds a ✅ 1 resolved✅ Quality: Recovery path can extend assertion runtime up to ~40s
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const assertWithReloadRecovery = async ( | ||
| page: Page, | ||
| verify: () => Promise<void> | ||
| ) => { | ||
| try { | ||
| await verify(); | ||
| } catch { | ||
| await expect(async () => { | ||
| await page.reload(); | ||
| await waitForAllLoadersToDisappear(page); | ||
| await verify(); | ||
| }).toPass({ intervals: [2_000, 5_000, 10_000], timeout: 30_000 }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
toPass is called with { intervals: [2_000, 5_000, 10_000], timeout: 30_000 }. Each iteration runs page.reload() + waitForAllLoadersToDisappear + verify() (which itself has a 10_000 ms internal timeout on failure). In a slow CI environment where reload + loader settling takes ~8–12 s, the first retry attempt alone consumes ~20–22 s, leaving only ~8–10 s for a second attempt before the 30 s wall hits. The intervals effectively become dead time that cannot be recovered, so the retry count is far lower than the three-slot array implies.
Raising timeout to 60_000 or reducing verify's internal timeout to e.g. 5_000 would make the budget more reliable without changing the intent.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
This PR has had no activity for 30 days and will be closed in 7 days if no further activity occurs. |
Problem
Several
Glossary.spec.tslisting-page tests (e.g. "Approve and reject glossary term from Glossary Listing") are flaky. The glossary listing page intentionally falls back to the first glossary (glossaries[0]) when the URL's glossary isn't present in the freshly loaded sidebar page (GlossaryPage.component.tsx:setActiveGlossary(find(fqn) || glossaries[0])).Under parallel test load the sidebar glossary list churns (other workers create/delete glossaries; infinite-scroll pagination re-sets the list), so the in-memory active glossary drifts to another worker's glossary even though the browser URL stays correct. The terms table then renders the wrong glossary's children, and the just-created term's row (
[data-row-key="<fqn>"]) never appears — the assertion times out.Trace evidence from the failing run: after the 2nd term POST (
201) into glossaryZany, the table refetcheddirectChildrenOf="…Quiet…"(a different glossary); the URL never leftZany. Pure in-memory state drift.Note: the
find(fqn) || glossaries[0]fallback is intended product behavior, so the fix is test-side.Change (test-only)
A shared
assertWithReloadRecoveryhelper that, on an assertion miss, reloads (which re-derives the active glossary from the URL FQN) and retries undertoPass. Applied at the two drift-exposed points:selectActiveGlossary— the universal re-pin chokepoint. The sidebar highlight is URL-driven, so a re-click is a no-op when drifted; it now verifies the header shows the requested glossary and reloads to re-pin if it drifted.validateGlossaryTerm— the actual failure point. The term-creation loop drifts between terms without re-selecting, so the term-row check now self-heals via reload.No app/product code touched.
Testing
yarn playwright:run --grep "Approve and reject glossary term from Glossary Listing"yarn lint(playwright) — only pre-existing warningstscerrors in the changed file🤖 Generated with Claude Code
Greptile Summary
This PR adds a
assertWithReloadRecoverytest utility toglossary.tsthat handles in-memory active-glossary drift caused by parallel test workers mutating the sidebar glossary list, without touching any product code.assertWithReloadRecoverywraps a Playwright assertion in a try/catch; on failure it enters atoPassloop that reloads the page (re-deriving the active glossary from the URL) and retries, with intervals of 2 s / 5 s / 10 s and a 30 s total budget.selectActiveGlossary(verifies the entity header reflects the requested glossary after sidebar click) andvalidateGlossaryTerm(guards the term-rowtoBeVisiblecheck that was the observed failure point).Confidence Score: 4/5
Test-only change with no product code touched; the reload recovery logic is sound but has two minor gaps worth addressing before relying on it for long-term flakiness suppression.
The new helper correctly exploits the fact that the URL stays accurate during drift and a reload re-derives active glossary state from it. The two concerns — unprotected assertions that follow the guarded toBeVisible check in validateGlossaryTerm, and a toPass budget that may only allow one reload on slower CI agents — are non-blocking but could mean the fix is only partially effective under adverse conditions.
openmetadata-ui/src/main/resources/ui/playwright/utils/glossary.ts — specifically the assertions following assertWithReloadRecovery in validateGlossaryTerm (lines 748–758) and the toPass timeout/interval configuration in the helper itself.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["assertWithReloadRecovery(page, verify)"] --> B["try: await verify()"] B -->|passes| C["Done"] B -->|throws| D["enter toPass loop\ntimeout: 30s\nintervals: 2s/5s/10s"] D --> E["page.reload()"] E --> F["waitForAllLoadersToDisappear(page)"] F --> G["await verify() with timeout: 10s"] G -->|passes| H["toPass succeeds"] G -->|throws| I{Wall clock < 30s?} I -->|yes, wait interval| E I -->|no| J["toPass throws last error"] subgraph selectActiveGlossary SA1["click sidebar menu item"] --> SA2["waitForAllLoadersToDisappear"] SA2 --> SA3["assertWithReloadRecovery:\nentity-header-display-name contains label"] end subgraph validateGlossaryTerm VT1["wait for table to load"] --> VT2["assertWithReloadRecovery:\ndata-row-key=fqn is visible"] VT2 --> VT3["toContainText(term.name) - not reload-protected"] VT3 --> VT4["statusSelector checks - not reload-protected"] end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["assertWithReloadRecovery(page, verify)"] --> B["try: await verify()"] B -->|passes| C["Done"] B -->|throws| D["enter toPass loop\ntimeout: 30s\nintervals: 2s/5s/10s"] D --> E["page.reload()"] E --> F["waitForAllLoadersToDisappear(page)"] F --> G["await verify() with timeout: 10s"] G -->|passes| H["toPass succeeds"] G -->|throws| I{Wall clock < 30s?} I -->|yes, wait interval| E I -->|no| J["toPass throws last error"] subgraph selectActiveGlossary SA1["click sidebar menu item"] --> SA2["waitForAllLoadersToDisappear"] SA2 --> SA3["assertWithReloadRecovery:\nentity-header-display-name contains label"] end subgraph validateGlossaryTerm VT1["wait for table to load"] --> VT2["assertWithReloadRecovery:\ndata-row-key=fqn is visible"] VT2 --> VT3["toContainText(term.name) - not reload-protected"] VT3 --> VT4["statusSelector checks - not reload-protected"] endComments Outside Diff (1)
openmetadata-ui/src/main/resources/ui/playwright/utils/glossary.ts, line 745-758 (link)assertWithReloadRecoveryguards thetoBeVisiblecheck and reloads when drifted, but the four subsequent assertions (toContainText,statusSelectorvisible, status text) run outside the helper against the post-reload page. If the component re-renders mid-assertion (e.g., an in-flight terms API response resolves after the reload and repopulates the wrong glossary's children again), those checks can still fail. The drift window is small but not zero — a second parallel-worker mutation arriving just after the reload could trigger it again before those assertions complete.Consider folding all five assertions into a single
assertWithReloadRecoverycall, or re-selecting the active glossary before proceeding past the reload.Reviews (1): Last reviewed commit: "Merge branch 'main' into pw-fix-glossary..." | Re-trigger Greptile