-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Automatically retry verify and run commands on Linux if suspect DISPLAY problem #4165
Conversation
it('expects solution to be a string', () => { | ||
const error = { | ||
description: 'description', | ||
solution: 42, |
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.
42 is never a good answer
ughh, what is going on with Win32 build
|
found that we can detect X11 problem using a flag
so need to switch this PR to enable logging during verify step |
the way to see this locally (via Docker)
var verify = require('./cli/lib/tasks/verify')
verify.start({force: true}) If the verify code uses
A nice way to iterate over the code (because the local source code folder has been mounted into Docker container) is to run this line, which goes around require cache
|
const issueUrl = util.getGitHubIssueUrl(4034) | ||
|
||
const message = stripIndent` | ||
DISPLAY environment variable is set to ${process.env.DISPLAY} on Linux |
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.
being verbose is good, hopefully, someone reads it and understand the problem, even if we work around it using our XVFB
uggh, if we enable Cypress logging during full
|
@@ -16,18 +27,54 @@ Cypress Version: 1.2.3 | |||
` | |||
|
|||
exports['errors individual has the following errors 1'] = [ |
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.
the names of the errors are now sorted before snapshotting
@@ -102,6 +108,39 @@ const smokeTestFailure = (smokeTestCommand, timedOut) => { | |||
} | |||
} | |||
|
|||
const invalidDisplayError = { |
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.
a complex error that has both the display error and the follow-up error
solution: '', | ||
solution: stripIndent` | ||
See discussion and possible solutions at | ||
${chalk.blue(util.getGitHubIssueUrl(1281))} |
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.
at least give the relevant issue url
// assuming that if there the solution is a function it will handle | ||
// error message and (optional previous error message) | ||
if (is.fn(obj.solution)) { | ||
const text = obj.solution(msg, prevMessage) |
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 error solution property is a function, let it form the full error text
if (os.platform() === 'linux' && process.env.DISPLAY) { | ||
// make sure we use the latest DISPLAY variable if any | ||
debug('passing DISPLAY', process.env.DISPLAY) | ||
options.env.DISPLAY = process.env.DISPLAY |
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.
otherwise a new value from XVFB will not take effect
// else pass it along! | ||
process.stderr.write(data) | ||
}) | ||
child.stderr && |
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.
just a little bit of reformatting
cli/lib/exec/spawn.js
Outdated
} | ||
|
||
return userFriendlySpawn() | ||
|
||
return userFriendlySpawn(os.platform() === 'linux') |
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 are on Linux, and there is a display problem, we should attempt handling it once
return spawn() | ||
.catch({ displayError: true, platform: 'linux' }, (e) => { |
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.
handle display error once
solution: 42, | ||
} | ||
|
||
return expect(formErrorText(error)).to.be.rejected |
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.
chai-as-promised is nice
@@ -33,6 +33,7 @@ | |||
"clean-deps": "npm run all clean-deps && rm -rf node_modules", | |||
"docker": "./scripts/run-docker-local.sh", | |||
"lint-js": "eslint --fix scripts/*.js packages/ts/*.js cli/*.js cli/**/*.js", | |||
"lint-changed": "git diff --name-only | grep '\\.js$' | xargs npx eslint --fix", |
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.
little utility to fix all changes JS files before committing
@@ -72,6 +72,10 @@ module.exports = { | |||
|
|||
debug("spawning %s with args", execPath, argv) | |||
|
|||
if debug.enabled | |||
# let's see everything Electron spits back | |||
argv.push("--enable-logging") |
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.
when running with DEBUG=cypress:electron
it would be nice to see everything
fn = (code) -> | ||
## juggle up the totalFailed since our outer | ||
## promise is expecting this object structure | ||
debug("electron finished with", code) | ||
resolve({totalFailed: code}) | ||
cypressElectron.open(".", require("./util/args").toArray(options), fn) | ||
electronFinished = Number(new Date()) |
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.
detect display or missing dependency problem by noticing how quickly Electron exits. If we suspect display or missing dependency problem then return a special exit code
cli/__snapshots__/errors_spec.js
Outdated
Cypress failed to start. | ||
|
||
First, we have tried to start Cypress using your DISPLAY settings | ||
but his the following problem: |
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.
typo
Did the same driver-integration test just fail twice? |
did it?
|
cypress run
if there is suspected display error (shows warning as well)cypress run
warning on Linux if DISPLAY is set before re-running with our XVFBNote: to see how it performs, spin a Docker container (instructions in a comment below) and