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

uncaughtException: fix recovery when current test is still running #4150

Merged
merged 4 commits into from Jan 23, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/runnable.js
Expand Up @@ -335,7 +335,7 @@ Runnable.prototype.run = function(fn) {
fn(err);
}

// for .resetTimeout()
// for .resetTimeout() and Runner#uncaught()
this.callback = done;

if (this.fn && typeof this.fn.call !== 'function') {
Expand Down
54 changes: 11 additions & 43 deletions lib/runner.js
Expand Up @@ -724,7 +724,6 @@ Runner.prototype.runSuite = function(suite, fn) {
var i = 0;
var self = this;
var total = this.grepTotal(suite);
var afterAllHookCalled = false;

debug('run suite %s', suite.fullTitle());

Expand Down Expand Up @@ -772,21 +771,13 @@ Runner.prototype.runSuite = function(suite, fn) {
self.suite = suite;
self.nextSuite = next;

if (afterAllHookCalled) {
fn(errSuite);
} else {
// mark that the afterAll block has been called once
// and so can be skipped if there is an error in it.
afterAllHookCalled = true;

// remove reference to test
delete self.test;
// remove reference to test
delete self.test;

self.hook(HOOK_TYPE_AFTER_ALL, function() {
self.emit(constants.EVENT_SUITE_END, suite);
fn(errSuite);
});
}
self.hook(HOOK_TYPE_AFTER_ALL, function() {
self.emit(constants.EVENT_SUITE_END, suite);
fn(errSuite);
});
}

this.nextSuite = next;
Expand Down Expand Up @@ -861,36 +852,13 @@ Runner.prototype.uncaught = function(err) {

// we cannot recover gracefully if a Runnable has already passed
// then fails asynchronously
var alreadyPassed = runnable.isPassed();
// this will change the state to "failed" regardless of the current value
this.fail(runnable, err);
if (!alreadyPassed) {
// recover from test
if (runnable.type === constants.EVENT_TEST_BEGIN) {
this.emit(constants.EVENT_TEST_END, runnable);
this.hookUp(HOOK_TYPE_AFTER_EACH, this.next);
return;
}
if (runnable.isPassed()) {
this.fail(runnable, err);
this.abort();
} else {
debug(runnable);

// recover from hooks
var errSuite = this.suite;

// XXX how about a less awful way to determine this?
// if hook failure is in afterEach block
if (runnable.fullTitle().indexOf('after each') > -1) {
return this.hookErr(err, errSuite, true);
}
// if hook failure is in beforeEach block
if (runnable.fullTitle().indexOf('before each') > -1) {
return this.hookErr(err, errSuite, false);
}
// if hook failure is in after or before blocks
return this.nextSuite(errSuite);
return runnable.callback(err);
}

// bail
this.abort();
};

/**
Expand Down
28 changes: 28 additions & 0 deletions test/integration/fixtures/uncaught/recover.fixture.js
@@ -0,0 +1,28 @@
'use strict';
const assert = require('assert');

describe('uncaught', function() {
var hookOrder = [];
it('throw delayed error', (done) => {
setTimeout(() => {
throw new Error('Whoops!');
}, 10)
setTimeout(done, 10);
});
it('should wait 15ms', (done) => {
setTimeout(done, 15);
});
it('test 3', () => { });

afterEach(function() {
hookOrder.push(this.currentTest.title);
});
after(function() {
hookOrder.push('after');
assert.deepEqual(
hookOrder,
['throw delayed error', 'should wait 15ms', 'test 3', 'after']
);
throw new Error('should get upto here and throw');
});
});
5 changes: 4 additions & 1 deletion test/integration/hook-err.spec.js
Expand Up @@ -194,7 +194,10 @@ describe('hook error handling', function() {
run('hooks/before-hook-async-error-tip.fixture.js', onlyErrorTitle())
);
it('should verify results', function() {
expect(lines, 'to equal', ['1) spec 2', '"before all" hook:']);
expect(lines, 'to equal', [
'1) spec 2',
'"before all" hook for "skipped":'
]);
});
});

Expand Down
29 changes: 28 additions & 1 deletion test/integration/uncaught.spec.js
Expand Up @@ -17,7 +17,7 @@ describe('uncaught exceptions', function() {

assert.strictEqual(
res.failures[0].fullTitle,
'uncaught "before each" hook'
'uncaught "before each" hook for "test"'
);
assert.strictEqual(res.code, 1);
done();
Expand Down Expand Up @@ -87,6 +87,33 @@ describe('uncaught exceptions', function() {
});
});

it('handles uncaught exceptions within open tests', function(done) {
run('uncaught/recover.fixture.js', args, function(err, res) {
if (err) {
return done(err);
}

expect(
res,
'to have failed with error',
'Whoops!',
'Whoops!', // JSON reporter does not show the second error message
'should get upto here and throw'
)
.and('to have passed test count', 2)
.and('to have failed test count', 3)
.and('to have passed test', 'should wait 15ms', 'test 3')
.and(
'to have failed test',
'throw delayed error',
'throw delayed error',
'"after all" hook for "test 3"'
);

done();
});
});

it('removes uncaught exceptions handlers correctly', function(done) {
run('uncaught/listeners.fixture.js', args, function(err, res) {
if (err) {
Expand Down