-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat: implement webdriver BiDi for Firefox versions 135 and greater #30870
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
Conversation
cypress
|
||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
feat/implement_bidi
|
| Run status |
|
| Run duration | 17m 55s |
| Commit |
|
| Committer | AtofStryker |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
2
|
|
|
77
|
|
|
0
|
|
|
5623
|
| View all changes introduced in this branch ↗︎ | |
Warning
No Report: Something went wrong and we could not generate a report for the Application Quality products.
d8df75c to
21ebfa8
Compare
6d48392 to
ab077a7
Compare
5264fcb to
8b3b779
Compare
3d23335 to
534be08
Compare
46edeb9 to
9c6e162
Compare
534be08 to
aecd4f6
Compare
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.
the header attachment is completely handled within the bidi_automation class
aecd4f6 to
be3a9dd
Compare
9c6e162 to
5219967
Compare
af8f1cd to
d605883
Compare
a7417d7 to
58fac0b
Compare
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 know it's not currently typical for the middleware here, but can this be in a separate file and exported from request-middleware.ts? That conforms a little closer to SRP, and would be a good move forward for the middlewares here
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.
exported from request-middleware or imported? I'm a bit hesitant to introduce a one off pattern here for a function so small, unless we refactor the middleware in a separate PR.
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.
Both, considering how the default export from this file is used - but I think that pattern needs a larger look. Leave it for now!
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.
these two conditionals can be combined
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.
updated this in 279b7561ba
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.
Oh, I was thinking of combining the nested ifs, rather than the isCDPForced
58fac0b to
279b756
Compare
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.
Is this spelled out in a spec somewhere? Just curious if we might need to know if this might change in the future. Do we have any firefox tests that would break in that case?
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.
in the Bidi spec or the cookie spec. I don't think they are in either. If it does change my guess is it wouldn't have an impact since we are only checking for the deviation. but that would mean the code would effectively be dead
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.
Do we need to unsubscribe to BIDI_EVENTS here as well?
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.
We could but its a bit hard to accomplish. We could probably do it instance kill but the events effectively get unsubscribed when geckodriver and firefox are killed AFAIK
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.
Couple of small questions and suggestions but looks good overall.
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 can't remember, did we talk about whether or not this would be considered a breaking change? Like if someone is intercepting a certain resource type but the types are different now so the intercept breaks.
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.
We did briefly which was one of the rationales around deprecating the field. The idea that we are trying our best to map these 1:1 and the types be the same shouldn't make it a breaking change, but it's difficult to predict the behavior for each type. That being said I don't think its a breaking change but there could be bugs that we will need to fix. I feel confident on fetch and xhr types, but the fonts might be a little more nuanced which is in use as seen here. Worse case, if a bug is present with the mapping, the user can opt out with the FORCE_FIREFOX_CDP until we fix it
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.
Does this have any impact on the public API?
https://docs.cypress.io/api/cypress-api/browser#Syntax
https://docs.cypress.io/api/cypress-api/isbrowser
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.
it doesn't but independently I think the type is wrong since its a string in both chrome and firefox. I can update it on the docs PR
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.
the types are actually correct here https://github.com/cypress-io/cypress/blob/develop/cli/types/cypress.d.ts#L107 but incorrect in the docs, so I added them to the docs PR cypress-io/cypress-documentation#6103
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.
Looks great overall! Added a couple questions.
…utover). [run ci]
… the code easier to read
6872595 to
a8e3686
Compare
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
Automates firefox with WedDriver BiDi instead of Chrome Devtools Protocol.
With Chrome Devtools Protocol being deprecated in Firefox 129 and slated for removal in the Firefox browser, Cypress has made the decision to automate Firefox with WedDriver BiDi.
This PR gets the basics cut over to BiDi, mainly the server's middleware prerequest logic and association, along with being able to associate the Application Under Test (AUT)
Currently, this is only enabled automatically for Firefox 135 and up. This PR also only cuts over to BiDi what is absolutely necessary. The rest of the work can be handled in #30221. This also should not be a major impact to users as the automation client is abstracted.
See #30447 for more additional details
Steps to test
Unit tests are added/modified in the server package, but the best way to test is to run cypress against firefox 135 and greater on this branch. To make sure there aren't regressions with the CDP implementation for firefox 134 and below, we run the driver tests additionally on an older version of firefox
How has the user experience changed?
Webdriver BiDi is now the default automation client for Firefox version 135 and up. A warning will be printed to the console (regardless of Firefox version) if CDP is in use for Firefox:
We are also adding a snippet in the docs on the update. If needed,
FORCE_FIREFOX_CDPcan be set as an env variable to force CDP on. The current plan is for CDP support for Firefox to be removed in Cypress 15.PR Tasks
cypress-documentation? chore: add firefox webdriver bidi docs cypress-documentation#6103type definitions?