-
Couldn't load subscription status.
- Fork 3.4k
chore: rework electron switches to avoid unknown side effects #32812
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
Conversation
cypress
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
chore/rework-electron-switches
|
| Run status |
|
| Run duration | 19m 36s |
| Commit |
|
| Committer | Bill Glesias |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
12
|
|
|
1102
|
|
|
4
|
|
|
26755
|
| View all changes introduced in this branch ↗︎ | |
Warning
Partial Report: The results for the Application Quality reports may be incomplete.
UI Coverage
45.61%
|
|
|---|---|
|
|
187
|
|
|
161
|
Accessibility
97.98%
|
|
|---|---|
|
|
4 critical
8 serious
2 moderate
2 minor
|
|
|
101
|
0b6c2cf to
36274e3
Compare
a4b281e to
c712e76
Compare
| // thus we don't have to worry about casting | ||
| // --foo=false for example will be "--foo", "false" | ||
| if (value.length) { | ||
| let joinedValues = value.join('=') |
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.
| let joinedValues = value.join('=') | |
| const joinedValues = value.join('=').replace(/^(?:['"](.*)['"]|\u0022(.*)\u0027)$/, '$1$2') |
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 56ab152. Thank you for the suggestion!
how bad does this fail? fix server tests
56ab152 to
ff1543a
Compare
ff1543a to
f60cf64
Compare
Additional details
refactors the
environment.jsfile to handle distinct operations. This file is now:append_electron_switcheswhich now exports a function that takes the electron app as a paramter and appends switches to it by referenceconfigureLongStackTracesinenvironment.tswhich just configures long stack traces on errors andbluebirdcalculateCypressInternalEnvwhich calculates the value ofCYPRESS_INTERNAL_ENV, which is now set instart-cypressThe reason for the move is the
environment.jsfile was acting as a global drop in that had side effects as opposed to functions or other types of patterns. It makes understanding what is used where and why unclear. For instance, the electron switches silently fail in the parent process, but work in the child electron process. This is why this is now moved tostart-cypresswhen we check if electron is running, and if so, get the electron app and pass it in to append switches by reference. This makes the intent much clearer.We don't have unit tests for
start-cypress(AFAIK?) but it is usually very clear when something is not behaving as it should (i.e., CI system tests start failing in large numbers.start-cypressis responsible for setting theCYPRESS_INTERNAL_ENVenv,CYPRESSenv, and appending the electron switches correctly. Tests are also added/refactored with this changeSteps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?Note
Extracts Electron switch logic and env/stack-trace configuration from a side-effectful module into explicit functions wired via start-cypress, with updated tests.
lib/environment.jsinto:lib/append_electron_switches.ts: appends default Chromium flags, sets--gtk-version=3, disables HW acceleration on Linux, and parsesELECTRON_EXTRA_LAUNCH_ARGS.lib/environment.ts: addsconfigureLongStackTraces(env)andcalculateCypressInternalEnv().start-cypress.jsto:process.env.CYPRESS_INTERNAL_ENVviacalculateCypressInternalEnv()and callconfigureLongStackTraces(); setprocess.env.CYPRESS = 'true'.appendElectronSwitches(app)before startup.require('./environment')usage (e.g., fromlib/cypress.ts).test/unit/append_electron_switches_spec.tsandtest/unit/environment_spec.tsfor new modules.test/spec_helper.jsto useconfigureLongStackTraces(); remove oldenvironment_spec.js.Written by Cursor Bugbot for commit 1b13a55. This will update automatically on new commits. Configure here.