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
feat: add option to ignore chrome preferences #29447
base: develop
Are you sure you want to change the base?
Conversation
8 flaky tests on run #55512 ↗︎
Details:
e2e/origin/cookie_login.cy.ts • 1 flaky test • 5x-driver-firefox
|
@@ -501,13 +513,16 @@ export = { | |||
launchOptions.preferences = _mergeChromePreferences(preferences, launchOptions.preferences as ChromePreferences) | |||
} | |||
|
|||
const p = _disableRestorePagesPrompt(userDir) |
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.
Any reason this was pulled out of the promise array below?
@@ -501,13 +513,16 @@ export = { | |||
launchOptions.preferences = _mergeChromePreferences(preferences, launchOptions.preferences as ChromePreferences) | |||
} | |||
|
|||
const p = _disableRestorePagesPrompt(userDir) |
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.
If we keep this here, we'll want a more appropriately named variable.
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.
This was for debugging while running - I'll remove
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.
addressed in c209969
* this effectively prevents Cypress from writing to the Chrome preferences files. | ||
* See: _writeChromePreferences | ||
*/ | ||
if (process.env.IGNORE_CHROME_PREFERENCES) { |
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 wonder if this should be more specific, SKIP_RETRIEVING_CHROME_PREFERENCES
or SKIP_READING_CHROME_PREFERENCE_PATHS
if (process.env.IGNORE_CHROME_PREFERENCES) { | ||
debug('ignoring chrome preferences...') | ||
|
||
return Bluebird.resolve(_.mapValues(CHROME_PREFERENCE_PATHS, () => ({}))) |
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.
What if only one of the CHROME_PREFERENCE_PATHS
is encrypted, would we want to read from the other paths?
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 don't think so - this makes more sense as an all-or-nothing thing. I'm curious if we should prevent writing preferences as well with this env var, but that may cause more confusion (or should be a separate env var). As is, if you set this env, and then provide preferences via config, it will still attempt to write preferences.
|
||
return Bluebird.resolve(_.mapValues(CHROME_PREFERENCE_PATHS, () => ({}))) | ||
} | ||
|
||
debug('reading chrome preferences... %o', { userDir, CHROME_PREFERENCE_PATHS }) | ||
|
||
return Bluebird.props(_.mapValues(CHROME_PREFERENCE_PATHS, (prefPath) => { |
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.
We are already catching the ENOENT
error below and returning an empty object. Is there an error thrown when reading an encrypted path? If so, can we just add it to below and remove the need of the new environment variable?
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.
It might be the write logic combined with the lockfile removal, on relaunch, that throws the error, not the read?
Co-authored-by: Matt Schile <mschile@cypress.io>
@cacieprins to make some updates here based on our convo - to ignore writing and reading chrome prefs |
Additional details
Adds an
IGNORE_CHROME_PREFERENCES
option to ignore Chrome preferences paths.Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
? Add note about how to set IGNORE_CHROME_PREFERENCES env var cypress-documentation#5822type definitions
?