misc: Where possible allow Firefox and Chrome to atomically allocate ports (#25498)

[ngladitz]

Firefox and Chrome let the system allocate a port when started with --remote-debugging-port=0 and announce the port on stderr afterwards where it is now parsed from.

Do something similar for Firefox's Marionette port and parse it from a message that gets written to stdout.

This doesn't seem possible with Firefox's devtools.debugger.remote-port so at least allow it to be overridden via preferences so users can implement their own race free port allocation mechanism when / where required:

            setupNodeEvents: (on, config) => {
                on('before:browser:launch', (browser, opts) => {
                    opts.preferences['devtools.debugger.remote-port'] = 12345
                    return opts;
                })
            }

• Closes #25498

User facing changelog

• Allow users to override Firefox's devtools.debugger.remote-port via a before:browser:launch hook in setupNodeEvents. This allows to work around issues which may result from automatically assigned / random ports.

Steps to test

To test custom 'devtools.debugger.remote-port' override use the snippet from the description and verify that the given port is used.

For the remote debugging port in Chrome / Firefox verify port 0 is requested from the command line (--remote-debugging-port=0), verify a random port is used and that the connection can still be established. For Firefox's marionette port a 0 should be written to the marionette.port preference resulting in a random port as well.

PR Tasks

• [x] Have tests been added/updated?
• [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• [na] Has a PR for user-facing changes been opened in cypress-documentation?
• [na] Have API changes been updated in the type definitions?

[CLAassistant]

All committers have signed the CLA.

[cypress[bot]]

---

Test summary

26575 • 0 • 1279 • 0 • 48

---

Run details

Project	cypress
Status	Passed
Commit	18c6fcc945
Started	Jan 24, 2023 1:51 PM
Ended	Jan 24, 2023 2:10 PM
Duration	19:28 💡
OS	Linux Debian -
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	create-from-component.cy.ts		1
1	... > runs generated spec
	e2e/origin/cookie_login.cy.ts		2
1	cy.origin - cookie login > Max-Age > past max-age -> not logged in
2	... > past Max-Age, before Expires -> not logged in
	commands/net_stubbing.cy.ts		2
1	network stubbing > intercepting request > can delay and throttle a StaticResponse
2	network stubbing > intercepting request > can delay and throttle a StaticResponse
This comment includes only the first 5 flaky tests. See all 48 flaky tests in the Cypress Dashboard.

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[ngladitz]

@emilyrohrbough Thank you for looking into this.

The ideal solution would be to allow the browser to bind to a random port and we pull this dynamic port from the browser. You are right that we can do this for the marionette port. I was able to draft this up on this branch: https://github.com/cypress-io/cypress/compare/emily/ff-dynamic-marionette-port

Thank you that looks good and seems to work for me as well. I experimented with something like this but was missing user_pref("browser.dom.window.dump.enabled", true); in my very reduced (just Firefox without Cypress; Cypress already sets the pref) testcase. Without it the marionette port does not get output to the console.

I think this could also be done for the port that is currently covered by the environment variable CYPRESS_REMOTE_DEBUGGING_PORT and which is passed to Firefox via the command line option --remote-debugging-port <port>. When I run Firefox with --remote-debugging-port 0 it binds to a random port (in the example output below this happens to be 32819) and outputs it as part of the following lines:

WebDriver BiDi listening on ws://127.0.0.1:32819
DevTools listening on ws://127.0.0.1:32819/devtools/browser/69d2d46c-4d87-4505-b51b-cf53fa299317

I am not sure about the devtools.debugger.remote-port at this time...though looking at where we determine this port value, I see we are determining this port so early to give the user this value in the before:browser:launch event, which is likely why there is a race condition on when it's "free" to when we try to use it.

With an explicit port e.g. 8081 I get the following output from Firefox:

Started devtools server on 8081

With devtools.debugger.remote-port set to 0 I get no corresponding output from Firefox; implying the devtools server does not get started at all.

So at least for this last case some way to manually supply the port would still be appreciated.

These port values can't be managed today without the ENVS because we aren't honoring and pulling these values back out from the user defined options. If we can pull these out from the launchOptions](https://github.com/cypress-io/cypress/compare/emily/ff-dynamic-marionette-port#diff-6ec303ebac972615597a2bbcc7cd2986c44cea3ff5b0eccb0b8724a8527d1972R468) we can pass them into [firefox.setup()`, which would override the dynamic port allocation we are doing.

This would allow for:

    setupNodeEvents (_on, config) {
      _on('before:browser:launch', (browser, opts) => {
        if (process.env.CYPRESS_FOXDRIVER_PORT) {
          opts.preferences['devtools.debugger.remote-port'] = process.env.CYPRESS_FOXDRIVER_PORT
        }

        if (process.env.CYPRESS_MARIONETTE_PORT) {
          opts.preferences['marionette.port'] = process.env.CYPRESS_MARIONETTE_PORT
        }

        return opts
      })
  },

I am not sure I fully understand this last part (I think the link may have been miscopied?) but if I understand the intention of the example this does look promising. Being able to override devtools.debugger.remote-port would suffice for me if the other two ports could be dynamically allocated by Firefox itself.

How would you like to proceed?

[emilyrohrbough]

@ngladitz there is a lot to comprehend here - I am going to comment in the code to break this conversation up to make this a bit easier to talk back and forth on.

[emilyrohrbough]

@ngladitz Just curious, do you have any updates with this PR?

[ngladitz]

@ngladitz Just curious, do you have any updates with this PR?

I hope I didn't misunderstand but the code changes in my branch associated with this PR are entirely obsoleted by the more sensible approach you implemented in your own ff-dynamic-marionette-port branch. I think the only thing missing is parsing the remote debugging port from the DevTools listening on message rather than the WebDriver BiDi listening on message(?).

[emilyrohrbough]

@ngladitz I was hoping you'd be able to borrow some of these change and pull them in to your change set. These changes I played with won't be something I can prioritize working on fully to get across the line. Sorry for the confusion.

[ngladitz]

@emilyrohrbough sorry about that. Based on your branch I generalized the changes to also cover Chrome's remote debugging port (apparently even the message with the port output by both browsers is the same).

I tested everything locally through manual testing as far as I could follow the possible implications but where I am unsure is Chrome with an existing instance. I hope this is covered by automated testing / CI as I don't really know how it is used exactly.

Also not really sure how automated testing and CI work here; I see failures but I can't tell what they are or if they are related to my changes.

[mschile]

@ngladitz, looks like some of the test failures (server-unit-tests and server-integration-tests) are from this PR along with some linting errors.

To run the server unit-tests: yarn workspace @packages/server test-unit chrome_spec yarn workspace @packages/server test-unit firefox_spec

To run the server integration tests: yarn workspace @packages/server test-integration cypress_spec

[ngladitz]

@mschile Thank you for the pointers. I Think I managed to fix the tests you indicated (at least locally). Mostly stubs not implementing the new browser requirements. I also dropped some pre-86 Firefox assumptions which I assume should not be relevant since >= 86 is required.

I put the changes into a new commit as I wasn't sure of what you'd prefer. I've been rebasing / rewriting history earlier but given that 'develop' was merged back into my branch I guess you might prefer a new commit?

[mschile]

I put the changes into a new commit as I wasn't sure of what you'd prefer. I've been rebasing / rewriting history earlier but given that 'develop' was merged back into my branch I guess you might prefer a new commit?

Yes, in general a new commit is preferred. We'll squash merge the PR into develop at the end.

[mschile]

Hi @ngladitz, would you be able to update the PR description with the most up to date information.

[ngladitz]

@mschile I've updated the description and fixed the check-ts test (I dropped the unused port argument from the launch signature but apparently forgot to drop it from one of the call sites). I managed to run the test locally which now seems to pass but I am unsure how to interpret or run (locally or otherwise) the rest of the failing tests.

[cacieprins]

Hi @ngladitz ! Thank you for updating the description, and fixing the check-ts test. Have you made any progress on resolving the failing unit tests? You can run these tests on your forked branch with the following commands:

For server unit tests: yarn workspace @packages/server test-unit chrome_spec yarn workspace @packages/server test-unit firefox_spec

To run the server integration tests: yarn workspace @packages/server test-integration cypress_spec

[ngladitz]

@cacieprins thank you. @mschile did mention those before and I was so sure I had them passing locally but rerunning them now they certainly don't. I must have missed a step somewhere.

I pushed a new commit and they all pass now except for the following integration test:

  1) lib/cypress --run-project logs error and exits when project folder has read permissions only and cannot write cypress.config.js:

      error was logged
      + expected - actual

       [
      -  "Error: Can't run because \u001b[95mno spec files\u001b[39m were found.\n\nWe searched for specs matching this glob pattern:\n\n\u001b[90m  > \u001b[39m\u001b[94m/home/ngladitz/src/git/cypress/packages/server/permissions/\u001b[33mcypress/e2e/**/*.cy.{js,jsx,ts,tsx}\u001b[94m\u001b[39m"
      +  {
      +    "type": "ERROR_WRITING_FILE"
      +  }
       ]

The description of the test seems to suggest that the expectation is that removing write permissions to the directory containing a cypress.config.js would remove permissions to the file itself as well ... which it doesn't so the test failure seems justified as far as I can tell. I might misinterpret the description / intention (doesn't cypress just consume the file? why does it need write permissions?) or maybe this is something system specific. Either way I presume this failure has nothing to do with my changes given that I haven't touched anything that would feel related(?)

[cypress-app-bot]

This PR has not had any activity in 180 days. If no activity is detected in the next 14 days, this PR will be closed.

[cypress-app-bot]

This PR has been closed due to inactivity