From 2b7c97db404980e2aaa9c00fdef51773bad3c271 Mon Sep 17 00:00:00 2001 From: Friedel Ziegelmayer Date: Thu, 24 Aug 2017 23:07:39 +0200 Subject: [PATCH] Handle async errors (#4016) * wip: start handling async errors Ref https://github.com/facebook/jest/issues/2059 * Implement basic cancelation * Improve cancelation * lint and flow happy, more tests * snapshot updates * happy tests * add tests for before hooks * log errors if they escape us * use p-cancelable * improve based on CR --- .../__tests__/jasmine_async.test.js | 26 ++++++++---- .../__tests__/promise_before_all.test.js | 15 +++++++ .../__tests__/promise_before_each.test.js | 16 +++++++ .../__tests__/promise_it.test.js | 40 ++++++++++++++++++ packages/jest-jasmine2/package.json | 3 +- .../src/__tests__/queue_runner.test.js | 26 ++++++++++++ packages/jest-jasmine2/src/jasmine/Env.js | 42 +++++++++++++++++-- packages/jest-jasmine2/src/jasmine/Spec.js | 12 +++++- packages/jest-jasmine2/src/jasmine_async.js | 4 +- packages/jest-jasmine2/src/queue_runner.js | 28 +++++++++++-- yarn.lock | 4 ++ 11 files changed, 195 insertions(+), 21 deletions(-) diff --git a/integration_tests/__tests__/jasmine_async.test.js b/integration_tests/__tests__/jasmine_async.test.js index 4ae55031d0f7..ac63db8f5edc 100644 --- a/integration_tests/__tests__/jasmine_async.test.js +++ b/integration_tests/__tests__/jasmine_async.test.js @@ -19,14 +19,16 @@ describe('async jasmine', () => { ]); const json = result.json; - expect(json.numTotalTests).toBe(2); + expect(json.numTotalTests).toBe(4); expect(json.numPassedTests).toBe(1); - expect(json.numFailedTests).toBe(1); + expect(json.numFailedTests).toBe(3); expect(json.numPendingTests).toBe(0); const {message} = json.testResults[0]; expect(message).toMatch('with failing timeout'); expect(message).toMatch('Async callback was not invoked within timeout'); + expect(message).toMatch('done - with error thrown'); + expect(message).toMatch('done - with error called back'); }); it('works with beforeEach', () => { @@ -35,11 +37,14 @@ describe('async jasmine', () => { ]); const json = result.json; - expect(json.numTotalTests).toBe(1); + expect(json.numTotalTests).toBe(3); expect(json.numPassedTests).toBe(1); - expect(json.numFailedTests).toBe(0); + expect(json.numFailedTests).toBe(2); expect(json.numPendingTests).toBe(0); - expect(json.testResults[0].message).toBe(''); + + const {message} = json.testResults[0]; + expect(message).toMatch('done - with error thrown'); + expect(message).toMatch('done - with error called back'); }); it('works with afterAll', () => { @@ -104,12 +109,17 @@ describe('async jasmine', () => { const json = result.json; const message = json.testResults[0].message; - expect(json.numTotalTests).toBe(9); - expect(json.numPassedTests).toBe(5); - expect(json.numFailedTests).toBe(4); + expect(json.numTotalTests).toBe(16); + expect(json.numPassedTests).toBe(6); + expect(json.numFailedTests).toBe(9); expect(message).toMatch('fails if promise is rejected'); expect(message).toMatch('works with done.fail'); + expect(message).toMatch('works with done(error)'); + expect(message).toMatch('fails if failed expectation with done'); + expect(message).toMatch('fails if failed expectation with done - async'); + expect(message).toMatch('fails with thrown error with done - sync'); + expect(message).toMatch('fails with thrown error with done - async'); expect(message).toMatch('fails a sync test'); expect(message).toMatch('fails if a custom timeout is exceeded'); }); diff --git a/integration_tests/jasmine_async/__tests__/promise_before_all.test.js b/integration_tests/jasmine_async/__tests__/promise_before_all.test.js index 931facf8240d..f9b659432818 100644 --- a/integration_tests/jasmine_async/__tests__/promise_before_all.test.js +++ b/integration_tests/jasmine_async/__tests__/promise_before_all.test.js @@ -34,4 +34,19 @@ describe('promise beforeAll', () => { it('fails', () => {}); }); + + describe('done - with error thrown', () => { + beforeAll(done => { + throw new Error('fail'); + done(); // eslint-disable-line + }); + it('fails', () => {}); + }); + + describe('done - with error called back', () => { + beforeAll(done => { + done(new Error('fail')); + }); + it('fails', () => {}); + }); }); diff --git a/integration_tests/jasmine_async/__tests__/promise_before_each.test.js b/integration_tests/jasmine_async/__tests__/promise_before_each.test.js index a3e59dd82bfa..8f349309df31 100644 --- a/integration_tests/jasmine_async/__tests__/promise_before_each.test.js +++ b/integration_tests/jasmine_async/__tests__/promise_before_each.test.js @@ -21,4 +21,20 @@ describe('promise beforeEach', () => { it('runs tests after beforeEach asynchronously completes', () => { expect(this.flag).toBe(1); }); + + // failing tests + describe('done - with error thrown', () => { + beforeEach(done => { + throw new Error('fail'); + done(); // eslint-disable-line + }); + it('fails', () => {}); + }); + + describe('done - with error called back', () => { + beforeEach(done => { + done(new Error('fail')); + }); + it('fails', () => {}); + }); }); diff --git a/integration_tests/jasmine_async/__tests__/promise_it.test.js b/integration_tests/jasmine_async/__tests__/promise_it.test.js index 8b67dd95be37..5b9458f3fbbb 100644 --- a/integration_tests/jasmine_async/__tests__/promise_it.test.js +++ b/integration_tests/jasmine_async/__tests__/promise_it.test.js @@ -26,6 +26,10 @@ describe('promise it', () => { done(); }); + it('works with async done', done => { + setTimeout(done, 1); + }); + it('is bound to context object', () => { return new Promise(resolve => { if (this.someContextValue !== 'value') { @@ -46,6 +50,42 @@ describe('promise it', () => { done.fail(new Error('done.fail was called')); }); + it('works with done(error)', done => { + done(new Error('done was called with error')); + }); + + it('fails if failed expectation with done', done => { + expect(true).toEqual(false); + done(); + }); + + it('fails if failed expectation with done - async', done => { + setTimeout(() => { + expect(true).toEqual(false); + done(); + }, 1); + }); + + it('fails with thrown error with done - sync', done => { + throw new Error('sync fail'); + done(); // eslint-disable-line + }); + + it('fails with thrown error with done - async', done => { + setTimeout(() => { + throw new Error('async fail'); + done(); // eslint-disable-line + }, 1); + }); + + // I wish it was possible to catch this but I do not see a way. + // Currently both jest and mocha will pass this test. + it.skip('fails with thrown error - async', () => { + setTimeout(() => { + throw new Error('async fail - no done'); + }, 1); + }); + it('fails a sync test', () => { expect('sync').toBe('failed'); }); diff --git a/packages/jest-jasmine2/package.json b/packages/jest-jasmine2/package.json index fa44c4e8fbd5..6902c9f1153d 100644 --- a/packages/jest-jasmine2/package.json +++ b/packages/jest-jasmine2/package.json @@ -14,7 +14,8 @@ "jest-diff": "^20.0.3", "jest-matcher-utils": "^20.0.3", "jest-message-util": "^20.0.3", - "jest-snapshot": "^20.0.3" + "jest-snapshot": "^20.0.3", + "p-cancelable": "^0.3.0" }, "devDependencies": { "jest-runtime": "^20.0.4" diff --git a/packages/jest-jasmine2/src/__tests__/queue_runner.test.js b/packages/jest-jasmine2/src/__tests__/queue_runner.test.js index 897365aced16..8e2a5f717863 100644 --- a/packages/jest-jasmine2/src/__tests__/queue_runner.test.js +++ b/packages/jest-jasmine2/src/__tests__/queue_runner.test.js @@ -130,4 +130,30 @@ describe('queueRunner', () => { expect(options.fail).toHaveBeenCalledWith('miserably', 'failed'); }); + + it('calls `fail` when done(error) is invoked', async () => { + const error = new Error('I am an error'); + const fail = jest.fn(); + const fnOne = jest.fn(next => next(error)); + const fnTwo = jest.fn(next => next()); + const options = { + clearTimeout, + fail, + onException: () => {}, + queueableFns: [ + { + fn: fnOne, + }, + { + fn: fnTwo, + }, + ], + setTimeout, + }; + await queueRunner(options); + expect(fnOne).toHaveBeenCalled(); + expect(fail).toHaveBeenCalledWith(error); + // Even if `fail` is called, the queue keeps running. + expect(fnTwo).toHaveBeenCalled(); + }); }); diff --git a/packages/jest-jasmine2/src/jasmine/Env.js b/packages/jest-jasmine2/src/jasmine/Env.js index abd2b3bdd30f..3b885618b8a6 100644 --- a/packages/jest-jasmine2/src/jasmine/Env.js +++ b/packages/jest-jasmine2/src/jasmine/Env.js @@ -161,7 +161,7 @@ module.exports = function(j$) { return seed; }; - async function queueRunnerFactory(options) { + function queueRunnerFactory(options) { options.clearTimeout = realClearTimeout; options.fail = self.fail; options.setTimeout = realSetTimeout; @@ -187,9 +187,31 @@ module.exports = function(j$) { } } - reporter.jasmineStarted({ - totalSpecsDefined, - }); + const uncaught = err => { + if (currentSpec) { + currentSpec.onException(err); + currentSpec.cancel(); + } else { + console.error('Unhandled error'); + console.error(err.stack); + } + }; + + // Need to ensure we are the only ones handling these exceptions. + const oldListenersException = process + .listeners('uncaughtException') + .slice(); + const oldListenersRejection = process + .listeners('unhandledRejection') + .slice(); + + process.removeAllListeners('uncaughtException'); + process.removeAllListeners('unhandledRejection'); + + process.on('uncaughtException', uncaught); + process.on('unhandledRejection', uncaught); + + reporter.jasmineStarted({totalSpecsDefined}); currentlyExecutingSuites.push(topSuite); @@ -215,6 +237,18 @@ module.exports = function(j$) { reporter.jasmineDone({ failedExpectations: topSuite.result.failedExpectations, }); + + process.removeListener('uncaughtException', uncaught); + process.removeListener('unhandledRejection', uncaught); + + // restore previous exception handlers + oldListenersException.forEach(listener => { + process.on('uncaughtException', listener); + }); + + oldListenersRejection.forEach(listener => { + process.on('unhandledRejection', listener); + }); }; this.addReporter = function(reporterToAdd) { diff --git a/packages/jest-jasmine2/src/jasmine/Spec.js b/packages/jest-jasmine2/src/jasmine/Spec.js index a0401c14da82..9b7b92599268 100644 --- a/packages/jest-jasmine2/src/jasmine/Spec.js +++ b/packages/jest-jasmine2/src/jasmine/Spec.js @@ -96,13 +96,15 @@ Spec.prototype.execute = function(onComplete, enabled) { const fns = this.beforeAndAfterFns(); const allFns = fns.befores.concat(this.queueableFn).concat(fns.afters); - this.queueRunnerFactory({ + this.currentRun = this.queueRunnerFactory({ queueableFns: allFns, onException() { self.onException.apply(self, arguments); }, userContext: this.userContext(), - }).then(() => complete(true)); + }); + + this.currentRun.then(() => complete(true)); function complete(enabledAgain) { self.result.status = self.status(enabledAgain); @@ -114,6 +116,12 @@ Spec.prototype.execute = function(onComplete, enabled) { } }; +Spec.prototype.cancel = function cancel() { + if (this.currentRun) { + this.currentRun.cancel(); + } +}; + Spec.prototype.onException = function onException(error) { if (Spec.isPendingSpecException(error)) { this.pend(extractCustomPendingMessage(error)); diff --git a/packages/jest-jasmine2/src/jasmine_async.js b/packages/jest-jasmine2/src/jasmine_async.js index 2d40c7a087c5..ef78b9cf8773 100644 --- a/packages/jest-jasmine2/src/jasmine_async.js +++ b/packages/jest-jasmine2/src/jasmine_async.js @@ -38,7 +38,7 @@ function promisifyLifeCycleFunction(originalFn, env) { const returnValue = fn.call({}); if (isPromise(returnValue)) { - returnValue.then(done, done.fail); + returnValue.then(done.bind(null, null), done.fail); } else { done(); } @@ -68,7 +68,7 @@ function promisifyIt(originalFn, env) { const returnValue = fn.call({}); if (isPromise(returnValue)) { - returnValue.then(done, done.fail); + returnValue.then(done.bind(null, null), done.fail); } else if (returnValue === undefined) { done(); } else { diff --git a/packages/jest-jasmine2/src/queue_runner.js b/packages/jest-jasmine2/src/queue_runner.js index 57309120300a..148ec7ac673a 100644 --- a/packages/jest-jasmine2/src/queue_runner.js +++ b/packages/jest-jasmine2/src/queue_runner.js @@ -8,6 +8,7 @@ * @flow */ +import PCancelable from 'p-cancelable'; import pTimeout from './p_timeout'; type Options = { @@ -24,10 +25,20 @@ type QueueableFn = { timeout?: () => number, }; -async function queueRunner(options: Options) { +function queueRunner(options: Options) { + const token = new PCancelable((onCancel, resolve) => { + onCancel(resolve); + }); + const mapper = ({fn, timeout}) => { - const promise = new Promise(resolve => { - const next = () => resolve(); + let promise = new Promise(resolve => { + const next = function(err) { + if (err) { + options.fail.apply(null, arguments); + } + resolve(); + }; + next.fail = function() { options.fail.apply(null, arguments); resolve(); @@ -39,6 +50,9 @@ async function queueRunner(options: Options) { resolve(); } }); + + promise = Promise.race([promise, token]); + if (!timeout) { return promise; } @@ -57,10 +71,16 @@ async function queueRunner(options: Options) { ); }; - return options.queueableFns.reduce( + const result = options.queueableFns.reduce( (promise, fn) => promise.then(() => mapper(fn)), Promise.resolve(), ); + + return { + cancel: token.cancel.bind(token), + catch: result.catch.bind(result), + then: result.then.bind(result), + }; } module.exports = queueRunner; diff --git a/yarn.lock b/yarn.lock index 533fa58a944f..861eb056b080 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4679,6 +4679,10 @@ osenv@^0.1.4: os-homedir "^1.0.0" os-tmpdir "^1.0.0" +p-cancelable@^0.3.0: + version "0.3.0" + resolved "https://registry.yarnpkg.com/p-cancelable/-/p-cancelable-0.3.0.tgz#b9e123800bcebb7ac13a479be195b507b98d30fa" + p-finally@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/p-finally/-/p-finally-1.0.0.tgz#3fbcfb15b899a44123b34b6dcc18b724336a2cae"