misc: replace marionette-client with geckodriver as b2g marionette client is no longer supported #30250
Conversation
AtofStryker
force-pushed
the
misc/remove_marionette_for_geckodriver
branch
2 times, most recently
from
161ebbf
to
7a1dc85
Compare
AtofStryker
force-pushed
the
misc/remove_marionette_for_geckodriver
branch
20 times, most recently
from
cac7939
to
192204d
Compare
AtofStryker
requested review from
ryanthemanuel and
cacieprins
and removed request for
ryanthemanuel and
cacieprins
AtofStryker
force-pushed
the
misc/remove_marionette_for_geckodriver
branch
from
3488c1c
to
fb54eaa
Compare
There was a problem hiding this comment.
Hi @AtofStryker. It's great to see the progress you've made over the past days. I hope it's fine when I just added a couple of minor comments and suggestions. You can find those all inline. I'm kinda looking forward to see it working, and especially the removal of the Marionette JS client usage.
| MOZ_HEADLESS_WIDTH: '1280', | ||
| MOZ_HEADLESS_HEIGHT: '721', | ||
| MOZ_HEADLESS_HEIGHT: '806', |
There was a problem hiding this comment.
Above you mention a total offset of 86px but this change is 85px only. Not sure if that matters but wanted to just mention.
There was a problem hiding this comment.
It's 85 plus whatever the height for the shrunken url bar, which is the 1 px already aded to the 721 here
| host: '127.0.0.1', | ||
| port: geckoDriverPort, | ||
| marionetteHost: '127.0.0.1', | ||
| marionettePort, |
There was a problem hiding this comment.
I assume this can be defined by the user? Please note that if set to 0 Firefox will automatically select a free port on the system.
There was a problem hiding this comment.
we define this on their behalf. We just find an available port on the machine to run it
| ## Debugging | ||
|
|
||
| To help debug `geckodriver` and `firefox`, the `DEBUG=cypress-verbose:server:browsers:geckodriver` can be used when launching `cypress`. This will | ||
| * Enable full `stdout` and `stderr` for `geckodriver` (including startup time) and the `firefox` process. The logs will **NOT** be truncated so they may get quite long. |
There was a problem hiding this comment.
Please note that geckodriver also accepts a -vv argument which will enable trace level logs for Firefox as well. This is pretty helpful in case you see some broken behavior or misbehavior by our implementation which needs to be investigated.
There was a problem hiding this comment.
does this have the same effect as setting --log=trace?
There was a problem hiding this comment.
Yes, one provides a string while for easier use we added the -v[v] argument.
| await Bluebird.resolve(waitPort({ | ||
| port: opts.port, | ||
| // add 1 second to the timeout so the timeout throws first if the limit is reached | ||
| timeout: timeout + 1000, |
There was a problem hiding this comment.
I assume this is waiting for a connection? Geckodriver itself waits up to 60s internally after Firefox is launched for the socket to Marionette and then fails & kills the browser process. Not sure if that would help you if you have a shorter timeout. There is no command line argument for that yet.
There was a problem hiding this comment.
correct. This is just waiting for geckodriver to be running before we proceed. If we reach the timeout threshold, we will the process if once exists, which ultimately just results in the app process tree being killed
There was a problem hiding this comment.
Just to be sure it's waiting for geckodriver running and allowing connections on its HTTP port, right? Thanks.
There was a problem hiding this comment.
yes that is correct to the best of my knowledge. wait-port should wait for the http endpoint on the port to response as well as the record to be available in the DNS.
Thank you for the review @whimboo! Once we get this in, I should have more updates to post on the bugzilla thread as far as progress / next steps. |
| // For whatever reason, we NEED to bind to stderr/stdout in order | ||
| // for the geckodriver process not to hang, even though the event effectively | ||
| // isn't doing anything without debug logs enabled. | ||
| geckoDriverChildProcess.stdout?.on('data', (buf) => { |
There was a problem hiding this comment.
@whimboo do you have any idea why the above comment would happen? @mschile and I tried gating the stdout/stderr behind an if statement and it looks to cause the process to almost freeze (is still running but appears stuck). We tested this on Firefox 125/130 with geckodriver 0.35.0. Is it possible the driver is timing out or something like that?
There was a problem hiding this comment.
No I don't know why that happens. If you may have the time and could create a minimal stand-alone testcase to reproduce this behavior it would be great. We haven't heard about it yet.
One thing which could happen is if a pipe was setup for stdout/stderr but nothing is read from it. That means that geckodriver will run into an overflow slowing down massively first. We had something similar in the past for Puppeteer when logging was enabled but the pipe was not read. It was fixed via puppeteer/puppeteer#7644.
…ove_marionette_for_geckodriver
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
As a precursor to supporting WebDriver BiDi, Cypress needs to replace the older marionette-client with geckodriver. This is the current recommendation from the Mozilla team as
marionette-clientis no longer supported (see Cypress migration bugzilla ticket).Currently, the responsibilities of the current
geckodriverimplementation and the oldermarionette-client:WebDriver Classicsession.WebDriver Classicto navigate the browser. This will be replaced by BiDi in the future.Marionetteto install the Cypress Web Extension.This is outlined in the added
READMEin this PR as well as some other useful information. This information will be updated as we implementBiDi.NOTE: right now, the WebDriver Classic methods are implementing the spec through HTTP instead of using a package or a client. This was chosen to keep the package change minimal in this PR and we will likely be moving to some type of client to reduce the overhead needed to maintain WebDriver. This will be either an OSS package like webdriverio/webdriver or we implement our own (work for either scoped to #30219)
It is worth noting that we patch the
geckodriverpackage to coincide with our debug logs in order to not print extraneous messages to the console that could disrupt end user experience as well as impact our system tests. We also need to patch top-level awaits to correctly build the app and feed in process args that are needed (such as theMOZ_prefix variables) when spawning firefox.To consume
GeckoDriver, this code installs thegeckodriverpackage, a lightweight wrapper around the geckodriver binary, to connect to the Firefox browser.geckodriverwill be downloaded automatically to the user'stmpdirectory if not already downloaded, and will be started and managed by the Cypress server.Bugfixes found while testing
SIGINTfromctrl+cin the terminal (fixed in 192204d)xulstore.jsonno longer has the same effect since geckodriver is creating a full profile (fixed in ae36a24). this will only take effect when headed and if the user does NOT supply a width or height to the browserSteps to test
Unit tests have been updated to reflect the replaced client, as well as tests added for our
geckodriverwrapper.The
system-testsandintegrationtests serve as a regression suite to help guarantee this change works.Manual testing should also be completed on
linux,mac, andwindowsin 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?
Ideally, this change should NOT impact the end user. This is an internal swap of a client used to automate the browser and should not affect the end user in any way.
PR Tasks
cypress-documentation?type definitions?