test(teams): stop the teams landing from eating the test budget - #31932
Conversation
TeamsDragAndDrop's beforeEach spent the whole 60s test timeout before any test body ran. settingClick already ends in waitForAllLoadersToDisappear (30s), and the hook then called it a second time. The Teams table's antd spinner is the shared `data-testid="loader"`, and it stays up until both the child-teams fetch and the per-team asset-count aggregation settle — on a long-lived deployment that is tens of seconds, so the two waits together consumed the budget and the hook timed out on the second one. Give the suite test.slow(true) so the landing has headroom, and hard-delete the four teams the suite creates. Without that cleanup every nightly run left four more teams under Organization, growing the aggregation the landing waits on and making the next run slower. Fold the duplicated navigation into visitTeamsPage and reuse it from TeamsHierarchy, which had the same hook with looser glob waits. Co-Authored-By: Claude Opus 5 (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! |
harsh-vador
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
✅ Playwright Results — workflow succeededValidated commit ✅ 564 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 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) 46m 39s ⏱️ Max setup 4m 33s · max shard execution 19m 20s · max shard-job elapsed before upload 22m 49s · reporting 4s 🌐 214.58 requests/attempt · 2.79 app boots/UI scenario · 33.45% 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 |
hardDeleteTeamByName issued the DELETE and ignored the result, so a delete rejected on permissions or failing with a 500 left the team behind with no signal in the run — quietly reintroducing the accumulation this cleanup exists to prevent. Assert the response instead, with the status and body in the message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lookup guard keyed off `ok()`, which is false for an auth error or a 500 just as it is for a 404. A broken lookup therefore took the same path as a team that was already gone: return without deleting, and report success. Tolerate 404 alone — the spec may have deleted the team itself, and a recursive delete of its parent takes its children — and assert every other lookup failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The assertion sat inside hardDeleteTeamByName, so the first team that failed to delete threw out of the caller's loop and the remaining names were never attempted — leaving those teams on the deployment, which is the accumulation the cleanup exists to prevent. Move the assertion up into hardDeleteTeamsByName: the per-team helper now reports a failure instead of throwing (network errors included), every name is attempted, and one assertion at the end names every team that survived along with the status and body that explain why. Wrap the call in try/finally in the spec so the API context is disposed even when cleanup fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(teams): backport addTeamHierarchy hardening to 1.13 1.13 never received the addTeamHierarchy waits that main and 2.0 carry, so the helper returns while the teams table is still refetching: it opens the modal on a plain click a toast can swallow, resolves on the first `/api/v1/teams` response regardless of method or status, and never waits for the modal to close or the row to render. Backports the team.ts changes from #25894, #30334 and #31734. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(teams): stop the teams landing from eating the test budget TeamsDragAndDrop's beforeEach spent the whole 60s test timeout before any test body ran. settingClick already ends in waitForAllLoadersToDisappear (30s), and the hook then called it a second time. The Teams table's antd spinner is the shared `data-testid="loader"`, and it stays up until both the child-teams fetch and the per-team asset-count aggregation settle — on a long-lived deployment that is tens of seconds, so the two waits together consumed the budget and the hook timed out on the second one. Give the suite test.slow(true) so the landing has headroom, and hard-delete the four teams the suite creates. Without that cleanup every nightly run left four more teams under Organization, growing the aggregation the landing waits on and making the next run slower. Fold the duplicated navigation into visitTeamsPage and reuse it from TeamsHierarchy, which had the same hook with looser glob waits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(teams): surface a failed cleanup delete hardDeleteTeamByName issued the DELETE and ignored the result, so a delete rejected on permissions or failing with a 500 left the team behind with no signal in the run — quietly reintroducing the accumulation this cleanup exists to prevent. Assert the response instead, with the status and body in the message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(teams): only tolerate a 404 on the cleanup lookup The lookup guard keyed off `ok()`, which is false for an auth error or a 500 just as it is for a 404. A broken lookup therefore took the same path as a team that was already gone: return without deleting, and report success. Tolerate 404 alone — the spec may have deleted the team itself, and a recursive delete of its parent takes its children — and assert every other lookup failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(teams): attempt every cleanup delete before asserting The assertion sat inside hardDeleteTeamByName, so the first team that failed to delete threw out of the caller's loop and the remaining names were never attempted — leaving those teams on the deployment, which is the accumulation the cleanup exists to prevent. Move the assertion up into hardDeleteTeamsByName: the per-team helper now reports a failure instead of throwing (network errors included), every name is attempted, and one assertion at the end names every team that survived along with the status and body that explain why. Wrap the call in try/finally in the spec so the API context is disposed even when cleanup fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback: no test.slow at describe level. It also had to move for correctness — hook time counts against the test timeout, so marking a test slow from its body is too late when it is the beforeEach that overruns. Calling test.slow() as the first statement of the hook applies it to the current test before the hook does any waiting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| test.beforeEach(async ({ page }) => { | ||
| await redirectToHomePage(page); | ||
| await visitTeamsPage(page); | ||
| }); |
There was a problem hiding this comment.
⚠️ Bug: Per-test test.slow() leaves slow beforeEach on default 60s budget
This commit removes test.slow() from beforeEach and instead calls test.slow(true) inside individual test bodies. Two problems reintroduce the very flake this PR targets: (1) the three 'Should fail...' tests (lines 125, 135, 149) get no test.slow() at all, so they run the slow visitTeamsPage hook under the default 60s timeout; (2) even for tests that do call test.slow(true), the call runs only after beforeEach completes — if the hierarchy-table aggregation makes visitTeamsPage exceed 60s (the exact failure in the RCA), the hook times out before the body runs and the tripling never applies. The deleted comment made precisely this point. Restore test.slow() at the start of beforeEach so the tripled budget covers hook time for every test, and drop the redundant per-test calls.
Move test.slow() back into beforeEach so it covers hook time for all tests; remove the per-test test.slow(true) calls.:
test.beforeEach(async ({ page }) => {
// Hook time counts against the test timeout, and visitTeamsPage waits on
// the hierarchy table's asset-count aggregation, which grows with the
// catalog. Mark slow here so the tripled budget covers the hook for every
// test (a per-test-body test.slow() runs too late if the hook overruns).
test.slow();
await redirectToHomePage(page);
await visitTeamsPage(page);
});
Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
2 participants
Problem
TeamsDragAndDrop.spec.tsfails on the nightly upgrade runs — AUT EKS/RDS MySQL 1.11.13 ➡ 1.13. The first failure is inbeforeEach, before any test body runs:RCA
The
data-testid="loader"the hook waits on is the teams table's own spinner:TeamHierarchy.tsx→loading={isTableLoading || isTeamBasicDataLoading || isSearchLoading}common/Table/Table.tsx→loading={{ indicator: <Loader /> }}common/Loader/Loader.tsx→data-testid="loader"isTeamBasicDataLoadingcovers the per-team asset-count aggregation, which scales with the catalog. So the landing is genuinely slow on a loaded deployment — and the hook spent its budget on it twice:settingClickalready ends inwaitForAllLoadersToDisappear(30s), and the hook called it a second time. Two 30s waits against a 60s test timeout leaves nothing, the hook times out, and serial mode skips the rest of the file.Why it degrades over time: this suite creates four
team-ct-test-*teams and never deletes them (TeamsHierarchy.spec.tsdeletes its parent team; this one doesn't). The EKS/RDS nightlies reuse a persistent database, so every run leaves four more teams under Organization, growing the aggregation the landing waits on. Matches the result spread —AUT kind/postgresql(fresh cluster) passed, both EKS/RDS runs failed.Fix
utils/team.ts— newvisitTeamsPage(): waits the two calls that gate the first paint (with.ok()rather than a bare status compare), assertsteam-hierarchy-table, then waits out loaders. NewhardDeleteTeamByName()for teams built through the UI, which have noTeamClasshandle.TeamsDragAndDrop.spec.ts—test.slow(true)at describe level so the landing has headroom (replaces three per-testtest.slow()calls); hook reduced toredirectToHomePage+visitTeamsPage;afterAllhard-deletes the four teams recursively so the deployment stops accumulating them.TeamsHierarchy.spec.ts— same hook folded ontovisitTeamsPage(it had the same shape with looser glob waits); drops a redundantsettingClickin Delete Parent Team, sincebeforeEachalready lands there.Backport
1.13 additionally lacks the
addTeamHierarchyhardening that main and 2.0 carry (#25894'steam.tshunk, #30334, #31734), which is what fails the retries atTeamsDragAndDrop.spec.ts:119. That is handled in the companion 1.13 PR. 2.0 needs only a cherry-pick of this one.Verification
tsc --noEmit -p playwright/tsconfig.json,eslint, andprettier --checkare clean on the touched files. The spec itself needs a live deployment and was not run locally.🤖 Generated with Claude Code
Greptile Summary
The PR makes the Teams Playwright suites wait for the hierarchy page to settle and adds reliable teardown for teams created by the drag-and-drop suite.
visitTeamsPage.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the cleanup now reports unsuccessful lookups and deletes while attempting every team before failing teardown.
Important Files Changed
Reviews (6): Last reviewed commit: "remove unwanted test.slow" | Re-trigger Greptile