Skip to content

Commit

Permalink
Remove enableTimeout api + allow overriding a disabled timeout (#4260)
Browse files Browse the repository at this point in the history
* Reset enableTimeouts. Integration test

* Spec update

* Use function which logs

* Remove _enableTimeouts

* Defensive check for 0

* Use correct timeout value

* Remove superfluous logic.

* Add note about `enableTimeouts` to docs

* Update comment

* Lint fix

* Disable if setting == or grtr to upper

* Swap asser to expect

* Excl docs in coverage + clamp in suite.

* Add note about async tests
  • Loading branch information
craigtaub authored May 6, 2020
1 parent 8b6a76c commit c0137eb
Show file tree
Hide file tree
Showing 14 changed files with 54 additions and 179 deletions.
1 change: 1 addition & 0 deletions .nycrc
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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;
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;
var ms = this.timeout();

if (!this._enableTimeouts) {
if (ms === 0) {
return;
}
this.clearTimeout();
this.timer = setTimeout(function() {
if (!self._enableTimeouts) {
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
Original file line number Diff line number Diff line change
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) {
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading

0 comments on commit c0137eb

Please sign in to comment.