Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

Commit

Permalink
fix memory leak when run in v8; closes mochajs#3119
Browse files Browse the repository at this point in the history
- this means "multiple calls to done" will once again fail to produce a stack trace in the case that those multiple calls happen in separate tasks.
- however, if the `done()` is called with an error, Mocha will emit that error, but also a note about multiple calls
- some reformatting, evidently
- removed some pointless checks of Mocha's exit code from "multiple done" integration tests

Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
  • Loading branch information
boneskull committed Jan 29, 2018
1 parent 908e2f0 commit d1de4b9
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 16 deletions.
12 changes: 9 additions & 3 deletions lib/runnable.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ function Runnable (title, fn) {
this._slow = 75;
this._enableTimeouts = true;
this.timedOut = false;
this._trace = new Error('done() called multiple times');
this._retries = -1;
this._currentRetry = 0;
this.pending = false;
Expand Down Expand Up @@ -278,7 +277,13 @@ Runnable.prototype.run = function (fn) {
return;
}
emitted = true;
self.emit('error', err || new Error('done() called multiple times; stacktrace may be inaccurate'));
var msg = 'done() called multiple times';
if (err && err.message) {
err.message += " (and Mocha's " + msg + ')';
self.emit('error', err);
} else {
self.emit('error', new Error(msg));
}
}

// finished
Expand All @@ -287,8 +292,9 @@ Runnable.prototype.run = function (fn) {
if (self.timedOut) {
return;
}

if (finished) {
return multiple(err || self._trace);
return multiple(err);
}

self.clearTimeout();
Expand Down
8 changes: 8 additions & 0 deletions test/integration/fixtures/multiple-done-with-error.fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

it('should fail in a test-case', function (done) {
process.nextTick(function () {
done();
done(new Error('second error'));
});
});
41 changes: 29 additions & 12 deletions test/integration/multiple-done.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,35 @@ describe('multiple calls to done()', function () {
});

it('results in failures', function () {
assert.equal(res.stats.pending, 0);
assert.equal(res.stats.passes, 1);
assert.equal(res.stats.failures, 1);
assert.equal(res.code, 1);
assert.equal(res.stats.pending, 0, 'wrong "pending" count');
assert.equal(res.stats.passes, 1, 'wrong "passes" count');
assert.equal(res.stats.failures, 1, 'wrong "failures" count');
});

it('throws a descriptive error', function () {
assert.equal(res.failures[0].err.message,
'done() called multiple times');
assert.equal(res.failures[0].err.message, 'done() called multiple times');
});
});

describe('with error passed on second call', function () {
before(function (done) {
run('multiple-done-with-error.fixture.js', args, function (err, result) {
res = result;
done(err);
});
});

it('results in failures', function () {
assert.equal(res.stats.pending, 0, 'wrong "pending" count');
assert.equal(res.stats.passes, 1, 'wrong "passes" count');
assert.equal(res.stats.failures, 1, 'wrong "failures" count');
});

it('should throw a descriptive error', function () {
assert.equal(
res.failures[0].err.message,
"second error (and Mocha's done() called multiple times)"
);
});
});

Expand All @@ -44,8 +64,7 @@ describe('multiple calls to done()', function () {

it('correctly attributes the error', function () {
assert.equal(res.failures[0].fullTitle, 'suite test1');
assert.equal(res.failures[0].err.message,
'done() called multiple times');
assert.equal(res.failures[0].err.message, 'done() called multiple times');
});
});

Expand All @@ -66,8 +85,7 @@ describe('multiple calls to done()', function () {

it('correctly attributes the error', function () {
assert.equal(res.failures[0].fullTitle, 'suite "before all" hook');
assert.equal(res.failures[0].err.message,
'done() called multiple times');
assert.equal(res.failures[0].err.message, 'done() called multiple times');
});
});

Expand All @@ -90,8 +108,7 @@ describe('multiple calls to done()', function () {
assert.equal(res.failures.length, 2);
res.failures.forEach(function (failure) {
assert.equal(failure.fullTitle, 'suite "before each" hook');
assert.equal(failure.err.message,
'done() called multiple times');
assert.equal(failure.err.message, 'done() called multiple times');
});
});
});
Expand Down
3 changes: 2 additions & 1 deletion test/unit/runnable.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,8 @@ describe('Runnable(title, fn)', function () {

test.on('error', function (err) {
++errCalls;
expect(err.message).to.equal('fail');
expect(err.message).to.equal(
"fail (and Mocha's done() called multiple times)");
expect(calls).to.equal(1);
expect(errCalls).to.equal(1);
done();
Expand Down

0 comments on commit d1de4b9

Please sign in to comment.