From c0137eb698add08f29035467ea1dc230904f82ba Mon Sep 17 00:00:00 2001 From: Craig Taub Date: Wed, 6 May 2020 12:44:12 +0100 Subject: [PATCH] Remove enableTimeout api + allow overriding a disabled timeout (#4260) * 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 --- .nycrc | 1 + docs/index.md | 2 + lib/context.js | 15 ---- lib/mocha.js | 17 ---- lib/runnable.js | 33 +++----- lib/suite.js | 30 ++----- lib/test.js | 1 - .../fixtures/timeout-override.fixture.js | 9 +++ test/integration/timeout.spec.js | 11 +++ test/unit/context.spec.js | 6 -- test/unit/mocha.spec.js | 13 --- test/unit/runnable.spec.js | 80 ++++--------------- test/unit/test.spec.js | 5 -- test/unit/timeout.spec.js | 10 +-- 14 files changed, 54 insertions(+), 179 deletions(-) create mode 100644 test/integration/fixtures/timeout-override.fixture.js diff --git a/.nycrc b/.nycrc index d4091b1a0f..aac56b8030 100644 --- a/.nycrc +++ b/.nycrc @@ -5,6 +5,7 @@ ], "exclude": [ "coverage/**", + "docs/**", "packages/*/test{,s}/**", "**/*.d.ts", "test{,s}/**", diff --git a/docs/index.md b/docs/index.md index 6c66053d8c..415430a3d4 100644 --- a/docs/index.md +++ b/docs/index.md @@ -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 diff --git a/lib/context.js b/lib/context.js index 4c6b0c23e5..2497ded868 100644 --- a/lib/context.js +++ b/lib/context.js @@ -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`. * diff --git a/lib/mocha.js b/lib/mocha.js index bc5462daac..d7d1d54709 100644 --- a/lib/mocha.js +++ b/lib/mocha.js @@ -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 @@ -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. * diff --git a/lib/runnable.js b/lib/runnable.js index bdd6fffe5c..ed585eb93f 100644 --- a/lib/runnable.js +++ b/lib/runnable.js @@ -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; @@ -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(); } @@ -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. * @@ -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)); @@ -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); diff --git a/lib/suite.js b/lib/suite.js index 191d946b50..2a64e2db8f 100644 --- a/lib/suite.js +++ b/lib/suite.js @@ -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; @@ -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; @@ -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; @@ -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". * @@ -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; @@ -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); @@ -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); diff --git a/lib/test.js b/lib/test.js index 87f1ccce7d..29c74b4563 100644 --- a/lib/test.js +++ b/lib/test.js @@ -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); diff --git a/test/integration/fixtures/timeout-override.fixture.js b/test/integration/fixtures/timeout-override.fixture.js new file mode 100644 index 0000000000..c780024824 --- /dev/null +++ b/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); + }); +}); diff --git a/test/integration/timeout.spec.js b/test/integration/timeout.spec.js index 53dfb58274..0f98467acc 100644 --- a/test/integration/timeout.spec.js +++ b/test/integration/timeout.spec.js @@ -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(); + }); + }); }); diff --git a/test/unit/context.spec.js b/test/unit/context.spec.js index 3f4d756469..d04b1ececa 100644 --- a/test/unit/context.spec.js +++ b/test/unit/context.spec.js @@ -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); diff --git a/test/unit/mocha.spec.js b/test/unit/mocha.spec.js index a317ca7c39..8839a2d0ed 100644 --- a/test/unit/mocha.spec.js +++ b/test/unit/mocha.spec.js @@ -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); diff --git a/test/unit/runnable.spec.js b/test/unit/runnable.spec.js index 2c079c3b40..fa328441ca 100644 --- a/test/unit/runnable.spec.js +++ b/test/unit/runnable.spec.js @@ -8,19 +8,19 @@ var STATE_FAILED = Runnable.constants.STATE_FAILED; describe('Runnable(title, fn)', function() { describe('#timeout(ms)', function() { - var MIN_TIMEOUT = 0; + var DISABLED_TIMEOUTS = 0; var MAX_TIMEOUT = 2147483647; // INT_MAX (32-bit signed integer) describe('when value is less than lower bound', function() { it('should clamp to lower bound given numeric', function() { var run = new Runnable(); run.timeout(-1); - expect(run.timeout(), 'to be', MIN_TIMEOUT); + expect(run.timeout(), 'to be', DISABLED_TIMEOUTS); }); it('should clamp to lower bound given timestamp', function() { var run = new Runnable(); run.timeout('-1 ms'); - expect(run.timeout(), 'to be', MIN_TIMEOUT); + expect(run.timeout(), 'to be', DISABLED_TIMEOUTS); }); }); @@ -29,25 +29,17 @@ describe('Runnable(title, fn)', function() { beforeEach(function() { run = new Runnable(); - run.timeout(MIN_TIMEOUT); + run.timeout(DISABLED_TIMEOUTS); }); describe('given numeric value', function() { - it('should set the timeout value', function() { - expect(run.timeout(), 'to be', MIN_TIMEOUT); - }); - - it('should disable timeouts', function() { - expect(run.enableTimeouts(), 'to be false'); + it('should set the timeout value to disabled', function() { + expect(run.timeout(), 'to be', DISABLED_TIMEOUTS); }); }); describe('given string timestamp', function() { - it('should set the timeout value', function() { - expect(run.timeout(), 'to be', MIN_TIMEOUT); - }); - - it('should disable timeouts', function() { - expect(run.enableTimeouts(), 'to be false'); + it('should set the timeout value to disabled', function() { + expect(run.timeout(), 'to be', DISABLED_TIMEOUTS); }); }); }); @@ -65,20 +57,12 @@ describe('Runnable(title, fn)', function() { it('should set the timeout value', function() { expect(run.timeout(), 'to be', timeout); }); - - it('should enable timeouts', function() { - expect(run.enableTimeouts(), 'to be true'); - }); }); describe('given string timestamp', function() { it('should set the timeout value', function() { expect(run.timeout(), 'to be', timeout); }); - - it('should enable timeouts', function() { - expect(run.enableTimeouts(), 'to be true'); - }); }); }); @@ -90,22 +74,8 @@ describe('Runnable(title, fn)', function() { run.timeout(MAX_TIMEOUT); }); describe('given numeric value', function() { - it('should set the timeout value', function() { - expect(run.timeout(), 'to be', MAX_TIMEOUT); - }); - - it('should disable timeouts', function() { - expect(run.enableTimeouts(), 'to be false'); - }); - }); - - describe('given string timestamp', function() { - it('should set the timeout value', function() { - expect(run.timeout(), 'to be', MAX_TIMEOUT); - }); - - it('should disable timeouts', function() { - expect(run.enableTimeouts(), 'to be false'); + it('should set the disabled timeout value', function() { + expect(run.timeout(), 'to be', 0); }); }); }); @@ -120,35 +90,13 @@ describe('Runnable(title, fn)', function() { }); describe('given numeric value', function() { - it('should clamp the value to max timeout', function() { - expect(run.timeout(), 'to be', MAX_TIMEOUT); - }); - - it('should enable timeouts', function() { - expect(run.enableTimeouts(), 'to be false'); - }); - }); - - describe('given string timestamp', function() { - it('should clamp the value to max timeout', function() { - expect(run.timeout(), 'to be', MAX_TIMEOUT); - }); - - it('should enable timeouts', function() { - expect(run.enableTimeouts(), 'to be false'); + it('should set the disabled timeout value', function() { + expect(run.timeout(), 'to be', 0); }); }); }); }); - describe('#enableTimeouts(enabled)', function() { - it('should set enabled', function() { - var run = new Runnable(); - run.enableTimeouts(false); - expect(run.enableTimeouts(), 'to be false'); - }); - }); - describe('#slow(ms)', function() { var run; @@ -314,7 +262,7 @@ describe('Runnable(title, fn)', function() { }, 2); }); runnable.timeout(1); - runnable.enableTimeouts(false); + runnable.timeout(0); runnable.run(function(err) { expect(err, 'to be falsy'); done(); @@ -721,7 +669,7 @@ describe('Runnable(title, fn)', function() { var runnable = new Runnable('foo', function() {}); runnable.timeout(10); runnable.resetTimeout(); - runnable.enableTimeouts(false); + runnable.timeout(0); setTimeout(function() { expect(runnable.timedOut, 'to be', false); done(); diff --git a/test/unit/test.spec.js b/test/unit/test.spec.js index 7bf739d47e..4ccd891bac 100644 --- a/test/unit/test.spec.js +++ b/test/unit/test.spec.js @@ -10,7 +10,6 @@ describe('Test', function() { this._test = new Test('To be cloned', function() {}); this._test._timeout = 3043; this._test._slow = 101; - this._test._enableTimeouts = true; this._test._retries = 3; this._test._currentRetry = 1; this._test._allowedGlobals = ['foo']; @@ -30,10 +29,6 @@ describe('Test', function() { expect(this._test.clone().slow(), 'to be', 101); }); - it('should copy the enableTimeouts value', function() { - expect(this._test.clone().enableTimeouts(), 'to be', true); - }); - it('should copy the retries value', function() { expect(this._test.clone().retries(), 'to be', 3); }); diff --git a/test/unit/timeout.spec.js b/test/unit/timeout.spec.js index e96f4b5d23..5cf3dce3b7 100644 --- a/test/unit/timeout.spec.js +++ b/test/unit/timeout.spec.js @@ -21,12 +21,6 @@ describe('timeouts', function() { }); describe('disabling', function() { - it('should allow overriding per-test', function(done) { - this.enableTimeouts(false); - this.timeout(1); - setTimeout(done, 2); - }); - it('should work with timeout(0)', function(done) { this.timeout(0); setTimeout(done, 1); @@ -52,13 +46,13 @@ describe('timeouts', function() { }); }); - describe('using enableTimeouts(false)', function() { + describe('using timeout(0)', function() { this.timeout(4); it('should suppress timeout(4)', function(done) { this.slow(100); // The test is in the before() call. - this.enableTimeouts(false); + this.timeout(0); setTimeout(done, 50); }); });