Skip to content
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

catch child process killed with a signal #5810

Merged
merged 6 commits into from
Nov 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions cli/__snapshots__/errors_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Cypress Version: 1.2.3
exports['errors individual has the following errors 1'] = [
"CYPRESS_RUN_BINARY",
"binaryNotExecutable",
"childProcessKilled",
"failedDownload",
"failedUnzip",
"invalidCacheDirectory",
Expand Down Expand Up @@ -71,3 +72,27 @@ If you are using Docker, we provide containers with all required dependencies in
Platform: test platform (Foo-OsVersion)
Cypress Version: 1.2.3
`

exports['child kill error object'] = {
"description": "The Test Runner unexpectedly exited via a exit event with signal SIGKILL",
"solution": "Please search Cypress documentation for possible solutions:\n\n https://on.cypress.io\n\nCheck if there is a GitHub issue describing this crash:\n\n https://github.com/cypress-io/cypress/issues\n\nConsider opening a new issue."
}

exports['Error message'] = `
The Test Runner unexpectedly exited via a exit event with signal SIGKILL

Please search Cypress documentation for possible solutions:

https://on.cypress.io

Check if there is a GitHub issue describing this crash:

https://github.com/cypress-io/cypress/issues

Consider opening a new issue.

----------

Platform: test platform (Foo-OsVersion)
Cypress Version: 1.2.3
`
18 changes: 18 additions & 0 deletions cli/__snapshots__/spawn_spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
exports['lib/exec/spawn .start detects kill signal exits with error on SIGKILL 1'] = `
The Test Runner unexpectedly exited via a exit event with signal SIGKILL

Please search Cypress documentation for possible solutions:

https://on.cypress.io

Check if there is a GitHub issue describing this crash:

https://github.com/cypress-io/cypress/issues

Consider opening a new issue.

----------

Platform: darwin (Foo-OsVersion)
Cypress Version: 0.0.0
`
59 changes: 49 additions & 10 deletions cli/lib/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,20 +168,21 @@ const versionMismatch = {
solution: 'Install Cypress and verify app again',
}

const unexpected = {
description:
'An unexpected error occurred while verifying the Cypress executable.',
solution: stripIndent`
Please search Cypress documentation for possible solutions:
const solutionUnknown = stripIndent`
Please search Cypress documentation for possible solutions:

${chalk.blue(docsUrl)}
${chalk.blue(docsUrl)}

Check if there is a GitHub issue describing this crash:
Check if there is a GitHub issue describing this crash:

${chalk.blue(util.issuesUrl)}
${chalk.blue(util.issuesUrl)}

Consider opening a new issue.
`,
Consider opening a new issue.
`
const unexpected = {
description:
'An unexpected error occurred while verifying the Cypress executable.',
solution: solutionUnknown,
}

const invalidCypressEnv = {
Expand All @@ -191,6 +192,20 @@ const invalidCypressEnv = {
exitCode: 11,
}

/**
* This error happens when CLI detects that the child Test Runner process
* was killed with a signal, like SIGBUS
* @see https://github.com/cypress-io/cypress/issues/5808
* @param {'close'|'event'} eventName Child close event name
* @param {string} signal Signal that closed the child process, like "SIGBUS"
*/
const childProcessKilled = (eventName, signal) => {
return {
description: `The Test Runner unexpectedly exited via a ${chalk.cyan(eventName)} event with signal ${chalk.cyan(signal)}`,
solution: solutionUnknown,
}
}

const removed = {
CYPRESS_BINARY_VERSION: {
description: stripIndent`
Expand Down Expand Up @@ -240,6 +255,28 @@ function addPlatformInformation (info) {
})
}

/**
* Given an error object (see the errors above), forms error message text with details,
* then resolves with Error instance you can throw or reject with.
* @param {object} errorObject
* @returns {Promise<Error>} resolves with an Error
* @example
```js
// inside a Promise with "resolve" and "reject"
const errorObject = childProcessKilled('exit', 'SIGKILL')
return getError(errorObject).then(reject)
```
*/
function getError (errorObject) {
return formErrorText(errorObject).then((errorMessage) => {
const err = new Error(errorMessage)

err.known = true

return err
})
}

/**
* Forms nice error message with error and platform information,
* and if possible a way to solve it. Resolves with a string.
Expand Down Expand Up @@ -355,6 +392,7 @@ module.exports = {
// formError,
formErrorText,
throwFormErrorText,
getError,
hr,
errors: {
nonZeroExitCodeXvfb,
Expand All @@ -373,5 +411,6 @@ module.exports = {
removed,
CYPRESS_RUN_BINARY,
smokeTestFailure,
childProcessKilled,
},
}
13 changes: 11 additions & 2 deletions cli/lib/exec/spawn.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const util = require('../util')
const state = require('../tasks/state')
const xvfb = require('./xvfb')
const verify = require('../tasks/verify')
const { throwFormErrorText, errors } = require('../errors')
const errors = require('../errors')

const isXlibOrLibudevRe = /^(?:Xlib|libudev)/
const isHighSierraWarningRe = /\*\*\* WARNING/
Expand Down Expand Up @@ -140,6 +140,13 @@ module.exports = {
function resolveOn (event) {
return function (code, signal) {
debug('child event fired %o', { event, code, signal })

if (code === null) {
const errorObject = errors.errors.childProcessKilled(event, signal)

return errors.getError(errorObject).then(reject)
}

resolve(code)
}
}
Expand Down Expand Up @@ -251,7 +258,9 @@ module.exports = {

return code
})
.catch(throwFormErrorText(errors.unexpected))
// we can format and handle an error message from the code above
// prevent wrapping error again by using "known: undefined" filter
.catch({ known: undefined }, errors.throwFormErrorText(errors.errors.unexpected))
}

if (needsXvfb) {
Expand Down
16 changes: 15 additions & 1 deletion cli/test/lib/errors_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ require('../spec_helper')

const os = require('os')
const snapshot = require('../support/snapshot')
const { errors, formErrorText } = require(`${lib}/errors`)
const { errors, getError, formErrorText } = require(`${lib}/errors`)
const util = require(`${lib}/util`)

describe('errors', function () {
Expand All @@ -19,6 +19,20 @@ describe('errors', function () {
})
})

context('getError', () => {
it('forms full message and creates Error object', () => {
const errObject = errors.childProcessKilled('exit', 'SIGKILL')

snapshot('child kill error object', errObject)

return getError(errObject).then((e) => {
expect(e).to.be.an('Error')
expect(e).to.have.property('known', true)
snapshot('Error message', e.message)
})
})
})

context('.errors.formErrorText', function () {
it('returns fully formed text message', () => {
expect(missingXvfb).to.be.an('object')
Expand Down
17 changes: 17 additions & 0 deletions cli/test/lib/exec/spawn_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ const tty = require('tty')
const path = require('path')
const EE = require('events')
const mockedEnv = require('mocked-env')
const debug = require('debug')('test')

const state = require(`${lib}/tasks/state`)
const xvfb = require(`${lib}/exec/xvfb`)
const spawn = require(`${lib}/exec/spawn`)
const verify = require(`${lib}/tasks/verify`)
const util = require(`${lib}/util.js`)
const expect = require('chai').expect
const snapshot = require('../../support/snapshot')

const cwd = process.cwd()

Expand Down Expand Up @@ -171,6 +173,20 @@ describe('lib/exec/spawn', function () {
})
})

context('detects kill signal', function () {
it('exits with error on SIGKILL', function () {
this.spawnedProcess.on.withArgs('exit').yieldsAsync(null, 'SIGKILL')

return spawn.start('--foo')
.then(() => {
throw new Error('should have hit error handler but did not')
}, (e) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the .catch() pattern is highly preferred over passing two arguments to .then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, in this case, catch would let false positive slide (we really want NOT to resolve). Ideally we would plug in https://www.chaijs.com/plugins/chai-as-promised/

return spawn.start('--foo').should.eventually.rejectWith(...)

debug('error message', e.message)
snapshot(e.message)
})
})
})

Comment on lines +176 to +189
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bahmutov I think it would be worth it to add an e2e test for this, since it involves multiple processes, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be tricky though, maybe not worth the time investment unless we have issues with this PR going forward

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not so sure e2e would be worth it

  • we don't have e2e tests for CLI right now
  • Node side (that one that delivers code: null, signal: <name> is pretty solid :)
  • we have only changed our handling, which is covered by the tests
  • finally, so far this has affected only very small number of our users I believe, otherwise we would hear about it again and again

it('does not start xvfb when its not needed', function () {
this.spawnedProcess.on.withArgs('close').yieldsAsync(0)

Expand Down Expand Up @@ -246,6 +262,7 @@ describe('lib/exec/spawn', function () {
.then(() => {
throw new Error('should have hit error handler but did not')
}, (e) => {
debug('error message', e.message)
expect(e.message).to.include(msg)
})
})
Expand Down