feat: Add support for optional env key to browser launch options #23624

ryanjduffy · 2022-08-30T17:48:26Z

User facing changelog

n/a

Additional details

Related PR: #22250

In building out support to integrate replay with cypress, I discovered there isn't a way to set environment variables for the browser without putting the onus on the end user. Further, since the browser launch seems to happen in a different process than the plugin, I couldn't inject those values into process.env and have them picked up by the existing logic.

This adds support for an optional env key on the launch options passed to before:browser:launch which is then merged with the defaultBrowserEnv and process.env. I placed it between those two so the default browser family config (defaultBrowserEnv) would be overwritten by browser instance config (browser.env) which would be overwritten by the current process.env.

Steps to test

Create a new cypress project
Add a before:browser:launch handler in setupNodeEvents to add an env variable
Verify that the browser receives that env variable. I added a debug entry to log the environment which is probably sufficient for testing this change.

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation? Update browser launch options for env key cypress-documentation#4730
Have API changes been updated in the type definitions?

cypress-bot · 2022-08-30T17:48:28Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

ryanjduffy · 2022-08-30T17:49:43Z

@emilyrohrbough - Finally got back to this! Let me know what y'all think when you get to it. It'll go a long way to improve the DX for replay.io users integrating with cypress.io!

emilyrohrbough · 2022-09-01T20:45:26Z

@ryanjduffy Thank you so much! I will add this to my list to review and get back to you shortly. Hopefully by end of day today, if not by EOD tomorrow!

emilyrohrbough · 2022-09-01T20:57:16Z

packages/launcher/lib/browsers.ts

@@ -181,7 +182,9 @@ export function launch (

  // allow setting default env vars such as MOZ_HEADLESS_WIDTH
  // but only if it's not already set by the environment
-  const env = Object.assign({}, defaultBrowserEnv, process.env)
+  const env = Object.assign({}, defaultBrowserEnv, browser.env, launchOptionsEnv, process.env)


@ryanjduffy It looks like this change is doing more than accepting env values form the launchOptions. I wouldn't expect envs set on the found browser to be used in the node process that starts the browser. In the previous PR you had opened, you had linked to a few code snipped in reply & you're plugin. Where are these ENV values actually getting applied on the browser obj?

Not sure I follow but I'll share some context here in hopes that clarifies!

Here's the current code in our plugin. It's of course not setting env variables yet because it can't!

https://github.com/replayio/replay-cli/blob/c90c03f08d9ccfcb9bc9a4a6a7c07246ff1b07f5/packages/cypress/src/index.ts#L11-L14

The eventual implementation after this landed would be:

on("before:browser:launch", (browser, launchOptions) => { selectedBrowser = browser.family; reporter.onTestSuiteBegin(undefined, "CYPRESS_REPLAY_METADATA"); launchOptions.env = { RECORD_ALL_CONTENT: 1, RECORD_REPLAY_METADATA_FILE: '/path/to/file.json' }; return launchOptions; });

The goal here being that we need to configure the replay browser with RECORD_ALL_CONTENT to instruct the firefox fork to record everything (unlike the desktop experience in which the user clicks the Rec button) and where to find metadata for the recording which is how we link up the recording with the test result.

This is all happening from the plugin because that's the point at which we have the test context to add the metadata. IIRC, I also didn't find another way to set an environment variables for the launcher node process beyond requiring the user to set them before invoking cypress.

Hope that helps! Happy to have a quick video call to chat if that's useful too.

Also, i'm in the cypress community discord as ryanjduffy if you want to chat there!

@ryanjduffy This is great insights! Thank you!

Explanation makes sense for why we pass launchOptionsEnv (which will contain the REPLAY_ variables) but why did we add browser.env here, too?

Looks like browsers.env was just cruft from my prior impl. @emilyrohrbough 's suggestions to drop those two diffs should do the trick!

packages/launcher/lib/browsers.ts

packages/types/src/browser.ts

lmiller1990

Is there a good way to test this manually? Eg - should we set up Replay locally? Considering this is a new feature, after all, I think we probably should do some testing on our end before we review + merge it.

ryanjduffy · 2022-09-08T19:16:58Z

Force pushed a fresh commit to rebase on develop and match up code style.

ryanjduffy · 2022-09-08T19:19:05Z

Is there a good way to test this manually? Eg - should we set up Replay locally? Considering this is a new feature, after all, I think we probably should do some testing on our end before we review + merge it.

Definitely! I'm walking through the steps myself now to verify it all one more time. Our Cypress instructions cover most of it. I can push an experimental version of our plugin that uses this feature so others can try it as well.

ryanjduffy · 2022-09-08T20:18:43Z

Manual testing steps:

Clone replayio/replayable and checkout demo-cypress-env. You could also follow the instructions and set up your own project for extra credit
```
git clone git@github.com:replayio/replayable && cd replayable && git checkout demo-cypress-env && npm i && npm run dev
```

From cypress, checkout this branch

git fetch upstream +refs/pull/23624/merge && git checkout -qf FETCH_HEAD

Run cypress in dev mode against the replayable repo

yarn cypress:run --project /path/to/replayable --browser "Replay Firefox"

Verify Replay made local recordings. There should be three entries.
```
npx @replayio/replay ls
```

flotwig

This needs a docs PR, this file should be edited: https://github.com/cypress-io/cypress-documentation/blob/master/content/api/plugins/browser-launch-api.md

flotwig · 2022-09-09T20:03:23Z

packages/launcher/lib/browsers.ts

@@ -166,6 +166,7 @@ export function launch (
  debuggingPort: number,
  args: string[] = [],
  defaultBrowserEnv = {},
+  launchOptionsEnv = {},


This is too many parameters for this function, can you either remove the need for this parameter or move the parameters for .launch into an options object?

👍 I'll collapse the two env members into one.

flotwig · 2022-09-09T20:04:03Z

cli/types/cypress.d.ts

@@ -5480,6 +5480,7 @@ declare namespace Cypress {
    extensions: string[]
    preferences: { [key: string]: any }
    args: string[]
+    env: { [key: string]: any }


This is essentially a no-op in Electron, should we add an error or warning if someone sets .env with Electron?

👍 I noticed that args seems to be the same so I've added a warning for both of those. Let me know if I've misread and args is used or if there's a different way that is preferred for warning.

ryanjduffy · 2022-09-10T21:09:15Z

This needs a docs PR, this file should be edited: https://github.com/cypress-io/cypress-documentation/blob/master/content/api/plugins/browser-launch-api.md

Opened cypress-io/cypress-documentation#4723

lmiller1990 · 2022-09-11T23:27:21Z

@ryanjduffy works great for me, I followed the steps and got it:

ID                                    Status  Title                          Created At
790783cd-f2c1-4c02-891c-fef7e2223767  onDisk  cypress/e2e/view-issues.cy.js  2022-09-11T23:15:34.279Z
054c8d69-fb1b-4429-a9da-6de10c48d738  onDisk  cypress/e2e/navigation.cy.js   2022-09-11T23:15:27.215Z
0943eec6-b68f-440b-86c1-341e161c7b76  onDisk  Replay of localhost:3000       2022-09-11T23:15:11.757Z

Left a quick comment on the docs PR, I'll wait for @flotwig to loop back and approve this PR, but looks good to me.

lmiller1990

Sorry for the delay, one last question!

lmiller1990 · 2022-09-20T07:48:03Z

packages/server/lib/browsers/electron.ts

+    if (launchOptions.args.length > 0) options.push('args')
+
+    if (options.length > 0) {
+      errors.warning('BROWSER_UNSUPPORTED_LAUNCH_OPTION', 'electron', options)


@ryanjduffy sorry last comment here before approval... how can I actually pass something to this? I want to verify this error actually shows up, but I couldn't figure out how.

Something like this should do it!

on("before:browser:launch", (browser, launchOptions) => { launchOptions.env = { DEBUG: "whatever", }; return launchOptions; });

ZachJW34

Changes look good! I reran CI and we have a few build failures, lmk if you need help fixing those up. A few are snapshot related, but we did recently add webkit support so looks like we need to make some tweaks there.

ryanjduffy · 2022-09-20T23:01:56Z

Changes look good! I reran CI and we have a few build failures, lmk if you need help fixing those up. A few are snapshot related, but we did recently add webkit support so looks like we need to make some tweaks there.

Looks like it. Fixing up now

lmiller1990

Pushed a commit ensuring the warning shows up correctly 👍

62ad6d3

lmiller1990 · 2022-09-21T07:54:08Z

Seems I broke something when I pushed a commit, reverting + fixing...

flotwig · 2022-09-21T13:25:11Z

Looks like the deprecated_spec snapshot needs to be updated, then this should be good to go:

cd system-tests
SNAPSHOT_UPDATE=1 yarn test deprecated_spec --browser electron

should do the trick.

lmiller1990 · 2022-09-22T02:44:54Z

Looks like a few more needed updating, I ran + pushed. 🤞

ZachJW34 · 2022-09-22T15:31:34Z

Looks like we need to tweak one more system-test using non-deprecated API - no warning. The snapshot needs to be browser specific, since args is not supported for Electron the snapshot will differ. Looks like the pattern is to add:

onRun: (exec, browser) => {
  if (browser === 'electron') {
    return exec({ originalTitle: `deprecated before:browser:launch args / using non-deprecated API - no warning - [electron]` })
  }

  return exec({ originalTitle: `deprecated before:browser:launch args / using non-deprecated API - no warning - [firefox,chromium]` })
},

I'd push but my local is a bit borked. Could you add those changes and run cd system-tests && SNAPSHOT_UPDATE=1 yarn test deprecated_spec

ZachJW34 · 2022-09-22T21:44:50Z

LGTM!

cypress-bot · 2022-09-27T16:55:18Z

Released in 10.9.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.9.0, please open a new issue.

nagash77 requested a review from emilyrohrbough August 31, 2022 18:54

emilyrohrbough reviewed Sep 1, 2022

View reviewed changes

packages/launcher/lib/browsers.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Sep 1, 2022

View reviewed changes

packages/types/src/browser.ts Outdated Show resolved Hide resolved

emilyrohrbough requested a review from flotwig September 1, 2022 21:33

nagash77 requested a review from lmiller1990 September 7, 2022 18:15

lmiller1990 reviewed Sep 8, 2022

View reviewed changes

emilyrohrbough approved these changes Sep 8, 2022

View reviewed changes

ryanjduffy force-pushed the add-browser-env branch 3 times, most recently from a29d202 to 7b637df Compare September 8, 2022 19:16

ryanjduffy mentioned this pull request Sep 8, 2022

Add environment variables to cypress browser launch replayio/replay-cli#86

Merged

flotwig suggested changes Sep 9, 2022

View reviewed changes

ryanjduffy mentioned this pull request Sep 10, 2022

Update browser launch options for env key cypress-io/cypress-documentation#4723

Closed

emilyrohrbough requested a review from flotwig September 12, 2022 16:03

ryanjduffy mentioned this pull request Sep 14, 2022

Update browser launch options for env key cypress-io/cypress-documentation#4730

Merged

lmiller1990 reviewed Sep 20, 2022

View reviewed changes

ZachJW34 reviewed Sep 20, 2022

View reviewed changes

ryanjduffy added 3 commits September 20, 2022 21:34

feat: Add support for optional env key to browser launch options

7d6e0da

combine env option and add warning

38a63d6

add default env to webkit launch options

ecbdfe1

ryanjduffy force-pushed the add-browser-env branch from 87e8fbe to ecbdfe1 Compare September 21, 2022 04:34

Update electron.ts

62ad6d3

lmiller1990 approved these changes Sep 21, 2022

View reviewed changes

lmiller1990 added 3 commits September 21, 2022 17:30

revert

1789849

fix tests

778b2a1

fix lint

788154b

flotwig approved these changes Sep 21, 2022

View reviewed changes

ryanjduffy and others added 3 commits September 21, 2022 11:44

update snapshots

25338aa

update tests

8300db1

snaps

743a7f3

ryanjduffy added 2 commits September 22, 2022 10:31

one more snapshot update

8ce893d

Merge branch 'develop' into add-browser-env

47149dd

ZachJW34 merged commit acc1050 into cypress-io:develop Sep 22, 2022

ryanjduffy deleted the add-browser-env branch September 22, 2022 22:37

emilyrohrbough mentioned this pull request Sep 26, 2022

10.9.0 Changelog cypress-io/cypress-documentation#4752

Merged

cypress-bot bot locked as resolved and limited conversation to collaborators Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for optional env key to browser launch options #23624

feat: Add support for optional env key to browser launch options #23624

ryanjduffy commented Aug 30, 2022 •

edited by flotwig

Loading

cypress-bot bot commented Aug 30, 2022

ryanjduffy commented Aug 30, 2022

emilyrohrbough commented Sep 1, 2022

emilyrohrbough Sep 1, 2022 •

edited

Loading

ryanjduffy Sep 1, 2022

ryanjduffy Sep 1, 2022

emilyrohrbough Sep 1, 2022

lmiller1990 Sep 8, 2022 •

edited

Loading

ryanjduffy Sep 8, 2022

lmiller1990 left a comment

ryanjduffy commented Sep 8, 2022

ryanjduffy commented Sep 8, 2022

ryanjduffy commented Sep 8, 2022

flotwig left a comment

flotwig Sep 9, 2022

ryanjduffy Sep 10, 2022

flotwig Sep 9, 2022

ryanjduffy Sep 10, 2022

ryanjduffy commented Sep 10, 2022

lmiller1990 commented Sep 11, 2022

lmiller1990 left a comment

lmiller1990 Sep 20, 2022

ryanjduffy Sep 20, 2022

ZachJW34 left a comment

ryanjduffy commented Sep 20, 2022

lmiller1990 left a comment •

edited

Loading

lmiller1990 commented Sep 21, 2022

flotwig commented Sep 21, 2022

lmiller1990 commented Sep 22, 2022

ZachJW34 commented Sep 22, 2022

ZachJW34 commented Sep 22, 2022

cypress-bot bot commented Sep 27, 2022

feat: Add support for optional env key to browser launch options #23624

feat: Add support for optional env key to browser launch options #23624

Conversation

ryanjduffy commented Aug 30, 2022 • edited by flotwig Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 30, 2022

ryanjduffy commented Aug 30, 2022

emilyrohrbough commented Sep 1, 2022

emilyrohrbough Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

ryanjduffy commented Sep 8, 2022

ryanjduffy commented Sep 8, 2022

ryanjduffy commented Sep 8, 2022

flotwig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanjduffy commented Sep 10, 2022

lmiller1990 commented Sep 11, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZachJW34 left a comment

Choose a reason for hiding this comment

ryanjduffy commented Sep 20, 2022

lmiller1990 left a comment • edited Loading

Choose a reason for hiding this comment

lmiller1990 commented Sep 21, 2022

flotwig commented Sep 21, 2022

lmiller1990 commented Sep 22, 2022

ZachJW34 commented Sep 22, 2022

ZachJW34 commented Sep 22, 2022

cypress-bot bot commented Sep 27, 2022

ryanjduffy commented Aug 30, 2022 •

edited by flotwig

Loading

emilyrohrbough Sep 1, 2022 •

edited

Loading

lmiller1990 Sep 8, 2022 •

edited

Loading

lmiller1990 left a comment •

edited

Loading