-
Notifications
You must be signed in to change notification settings - Fork 291
Enable Support for WebDriver-Bidi in Firefox #619
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
base: main
Are you sure you want to change the base?
Conversation
cdbe8d6 to
8b5a2a5
Compare
gregberge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the concept of this PR sorry. We are now disconnecting all browsers instantely and we connect / disconnect into the close browsers function. Something is not clear to me.
| ); | ||
| const wsEndpoints = browsers.map((browser) => browser.wsEndpoint()); | ||
| saveWsEndpoints(wsEndpoints); | ||
| browsers.forEach((browser) => browser.disconnect()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why disconnecting directly?
| while (wsEndpoints.length) { | ||
| const wsEndpoint = wsEndpoints.pop()!; | ||
| closeRequests.push( | ||
| puppeteer | ||
| .connect({ | ||
| ...(config.launch?.browser === "firefox" && { | ||
| protocol: "webDriverBiDi", | ||
| }), | ||
| ...config.connect, | ||
| ...config.launch, | ||
| browserURL: undefined, | ||
| browserWSEndpoint: wsEndpoint, | ||
| }) | ||
| .then((browser) => browser.close()), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just do a map instead of doing a loop like that?
| ...(config.launch?.browser === "firefox" && { | ||
| protocol: "webDriverBiDi", | ||
| }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have a hack here?
| puppeteer | ||
| .connect({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we connecting into the closeBrowsers
|
@gregberge I decided to reply here rather than in-line as you may prefer an alternative approach. Firefox 141+ no longer works because it dropped supported for CDP (puppeteer also remove CDP support for Firefox) and it does not support multiple connections over WebDriver BiDi (CDP did). The easiest way to re-enable support for Firefox is to first launch the browser and record the wsEndpoint. We have to disconnect after the initial setup as other connections are made during jest-puppeteer/packages/jest-environment-puppeteer/src/browsers.ts Lines 125 to 136 in ddb3fb9
This will throw in firefox. See #609. I suppose an alternative (and cleaner) fix here would be to return the existing browser instance from |
|
@gregberge Just following up on this. Is there one approach you'd prefer over the other? |
Summary
Closes #609.
Support for CDP was removed in Firefox v141, leaving WebDriver BiDi as the only supported automation protocol. However, Firefox currently does not support multiple concurrent WebDriver BiDi sessions.
To work around this limitation, the setup has been modified to:
wsEndpointSubsequent access to the browser is handled via
puppeteer.connect()using the savedwsEndpoint, ensuring only a single active connection at any time.Test Plan
Firefox-specific tests were added and can be run via the
test:firefoxnpm script.