-
Notifications
You must be signed in to change notification settings - Fork 3.3k
chore: port websocket in ext to bidi #31209
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
chore: port websocket in ext to bidi #31209
Conversation
cypress
|
Project |
cypress
|
Branch Review |
chore/port_websocket_in_ext_to_bidi
|
Run status |
|
Run duration | 09m 22s |
Commit |
|
Committer | Bill Glesias |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
694
|
|
0
|
|
130
|
View all changes introduced in this branch ↗︎ |
a7c7f64
to
8f73f05
Compare
07c5a8e
to
52676da
Compare
52676da
to
70e438d
Compare
ee30eb9
to
425bbaa
Compare
c7f8493
to
e3bac5f
Compare
caa42e1
to
e970a54
Compare
…te cookie behavior similar to CDP client
e970a54
to
f76f89d
Compare
@@ -122,287 +121,6 @@ describe('lib/socket', () => { | |||
}) | |||
}) | |||
|
|||
context('on(automation:request)', () => { |
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.
Some of these tests I wanted to keep but I can't even get them to run locally
|
||
// CLOSE ALL BUT THE NEW CONTEXT, which makes it active | ||
// also do not need to navigate to about:blank as this happens by default | ||
for (const context of contexts) { |
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.
TODO: add #31480 to comments
return false | ||
} | ||
|
||
if (filter.path && filter.path !== cookie.path) { | ||
if (filter?.path && !pathMatch(filter.path, cookie.path)) { |
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 a part of the migration? Or a separate improvement? Mostly just curious.
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.
its an improvement which we need since bidi cookie filter criteria is a bit different. What we really want is to check path hierarchy and not an exact match.
For instance, cookies going to request /foo
should send cookies that have /
or /foo
as the path, but not /bar
. If we are checking for strict equality on just path, only 1 of those 3 would be sent, which is incorrect. We don't notice this with CDP because it does this implicitly when we look up cookies
Looks good to me. Ran some e2e/ct tests and Run all specs in firefox. |
@packages/extension
methods that are now implemented by WebDriver BiDi when BiDi is enabled #30221Additional details
Cuts over the remaining methods in the web extension that are possible (currently only used by firefox) to WebDriver BiDi. This gets us about 90% off of the websocket in the webextension via this BiDi port, which also helps us when debugging the app code in the server.
This should close the epic #29714, but does not entirely get us off the web extension for socket commands as documented in #30447 (see #30221 (comment) for more details on what is still needed and why).
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?