playwright(fix): fix unclosed browser contexts and page leaks in E2E tests#29694
Open
chirag-madlani wants to merge 10 commits into
Open
playwright(fix): fix unclosed browser contexts and page leaks in E2E tests#29694chirag-madlani wants to merge 10 commits into
chirag-madlani wants to merge 10 commits into
Conversation
…tests
Scan all Playwright specs for resource leaks and fix them:
- PersonaFlow.spec.ts: wrap userContext (browser.newContext) in try/finally
- LineagePipelineAnnotator.spec.ts: wrap beforeAll/afterAll setup in try/finally
so apiContext.dispose() + page.close() run even when an API call throws
- DataAssetLineage.spec.ts: destructure afterAction from getApiContext() and
call it in a finally block (previously discarded, leaking APIRequestContext)
- Glossary.spec.ts: wrap deny-permission tests in try/finally for
consumerAfterAction / cleanup / afterAction
- GlossaryVersionPage.spec.ts: wrap glossary.delete + afterAction in finally
- GlossaryRelationsGraph.spec.ts, GlossaryRelationsGraphPerf.spec.ts,
GlossaryTermRelationsGraphNested.spec.ts: replace browser.newPage() +
adminUser.login() per test with test.use({ storageState }) + page fixture —
eliminates the auth-per-test overhead and removes the page-leak-on-failure risk
Also adds an ESLint warn rule on bare browser.newPage() in e2e spec files to
surface the anti-pattern at code-review time without blocking multi-user tests
that legitimately need a second page as a different user.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 |
github-actions
Bot
added
safe to test
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Playwright E2E specs against resource leaks (pages, browser contexts, and API request contexts) by ensuring cleanup runs even when assertions/setup fail, and by migrating single-user admin tests to the page fixture + storageState pattern to avoid manual browser.newPage() + login.
Changes:
- Added
try/finallycleanup patterns across several specs to ensureafterAction(),context.close(),page.close(), andapiContext.dispose()run on failures. - Migrated Glossary Relations Graph specs from per-test
browser.newPage()+ manual admin login totest.use({ storageState })+pagefixture. - Added an ESLint warning rule for the
browser.newPage()anti-pattern in E2E spec files.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/playwright/e2e/VersionPages/GlossaryVersionPage.spec.ts | Adds cleanup finally for glossary version tests to avoid API context leaks. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Lineage/DataAssetLineage.spec.ts | Ensures getApiContext()’s afterAction() is always executed via try/finally. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Glossary.spec.ts | Wrapes deny-permission flows in try/finally to ensure cleanup runs on assertion failures. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/PersonaFlow.spec.ts | Ensures browser.newContext() is closed via try/finally to prevent context leaks. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/LineagePipelineAnnotator.spec.ts | Wraps beforeAll/afterAll setup & teardown with cleanup finally blocks to prevent page/api leaks. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryTermRelationsGraphNested.spec.ts | Switches to storageState + page fixture to avoid per-test pages and login overhead. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryRelationsGraphPerf.spec.ts | Switches to storageState + page fixture to eliminate manual page lifecycle handling and reduce auth overhead. |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryRelationsGraph.spec.ts | Switches to storageState + page fixture to eliminate per-test browser.newPage() usage. |
| openmetadata-ui/src/main/resources/ui/eslint.config.mjs | Adds a warn-level no-restricted-syntax rule to surface bare browser.newPage() in E2E specs. |
Comment on lines
336
to
+340
| await user.login(userPage); | ||
|
|
||
| test.slow(true); | ||
|
|
||
| await test.step('Admin creates a persona and sets the default persona', async () => { | ||
| await navigateToPersonaSettings(adminPage); | ||
| await adminPage.getByTestId('add-persona-button').click(); | ||
|
|
||
| await validateFormNameFieldInput({ | ||
| page: adminPage, | ||
| value: PERSONA_DETAILS.name, | ||
| fieldName: 'Name', | ||
| fieldSelector: '[data-testid="name"]', | ||
| errorDivSelector: '#name_help', | ||
| }); | ||
|
|
||
| await adminPage | ||
| .getByTestId('displayName') | ||
| .fill(PERSONA_DETAILS.displayName); | ||
|
|
||
| await adminPage.locator(descriptionBox).fill(PERSONA_DETAILS.description); | ||
|
|
||
| const userListResponse = adminPage.waitForResponse( | ||
| '/api/v1/users?limit=*&isBot=false*' | ||
| ); | ||
| await adminPage.getByTestId('add-users').click(); | ||
| await userListResponse; | ||
|
|
||
| await waitForAllLoadersToDisappear(adminPage); | ||
|
|
||
| const searchUser = adminPage.waitForResponse( | ||
| `/api/v1/search/query?q=*${encodeURIComponent( | ||
| user.responseData.displayName | ||
| )}*` | ||
| ); | ||
| await adminPage | ||
| .getByTestId('searchbar') | ||
| .fill(user.responseData.displayName); | ||
| await searchUser; | ||
|
|
||
| await adminPage | ||
| .getByRole('listitem', { name: user.responseData.displayName }) | ||
| .click(); | ||
| await adminPage.getByTestId('selectable-list-update-btn').click(); | ||
|
|
||
| await adminPage.getByRole('button', { name: 'Create' }).click(); | ||
|
|
||
| await navigateToPersonaSettings(adminPage); | ||
|
|
||
| await waitForAllLoadersToDisappear(adminPage, 'skeleton-card-loader'); | ||
|
|
||
| const personaResponse = adminPage.waitForResponse( | ||
| `/api/v1/personas/name/${encodeURIComponent( | ||
| try { |
Comment on lines
51
to
+55
| const { afterAction, apiContext } = await getApiContext(page); | ||
| await glossary.create(apiContext); | ||
| await glossary.patch(apiContext, GLOSSARY_PATCH_PAYLOAD); | ||
|
|
||
| await test.step('Version changes', async () => { | ||
| await glossary.visitPage(page); | ||
|
|
||
| await page.click('[data-testid="version-button"]'); | ||
|
|
||
| await expect( | ||
| page | ||
| .getByTestId('asset-description-container') | ||
| .getByTestId('markdown-parser') | ||
| .locator('span') | ||
| .filter({ hasText: 'Description' }) | ||
| ).toBeVisible(); | ||
|
|
||
| await expect( | ||
| page.locator( | ||
| '.diff-added [data-testid="tag-PersonalData.SpecialCategory"]' | ||
| ) | ||
| ).toBeVisible(); | ||
|
|
||
| await expect( | ||
| page.locator('.diff-added [data-testid="tag-PII.Sensitive"]') | ||
| ).toBeVisible(); | ||
| }); | ||
|
|
||
| await test.step('Should display the owner & reviewer changes', async () => { | ||
| await glossary.visitPage(page); | ||
|
|
||
| await expect(page.getByTestId('version-button')).toHaveText(/0.2/); | ||
|
|
||
| await addMultiOwner({ | ||
| page, | ||
| ownerNames: [user.getUserDisplayName()], | ||
| activatorBtnDataTestId: 'add-owner', | ||
| resultTestId: 'glossary-right-panel-owner-link', | ||
| endpoint: EntityTypeEndpoint.Glossary, | ||
| isSelectableInsideForm: true, | ||
| type: 'Users', | ||
| try { |
Comment on lines
+125
to
+128
| await page.reload(); | ||
| await page.click('[data-testid="version-button"]'); | ||
| await versionPageResponse; | ||
|
|
Comment on lines
+135
to
+138
| } finally { | ||
| await glossary.delete(apiContext); | ||
| await afterAction(); | ||
| } |
Comment on lines
52
to
+56
| await redirectToHomePage(page); | ||
| const token = await getToken(page); | ||
| const apiContext = await getAuthContext(token); | ||
|
|
||
| table = new TableClass(); | ||
| await table.create(apiContext); | ||
| dbServiceFqn = table.serviceResponseData.fullyQualifiedName ?? ''; | ||
|
|
||
| const msName = `pw-kafka-${uuid()}`; | ||
| const msResp = await apiContext | ||
| .post('/api/v1/services/messagingServices', { | ||
| data: { | ||
| name: msName, | ||
| serviceType: 'Kafka', | ||
| connection: { | ||
| config: { type: 'Kafka', bootstrapServers: 'localhost:9092' }, | ||
| try { |
Comment on lines
135
to
+139
| await redirectToHomePage(page); | ||
| const token = await getToken(page); | ||
| const apiContext = await getAuthContext(token); | ||
|
|
||
| await table.delete(apiContext); | ||
| await apiContext.delete( | ||
| `/api/v1/services/messagingServices/name/${encodeURIComponent( | ||
| messagingServiceFqn | ||
| )}?recursive=true&hardDelete=true` | ||
| ); | ||
| await apiContext.delete( | ||
| `/api/v1/services/pipelineServices/name/${encodeURIComponent( | ||
| pipelineServiceFqn | ||
| )}?recursive=true&hardDelete=true` | ||
| ); | ||
|
|
||
| await apiContext.dispose(); | ||
| await page.close(); | ||
| try { |
…nally cleanup chains - Replace second await on resolved versionPageResponse promise with new waitForResponse registrations in Glossary and GlossaryTerm tests so the test actually waits for the second network request. - Nest glossary.delete() inside try/finally so afterAction() always runs even when delete throws (GlossaryVersionPage — three tests affected). - Nest apiContext.dispose() inside try/finally in LineagePipelineAnnotator beforeAll/afterAll so page.close() always runs even when dispose throws. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
🔴 Playwright Results — 1 failure(s), 20 flaky✅ 4485 passed · ❌ 1 failed · 🟡 20 flaky · ⏭️ 37 skipped
Genuine Failures (failed on all attempts)❌
|
…text in GlossaryRelations* beforeAll performAdminLogin does a full browser UI login (navigate to /signin, fill credentials) and was hitting the 60-second beforeAll timeout in loaded CI environments. getDefaultAdminAPIContext creates a context from the pre-saved storageState without any UI interaction, which is both faster and more robust. Affected: GlossaryRelationsGraph, GlossaryRelationsGraphPerf, GlossaryTermRelationsGraphNested. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ard 1 OOM PlatformLineage, DataAssetLineage, and LineageInteraction were tagged @basic (routing them to the Basic project on shard 1, 3 parallel workers). Canvas-heavy lineage tests running concurrently with Docker overhead was causing OOM → SIGTERM (exit 143) on shard 1. Remove the tag so these tests return to the chromium shards (2–6) where load is distributed 1/5 each. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ree canvas memory After a ReactFlow lineage test completes, the canvas nodes/edges/SVG stay resident in Chrome until the context closes. Adding an afterEach that navigates to about:blank triggers React to unmount the lineage graph and lets Chrome GC the canvas objects before the next test loads a new page. Applied to the three heaviest lineage files (LineageInteraction, DataAssetLineage, LineageControls). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…DOM detachment After #29642 (keep Explore browse tree expanded), ExploreTree.tsx conditionally skips setIsLoading on browse selections, making the loader-based strategy in expandTreeNode unreliable. The child nodes then briefly detach/reattach during the tree's setTreeData reconciliation, causing the serviceTitle click to fail with "element was detached from the DOM" on all 3 retries. Switch expandTreeNode to the same response-based pattern used by expandServiceInExploreTree / expandDatabaseInExploreTree: set up waitForResponse before the switcher click so the await anchors on the actual data fetch completing, after which tree nodes are stable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eInteraction Reverts the PLAYWRIGHT_BASIC_TEST_TAG_OBJ removal from these two files (originally removed in 2b47c00 to fix shard 1 OOM). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review ✅ Approved 1 resolved / 1 findingsEliminates E2E test resource leaks by wrapping cleanup routines in try/finally blocks and refactoring auth patterns to use managed fixtures. Addresses resource setup leaking on failure by enforcing cleaner lifecycle management. ✅ 1 resolved✅ Quality: Resource setup outside try/finally still leaks on failure
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
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
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.
Describe your changes:
Fixes #29701
I worked on fixing Playwright E2E test resource leaks because unclosed browser contexts and pages from failing tests were appearing in traces of unrelated tests, causing spurious failures and flakiness.
Changes made:
PersonaFlow.spec.ts— wrapuserContext(browser.newContext()) intry/finallyso it always closes even when assertions throwLineagePipelineAnnotator.spec.ts— wrapbeforeAll/afterAllAPI setup intry/finallysoapiContext.dispose()+page.close()run even if an API call throwsDataAssetLineage.spec.ts— destructure and callafterAction()fromgetApiContext()in afinallyblock (was previously silently discarded, permanently leakingAPIRequestContext)Glossary.spec.ts— wrap deny-permission tests intry/finallyforconsumerAfterAction/cleanup/afterActionGlossaryVersionPage.spec.ts— wrapglossary.delete+afterActioninfinallyGlossaryRelationsGraph.spec.ts,GlossaryRelationsGraphPerf.spec.ts,GlossaryTermRelationsGraphNested.spec.ts— replacebrowser.newPage()+adminUser.login()per-test withtest.use({ storageState })+pagefixture; eliminates auth overhead per test and removes the page-leak-on-failure risk entirelyeslint.config.mjs— adds awarn-levelno-restricted-syntaxrule on barebrowser.newPage()in e2e spec files to surface the anti-pattern at code-review time without blocking multi-user tests that legitimately need a second page as a different userType of change:
High-level design:
Two root causes were addressed:
Missing
try/finallyaround cleanup calls — AnyafterAction(),context.close(), orapiContext.dispose()placed as the last statement in a test body is unreachable when an earlier assertion throws. Wrapping the test body intry { ... } finally { cleanup }guarantees execution. Applied toPersonaFlow,LineagePipelineAnnotator,DataAssetLineage,Glossary, andGlossaryVersionPage.browser.newPage()+ manual login inside test bodies — For tests that only need one admin perspective, this pattern re-runs a full login flow per test and leaks the page if the test fails beforepage.close(). The correct pattern istest.use({ storageState: 'playwright/.auth/admin.json' })at file level and letting thepagefixture manage the lifecycle. Applied to the three GlossaryRelations* specs.The ESLint rule uses the selector
CallExpression[callee.object.name='browser'][callee.property.name='newPage'][arguments.length=0]scoped toplaywright/e2e/**/*.spec.*files only (excludesutils/andauth.setup.ts). It iswarnnoterrorbecause multi-user tests legitimately callbrowser.newPage()to open a second page as a different user.Tests:
Use cases covered
browser.newPage()anti-pattern going forwardPlaywright (UI) tests
Manual testing performed
warnfires onTaskPermissions.spec.ts(legitimate multi-user, expected warning)auth.setup.tsis excluded from the rule (glob matches*.spec.tsonly)UI screen recording / screenshots:
Not applicable.
Checklist:
Summary by Gitar
afterEachhook inDataAssetLineage.spec.ts,LineageInteraction.spec.ts, andLineageControls.spec.tswithpage.goto('about:blank')to clear memory and prevent canvas leaks.PLAYWRIGHT_BASIC_TEST_TAG_OBJfromPlatformLineage.spec.ts,DataAssetLineage.spec.ts, andLineageInteraction.spec.tssuites, which may affect automated test selection in CI.This will update automatically on new commits.
Greptile Summary
This PR fixes Playwright E2E test resource leaks — unclosed browser contexts, pages, and API request contexts that were contaminating traces of unrelated tests. Affected files receive
try/finallywrapping around cleanup calls, and threeGlossaryRelations*specs are refactored to usetest.use({ storageState })+ thepagefixture instead of per-testbrowser.newPage()+ manual login.PersonaFlow,LineagePipelineAnnotator,DataAssetLineage,Glossary, andGlossaryVersionPagespec files wrap cleanup calls intry/finallysoapiContext.dispose()/page.close()/context.close()always execute even when assertions throw.GlossaryRelationsGraph,GlossaryRelationsGraphPerf, andGlossaryTermRelationsGraphNestedreplaces per-test page creation and login with the built-inpagefixture, eliminating auth overhead and page-leak-on-failure risk.no-restricted-syntax,warnlevel) added to surface futurebrowser.newPage()anti-patterns in spec files at review time.Confidence Score: 5/5
Safe to merge — all changes are confined to E2E test files and ESLint config with no production code affected.
The changes correctly address the stated resource-leak patterns. Two test cleanup sequences remain partially unguarded (setup calls before try in GlossaryVersionPage, and sequential finally steps in Glossary), but these are test-infrastructure robustness gaps rather than correctness issues that would break application behavior or block other tests from running.
GlossaryVersionPage.spec.ts (setup calls before try) and Glossary.spec.ts (sequential cleanup in finally) have incomplete resource-safety, though both are incremental improvements over the pre-PR state.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Test Body / Hook starts] --> B{try block} B --> C[Test assertions / API calls] C -->|assertion throws| D[finally block] C -->|passes| D D --> E{Nested try/finally\nfor cleanup step 1} E --> F[cleanup step 1\ne.g. consumerAfterAction / glossary.delete] F -->|throws| G[inner finally\ncleanup step 2 still runs] F -->|ok| G G --> H[cleanup step 2\ne.g. afterAction / apiContext.dispose] B2[Old pattern: sequential finally] --> C2[cleanup step 1] C2 -->|throws| Z[cleanup step 2 SKIPPED — resource leak] C2 -->|ok| D2[cleanup step 2] style Z fill:#f88,stroke:#c00 style G fill:#8f8,stroke:#080%%{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[Test Body / Hook starts] --> B{try block} B --> C[Test assertions / API calls] C -->|assertion throws| D[finally block] C -->|passes| D D --> E{Nested try/finally\nfor cleanup step 1} E --> F[cleanup step 1\ne.g. consumerAfterAction / glossary.delete] F -->|throws| G[inner finally\ncleanup step 2 still runs] F -->|ok| G G --> H[cleanup step 2\ne.g. afterAction / apiContext.dispose] B2[Old pattern: sequential finally] --> C2[cleanup step 1] C2 -->|throws| Z[cleanup step 2 SKIPPED — resource leak] C2 -->|ok| D2[cleanup step 2] style Z fill:#f88,stroke:#c00 style G fill:#8f8,stroke:#080Comments Outside Diff (2)
openmetadata-ui/src/main/resources/ui/playwright/e2e/VersionPages/GlossaryVersionPage.spec.ts, line 51-55 (link)trystill leakapiContextglossary.create()andglossary.patch()are called before thetryblock on lines 52–53. If either throws,afterAction()(which disposesapiContext) andglossary.delete()in thefinallyare never reached — the very leak this PR aims to fix still occurs for setup failures. Moving thetryto wrap fromawait glossary.create(apiContext)through the end would close this gap. The same pattern exists in bothGlossary.spec.tsdeny-permission tests, whereperformAdminLogin,setupGlossaryDenyPermissionTest, andperformUserLoginall run before theirtryblocks.openmetadata-ui/src/main/resources/ui/playwright/e2e/VersionPages/GlossaryVersionPage.spec.ts, line 141-144 (link)finallycleanup — later steps are skipped if the first throwsIf
glossary.delete(apiContext)throws (e.g., entity not found after a partial setup failure),afterAction()on the next line is never called, leavingapiContextundisposed. The same pattern appears inLineagePipelineAnnotator.spec.ts(apiContext.dispose()/page.close()) andGlossary.spec.ts(consumerAfterAction()/cleanup()/afterAction()). Wrapping each step with its owntry/catchor usingPromise.allSettled-style chaining would ensure all cleanup steps run regardless of individual failures.Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile