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

Remove enableTimeout api + allow overriding a disabled timeout #4260

Merged
merged 14 commits into from May 6, 2020
1 change: 1 addition & 0 deletions .nycrc
Expand Up @@ -5,6 +5,7 @@
],
"exclude": [
"coverage/**",
"docs/**",
"packages/*/test{,s}/**",
"**/*.d.ts",
"test{,s}/**",
Expand Down
2 changes: 2 additions & 0 deletions docs/index.md
Expand Up @@ -809,6 +809,8 @@ describe('a suite of tests', function() {
Again, use `this.timeout(0)` to disable the timeout for a hook.

> In v3.0.0 or newer, a parameter passed to `this.timeout()` greater than the [maximum delay value][mdn-settimeout-maxdelay] will cause the timeout to be disabled.
> In v8.0.0 or newer, `this.enableTimeouts()` has been removed.
> **Warning:** With async tests if you disable timeouts via `this.timeout(0)` and then do not call `done()`, your test will exit silently.

## Diffs

Expand Down
15 changes: 0 additions & 15 deletions lib/context.js
Expand Up @@ -45,21 +45,6 @@ Context.prototype.timeout = function(ms) {
return this;
};

/**
* Set test timeout `enabled`.
*
* @private
* @param {boolean} enabled
* @return {Context} self
*/
Context.prototype.enableTimeouts = function(enabled) {
if (!arguments.length) {
return this.runnable().enableTimeouts();
}
this.runnable().enableTimeouts(enabled);
return this;
};

/**
* Set or get test slowness threshold `ms`.
*
Expand Down
17 changes: 0 additions & 17 deletions lib/mocha.js
Expand Up @@ -627,7 +627,6 @@ Mocha.prototype.diff = function(diff) {
* @public
* @see [CLI option](../#-timeout-ms-t-ms)
* @see [Timeouts](../#timeouts)
* @see {@link Mocha#enableTimeouts}
* @param {number|string} msecs - Timeout threshold value.
* @return {Mocha} this
* @chainable
Expand Down Expand Up @@ -686,22 +685,6 @@ Mocha.prototype.slow = function(msecs) {
return this;
};

/**
* Enables or disables timeouts.
*
* @public
* @see [CLI option](../#-timeout-ms-t-ms)
* @param {boolean} enableTimeouts - Whether to enable timeouts.
* @return {Mocha} this
* @chainable
*/
Mocha.prototype.enableTimeouts = function(enableTimeouts) {
this.suite.enableTimeouts(
arguments.length && enableTimeouts !== undefined ? enableTimeouts : true
);
return this;
};

/**
* Forces all tests to either accept a `done` callback or return a promise.
*
Expand Down
33 changes: 9 additions & 24 deletions lib/runnable.js
Expand Up @@ -35,7 +35,6 @@ function Runnable(title, fn) {
this.sync = !this.async;
this._timeout = 2000;
this._slow = 75;
this._enableTimeouts = true;
this.timedOut = false;
this._retries = -1;
this._currentRetry = 0;
Expand Down Expand Up @@ -83,10 +82,12 @@ Runnable.prototype.timeout = function(ms) {

// see #1652 for reasoning
if (ms === range[0] || ms === range[1]) {
this._enableTimeouts = false;
craigtaub marked this conversation as resolved.
Show resolved Hide resolved
this._timeout = 0;
} else {
this._timeout = ms;
}
debug('timeout %d', ms);
this._timeout = ms;
debug('timeout %d', this._timeout);

if (this.timer) {
this.resetTimeout();
}
Expand All @@ -112,22 +113,6 @@ Runnable.prototype.slow = function(ms) {
return this;
};

/**
* Set and get whether timeout is `enabled`.
*
* @private
* @param {boolean} enabled
* @return {Runnable|boolean} enabled or Runnable instance.
*/
Runnable.prototype.enableTimeouts = function(enabled) {
if (!arguments.length) {
return this._enableTimeouts;
}
debug('enableTimeouts %s', enabled);
this._enableTimeouts = enabled;
return this;
};

/**
* Halt and mark as pending.
*
Expand Down Expand Up @@ -229,14 +214,14 @@ Runnable.prototype.clearTimeout = function() {
*/
Runnable.prototype.resetTimeout = function() {
var self = this;
var ms = this.timeout() || 1e9;
craigtaub marked this conversation as resolved.
Show resolved Hide resolved
var ms = this.timeout();

if (!this._enableTimeouts) {
if (ms === 0) {
return;
}
this.clearTimeout();
this.timer = setTimeout(function() {
if (!self._enableTimeouts) {
craigtaub marked this conversation as resolved.
Show resolved Hide resolved
if (self.timeout() === 0) {
return;
}
self.callback(self._timeoutError(ms));
Expand Down Expand Up @@ -306,7 +291,7 @@ Runnable.prototype.run = function(fn) {
self.clearTimeout();
self.duration = new Date() - start;
finished = true;
if (!err && self.duration > ms && self._enableTimeouts) {
if (!err && self.duration > ms && ms > 0) {
err = self._timeoutError(ms);
}
fn(err);
Expand Down
30 changes: 6 additions & 24 deletions lib/suite.js
Expand Up @@ -68,7 +68,6 @@ function Suite(title, parentContext, isRoot) {
this._afterAll = [];
this.root = isRoot === true;
this._timeout = 2000;
this._enableTimeouts = true;
this._slow = 75;
this._bail = false;
this._retries = -1;
Expand Down Expand Up @@ -105,7 +104,6 @@ Suite.prototype.clone = function() {
suite.root = this.root;
suite.timeout(this.timeout());
suite.retries(this.retries());
suite.enableTimeouts(this.enableTimeouts());
suite.slow(this.slow());
suite.bail(this.bail());
return suite;
Expand All @@ -123,12 +121,15 @@ Suite.prototype.timeout = function(ms) {
if (!arguments.length) {
craigtaub marked this conversation as resolved.
Show resolved Hide resolved
return this._timeout;
}
if (ms.toString() === '0') {
this._enableTimeouts = false;
}
if (typeof ms === 'string') {
ms = milliseconds(ms);
}

// Clamp to range
var INT_MAX = Math.pow(2, 31) - 1;
var range = [0, INT_MAX];
ms = utils.clamp(ms, range);

debug('timeout %d', ms);
this._timeout = parseInt(ms, 10);
return this;
Expand All @@ -150,22 +151,6 @@ Suite.prototype.retries = function(n) {
return this;
};

/**
* Set or get timeout to `enabled`.
*
* @private
* @param {boolean} enabled
* @return {Suite|boolean} self or enabled
*/
Suite.prototype.enableTimeouts = function(enabled) {
if (!arguments.length) {
return this._enableTimeouts;
}
debug('enableTimeouts %s', enabled);
this._enableTimeouts = enabled;
return this;
};

/**
* Set or get slow `ms` or short-hand such as "2s".
*
Expand Down Expand Up @@ -222,7 +207,6 @@ Suite.prototype._createHook = function(title, fn) {
hook.parent = this;
hook.timeout(this.timeout());
hook.retries(this.retries());
hook.enableTimeouts(this.enableTimeouts());
hook.slow(this.slow());
hook.ctx = this.ctx;
hook.file = this.file;
Expand Down Expand Up @@ -337,7 +321,6 @@ Suite.prototype.addSuite = function(suite) {
suite.root = false;
suite.timeout(this.timeout());
suite.retries(this.retries());
suite.enableTimeouts(this.enableTimeouts());
suite.slow(this.slow());
suite.bail(this.bail());
this.suites.push(suite);
Expand All @@ -356,7 +339,6 @@ Suite.prototype.addTest = function(test) {
test.parent = this;
test.timeout(this.timeout());
test.retries(this.retries());
test.enableTimeouts(this.enableTimeouts());
test.slow(this.slow());
test.ctx = this.ctx;
this.tests.push(test);
Expand Down
1 change: 0 additions & 1 deletion lib/test.js
Expand Up @@ -61,7 +61,6 @@ Test.prototype.clone = function() {
var test = new Test(this.title, this.fn);
test.timeout(this.timeout());
test.slow(this.slow());
test.enableTimeouts(this.enableTimeouts());
test.retries(this.retries());
test.currentRetry(this.currentRetry());
test.retriedTest(this.retriedTest() || this);
Expand Down
9 changes: 9 additions & 0 deletions test/integration/fixtures/timeout-override.fixture.js
@@ -0,0 +1,9 @@
'use strict';

describe('timeout override', function() {
it('should fail async test due to re-enable', function(done) {
this.timeout(0);
this.timeout(1);
setTimeout(done, 2);
});
});
11 changes: 11 additions & 0 deletions test/integration/timeout.spec.js
Expand Up @@ -18,4 +18,15 @@ describe('this.timeout()', function() {
done();
});
});

it('should allow overriding if disabled per-test', function(done) {
run('timeout-override.fixture.js', args, function(err, res) {
if (err) {
done(err);
return;
}
expect(res.stats.failures, 'to be', 1);
done();
});
});
});
6 changes: 0 additions & 6 deletions test/unit/context.spec.js
Expand Up @@ -90,12 +90,6 @@ describe('methods', function() {
});
});

describe('enableTimeouts()', function() {
it('should return the enableTimeouts', function() {
expect(this.enableTimeouts(), 'to be', true);
});
});

describe('retries', function() {
it('should return the number of retries', function() {
expect(this.retries(), 'to be', -1);
Expand Down
13 changes: 0 additions & 13 deletions test/unit/mocha.spec.js
Expand Up @@ -159,19 +159,6 @@ describe('Mocha', function() {
});
});

describe('#enableTimeouts()', function() {
it('should set the suite._enableTimeouts to true if no argument', function() {
var mocha = new Mocha(opts);
mocha.enableTimeouts();
expect(mocha.suite._enableTimeouts, 'to be', true);
});

it('should be chainable', function() {
var mocha = new Mocha(opts);
expect(mocha.enableTimeouts(), 'to be', mocha);
});
});

describe('#diff()', function() {
it('should set the diff option to true', function() {
var mocha = new Mocha(opts);
Expand Down