chore: lift up playwright prelaunch #36330
Conversation
This comment has been minimized.
This comment has been minimized.
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' })); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added it back to the PlaywrightConnection#onClose handler, is that what you had in mind?
There was a problem hiding this comment.
Hmm, that fails all multiclient tests. Probably not what you had in mind.
|
|
||
| onClose: async () => { | ||
| debugLogger.log('server', 'closing browsers'); | ||
| if (this._preLaunchedPlaywright) |
There was a problem hiding this comment.
I like this code, but for connection.onClose() scenario as mentioned in another comment.
There was a problem hiding this comment.
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 }); |
There was a problem hiding this comment.
It looks like both sdkLanguage and socksProxyPort options should become per-browser instead of per-playwright.
There was a problem hiding this comment.
Per-browser, or maybe per connection instead? That'd be a much bigger refactoring though.
There was a problem hiding this comment.
"connection" does not exist on the server-side that operates sdk objects. So "per browser".
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"73 failed 7 flaky39341 passed, 822 skipped Merge workflow run. |
Prep work for multiple connections to the same server. Removes Browser closing from the server, and makes sure all connections use the same Playwright instance.