Skip to content

chore: lift up playwright prelaunch #36330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 19, 2025

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Jun 16, 2025

Prep work for multiple connections to the same server. makes sure all connections use the same Playwright instance.

@Skn0tt Skn0tt requested a review from dgozman June 16, 2025 15:28
@Skn0tt Skn0tt self-assigned this Jun 16, 2025

This comment has been minimized.

This comment has been minimized.

for (const browser of playwright.allBrowsers())
await browser.close({ reason: 'Connection terminated' });
});
this._cleanups.push(() => browser.close({ reason: 'Connection terminated' }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this change here makes sense, let's keep the "close all browsers upon disconnect" logic in PlaywrightServer when we are running in non-extension mode for now.

Copy link
Member Author

@Skn0tt Skn0tt Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it back to the PlaywrightConnection#onClose handler, is that what you had in mind?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that fails all multiclient tests. Probably not what you had in mind.


onClose: async () => {
debugLogger.log('server', 'closing browsers');
if (this._preLaunchedPlaywright)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this code, but for connection.onClose() scenario as mentioned in another comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got confused by this code. I thought that onClose is about the WS connection closing. But instead, it's about WS server winding down. I'm unsure if it makes sense to kill browsers that were injected via preLaunchedBrowser - reverted this change for now.

if (options.preLaunchedAndroidDevice)
this._preLaunchedPlaywright = options.preLaunchedAndroidDevice._android.attribution.playwright;
this._playwright = options.preLaunchedAndroidDevice._android.attribution.playwright;
this._playwright ??= createPlaywright({ sdkLanguage: 'javascript', isServer: true });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like both sdkLanguage and socksProxyPort options should become per-browser instead of per-playwright.

Copy link
Member Author

@Skn0tt Skn0tt Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per-browser, or maybe per connection instead? That'd be a much bigger refactoring though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"connection" does not exist on the server-side that operates sdk objects. So "per browser".

@Skn0tt Skn0tt requested a review from dgozman June 17, 2025 12:15

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

Test results for "tests 1"

7 flaky ⚠️ [firefox-library] › library/browsercontext-fetch.spec.ts:900:3 › should respect timeout after redirects @firefox-ubuntu-22.04-node18
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:986:7 › cli codegen › should not throw csp directive violation errors @firefox-ubuntu-22.04-node18
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-ubuntu-22.04-node18
⚠️ [chromium] › headerView.spec.tsx:46:1 › should toggle filters @web-components-html-reporter
⚠️ [webkit-library] › library/browsercontext-viewport-mobile.spec.ts:124:5 › mobile viewport › respect meta viewport tag @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/screenshot.spec.ts:205:14 › element screenshot › element screenshot should work with a mobile viewport @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

39426 passed, 822 skipped
✔️✔️✔️

Merge workflow run.

@Skn0tt Skn0tt merged commit 66e9030 into microsoft:main Jun 19, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants