-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
misc: implement webdriver on top of geckodriver in order to reduce overhead maintenance and code #30324
Conversation
cypress Run #57639
Run Properties:
|
Project |
cypress
|
Run status |
Passed #57639
|
Run duration | 23m 56s |
Commit |
e8ab563b78: address comments from code review [run ci]
|
Committer | AtofStryker |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
9
|
Pending |
1326
|
Skipped |
0
|
Passing |
29395
|
UI Coverage
45.96%
|
|
---|---|
Untested elements |
189
|
Tested elements |
165
|
Accessibility
92.55%
|
|
---|---|
Failed rules |
3 critical
8 serious
2 moderate
2 minor
|
Failed elements |
906
|
06d417c
to
5fc6148
Compare
5fc6148
to
dae5968
Compare
dae5968
to
69b7196
Compare
@@ -181,7 +181,7 @@ export class DataContext { | |||
@cached | |||
get cloud () { | |||
return new CloudDataSource({ | |||
fetch: (...args) => this.util.fetch(...args), | |||
fetch: (...args: [RequestInfo | URL, (RequestInit | undefined)?]) => this.util.fetch(...args), |
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.
ran into typings issues, my guess with the updated lock, which is kinda weird but I just chose to strongly type the lines that were failing
268d44c
to
5809c5f
Compare
0132fe9
to
4f3804d
Compare
ce23221
to
a2b40a6
Compare
@mschile @cacieprins @ryanthemanuel this should now be ready for review. I checked the binary size and it looks to at 6MB to the total binary, which is about a 5% increase. I also noticed #30352 is still happening intermittently on this branch, so I am going to work on resolving this issue but in a separate PR |
@@ -0,0 +1,79 @@ | |||
diff --git a/node_modules/@wdio/protocols/README.md b/node_modules/@wdio/protocols/README.md |
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 usually delete the README.md patches but no biggie.
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 ran into so many issues with patch-package and the yarn clean I am afraid to touch 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.
A couple of small things, but nothing major. Nice work!
|
||
* Navigating to the current/next spec URL via [WebDriver Classic](https://w3c.github.io/webdriver.). | ||
Cypress uses [GeckoDriver](https://firefox-source-docs.mozilla.org/testing/geckodriver/index.html) to drive [classic WebDriver](https://w3c.github.io/webdriver) methods, as well as interface with Firefox's [Marionette Protocol](https://firefox-source-docs.mozilla.org/testing/marionette/Intro.html). This is necessary to automate the Firefox browser in the following cases: |
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 I ask for which reason you still need the Marionette protocol? I assumed that Marionette is no longer used since the move from marionette-js to geckodriver.
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.
@whimboo we are using marionette
through geckodriver
since we need to install our web extension unsigned to automate a few additional things in the browser that we do not get with CDP. Based on the source code and the browser debug logs, this looks to use marionette under the hood. We are just abstracting the usage through webdriver.
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.
Well, that is just our WebDriver classic implementation with the vendor extension to install WebExtensions. As such you shouldn't have to call this out separately given that geckodriver acts as a proxy for that command.
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 add an edit into the changelog that just states we are just using webdriver. Might be simpler at least for user facing details
@@ -3,7 +3,7 @@ import type { SocketShape } from '@packages/socket/lib/types' | |||
import type { ClientOptions } from '@urql/core' | |||
|
|||
export const urqlFetchSocketAdapter = (io: SocketShape): ClientOptions['fetch'] => { | |||
return (url, fetchOptions = {}) => { | |||
return (url, fetchOptions: RequestInit = {}) => { |
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.
💯
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
With the merging of #30250 and geckodriver being implemented, this PR takes the next step to move all WebDriver Classic methods out of our temporary homegrown http api and uses webdriver commands in its place to reduce overhead, and additionally abstracts the arguments needed to start
geckodriver
to thewdio:geckodriverOptions
capability.Also, in #30250 , we replaced the
firefox-profile
xulstore.json
with the maximize command to record users saved screen preference. This reimplements thexulstore
that was removed as we just needed to convert the profile to base64 and set the preferences through the browser instead of the profile. However, this might now have the same sizing effect it previously did since the profile is copied over on each run, so the changes are likely not persisted.Steps to test
Unit tests have been updated to reflect the replaced client.
The system-tests and integration tests serve as a regression suite to help guarantee this change works.
Manual testing should also be completed on linux, mac, and windows in open/run mode permutations to verify the code and client changes work universally. The app team will be participating in a small bughunt to identify issues we may have not already found.
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?