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
mochajs · May 6, 2020 · c0137eb · c0137eb
1 parent 8b6a76c
commit c0137eb
Show file tree

Hide file tree

Showing 14 changed files with 54 additions and 179 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);