Add more informative error messages when cypress verify hangs #3807
Conversation
|
|
cli/lib/tasks/verify.js
Outdated
| @@ -65,7 +65,7 @@ const runSmokeTest = (binaryDir) => { | |||
|
|
|||
| debug('smoke test command:', smokeTestCommand) | |||
|
|
|||
| return Promise.resolve(util.exec(cypressExecPath, args)) | |||
| return Promise.resolve(util.exec(cypressExecPath, args, { stdio: ['pipe', 'pipe', process.stderr] })) | |||
There was a problem hiding this comment.
One specific concern I have but am unsure how to test is, the onSmokeTestError callback contains:
return throwFormErrorText(errors.missingDependency)(err.stderr || err.message)And possibly the suggested change is hosing its ability to hook into err.stderr, for those cases where the smoke test exits with useful info on stderr.
There was a problem hiding this comment.
Just pushed an update that addresses this by preserving stderr in the child as well
| function smokeTestExec () { | ||
| let child = util.exec(cypressExecPath, args) | ||
|
|
||
| if (child.stderr) { |
There was a problem hiding this comment.
Looking at https://github.com/sindresorhus/execa/tree/v0.10.0, no idea why sometimes stderr isn't there, but the unit tests fail without this guard 🤷♂️ .
|
Status: this is ready for review. There is a failing integration test, but it doesn't seem related to the code change (AFAICT) |
There was a problem hiding this comment.
child can safely be declared with const here as a best practice.
I also kinda like returning a new Promise and calling it's resolve to capture the functionality inside smokeTestExec, if you're creating a function anyway it feels more explicit. But it's neither here nor there.
Great work though. Not having this err output has been a pain point for our team for a while now 👌
|
Hey @stuartsan, we haven't forgotten about you. We will make sure to review this before our next release. 😄 |
|
Thanks @jennifer-shehane, all good, literally zero rush on my end, :) |
|
While not actually testing this PR, it did give the crucial tip to check for permissions on |
|
I think adding a timeout to the smoke test process is a better solution here, we can then print you a proper error message and not hang the process forever. If the process does not exit within 10s, we'll kill it and print its PR for adding the timeout here #4080 |
address #819
The purpose of this change is to add better visibility when
cypress runhangs indefinitely on the verification step.When running the smoke test, the child process's stderr contains useful information but it is currently being swallowed when the process doesn't exit. The proposed change writes the child's stderr to the parent's.
The result is that instead of seeing this:
With no further output, I now see this:
And then, after googling and fixing my specific underlying problem (Electron's default
appDatapath,~/.config, was read-only), if I docypress runagain it works fine.This doesn't address the underlying issue of why
cypress --smoke-testseems to be not exiting when this kind of failure happens, but it at least adds visibility so folks can self-serve and fix their specific problem more easily.See also: #819 (comment).
Validation
To validate this, I've made the change manually to the cypress CLI source within my own project. I cloned the cypress repo and fiddled around but couldn't figure out a good way to test it within here.
This doesn't seem like it lends itself well to an automated test (cause it would be hanging on purpose)...what I wanted to do manually to reproduce the problem case is something like:
And then (on linux) set
XDG_CONFIG_HOME(per this) to tell Electron to use that as the appData path, and docypress run(or maybecypress verifyfor a smaller case).Anyway, I read the contributing docs but couldn't figure out how to do that within this repo. I'm hoping that between feedback from the CI tests, and maintainers, I can get an idea if this change is legit and if there's anything else I should do to validate.