Repository: mochajs/mocha

Problem statement:
Calling done() after this.skip() results in 'done() called multiple times'
The following code skips the test in mocha v2, but causes an error in v3
#### Test

```
describe('broken skip behaviour', function() {
    it('should not report done() called multiple times', function(done) {
        this.skip()
        done()
    })
})
```
#### v2 Output

```
  broken skip behaviour
    - should not report done() called multiple times


  0 passing (11ms)
  1 pending
```
#### v3 Output

```
  broken skip behaviour
    - should not report done() called multiple times
    1) should not report done() called multiple times


  0 passing (15ms)
  1 pending
  1 failing

  1) broken skip behaviour should not report done() called multiple times:
     Error: done() called multiple times
      at Suite.<anonymous> (test/index.test.js:3:5)
      at Object.<anonymous> (test/index.test.js:1:63)
      at require (internal/module.js:20:19)
      at Array.forEach (native)
      at node.js:463:3
```

Unfortunately I rely on this behaviour for skipping tests in [Yadda](https://github.com/acuminous/yadda/blob/master/lib/plugins/mocha/StepLevelPlugin.js#L51-L52). 

I've tried with various version of mocha from 1-3 and it looks like I can simply remove the call to done() in all versions. Can you confirm?



Hints:
> I've tried with various version of mocha from 1-3 and it looks like I can simply remove the call to done() in all versions. Can you confirm?

You don't need `done` if all your test code executes synchronously; you need it (or to return a promise encapsulating the asynchronous code) if your test executes anything synchronous.

Either way, I'm pretty sure this is a legit bug -- once `skip` is called the test should be aborted as pending and `done` shouldn't affect anything, unless I'm missing something.

Hi @ScottFreeCode, thanks for responding.

> > I've tried with various version of mocha from 1-3 and it looks like I can simply remove the call to done() in all versions. Can you confirm?
> 
> You don't need done if all your test code executes synchronously; you need it (or to return a promise encapsulating the asynchronous code) if your test executes anything synchronous.

I mean removing the call to done if `this.skip()` was called (Yadda calls this.skip() conditionally), e.g. 

```
it('should not report done() called multiple times', function(done) {
  if (shouldAbort()) return this.skip()
  doStuff((result) => {
    assert(result)
    done()
  })
}
```

The above seems to work fine, but not sure if it was an intentional change from 2 => 3 or accidental one.

Yeah, that seems like it should work, but it also seems like it shouldn't be necessary; @boneskull, do you know if there was any intended behavior change in 3.x that might be causing this?

Yes.

@cressie176 Mocha pre-v3 doesn't truly support `this.skip()` in async tests.  In your example, your test is not actually running async code, so you wouldn't notice:

``` js
describe('broken skip behaviour', function() {
    it('should not report done() called multiple times', function(done) {
        this.skip()
        done() // this is never actually called.
    })
})
```

But:

``` js
describe('broken skip behaviour', function() {
    it('should not report done() called multiple times', function(done) {
        setTimeout(() => {
          this.skip() // uncaught exception
          done() // still never called
        })
    })
})
```

results in (again, Mocha 2.x here):

```
  broken skip behaviour
    1) should not report done() called multiple times


  0 passing (16ms)
  1 failing

  1) broken skip behaviour should not report done() called multiple times:
     Error: the object {
  "message": [undefined]
  "uncaught": true
} was thrown, throw an Error :)
      at process._fatalException (bootstrap_node.js:296:26)
```

See [this comment](https://github.com/mochajs/mocha/pull/946#issuecomment-227715287) and subsequent discussion.

But, yes, there's a bug here; Mocha 3.x does this:

``` js
describe('broken skip behaviour', function() {
    it('should not report done() called multiple times', function(done) {
        this.skip()
        done() // this actually gets called, which is wrong.
    })
})
```

So that needs fixing; hopefully it won't be too much of a headache.


Patch:
diff --git a/lib/runnable.js b/lib/runnable.js
--- a/lib/runnable.js
+++ b/lib/runnable.js
@@ -133,7 +133,7 @@ Runnable.prototype.enableTimeouts = function(enabled) {
  * @api public
  */
 Runnable.prototype.skip = function() {
-  throw new Pending();
+  throw new Pending('sync skip');
 };
 
 /**
@@ -298,14 +298,19 @@ Runnable.prototype.run = function(fn) {
   if (this.async) {
     this.resetTimeout();
 
+    // allows skip() to be used in an explicit async context
+    this.skip = function asyncSkip() {
+      done(new Pending('async skip call'));
+      // halt execution.  the Runnable will be marked pending
+      // by the previous call, and the uncaught handler will ignore
+      // the failure.
+      throw new Pending('async skip; aborting execution');
+    };
+
     if (this.allowUncaught) {
       return callFnAsync(this.fn);
     }
     try {
-      // allows skip() to be used in an explicit async context
-      this.skip = function() {
-        done(new Pending());
-      };
       callFnAsync(this.fn);
     } catch (err) {
       done(utils.getError(err));
diff --git a/lib/runner.js b/lib/runner.js
--- a/lib/runner.js
+++ b/lib/runner.js
@@ -220,6 +220,10 @@ Runner.prototype.checkGlobals = function(test) {
  * @param {Error} err
  */
 Runner.prototype.fail = function(test, err) {
+  if (test.isPending()) {
+    return;
+  }
+
   ++this.failures;
   test.state = 'failed';
 
@@ -309,6 +313,8 @@ Runner.prototype.hook = function(name, fn) {
             suite.tests.forEach(function(test) {
               test.pending = true;
             });
+            // a pending hook won't be executed twice.
+            hook.pending = true;
           }
         } else {
           self.failHook(hook, err);
@@ -695,8 +701,8 @@ Runner.prototype.uncaught = function(err) {
 
   runnable.clearTimeout();
 
-  // Ignore errors if complete
-  if (runnable.state) {
+  // Ignore errors if complete or pending
+  if (runnable.state || runnable.isPending()) {
     return;
   }
   this.fail(runnable, err);


Test patch:
diff --git a/test/runner.js b/test/runner.js
--- a/test/runner.js
+++ b/test/runner.js
@@ -1,55 +1,59 @@
-var mocha = require('../')
-  , Suite = mocha.Suite
-  , Runner = mocha.Runner
-  , Test = mocha.Test;
+var mocha = require('../');
+var Suite = mocha.Suite;
+var Runner = mocha.Runner;
+var Test = mocha.Test;
+var Hook = mocha.Hook;
 
-describe('Runner', function(){
-  var suite, runner;
+function noop() {}
 
-  beforeEach(function(){
+describe('Runner', function() {
+  var suite;
+  var runner;
+
+  beforeEach(function() {
     suite = new Suite('Suite', 'root');
     runner = new Runner(suite);
-  })
+  });
 
-  describe('.grep()', function(){
-    it('should update the runner.total with number of matched tests', function(){
-      suite.addTest(new Test('im a test about lions'));
-      suite.addTest(new Test('im another test about lions'));
-      suite.addTest(new Test('im a test about bears'));
+  describe('.grep()', function() {
+    it('should update the runner.total with number of matched tests', function() {
+      suite.addTest(new Test('im a test about lions', noop));
+      suite.addTest(new Test('im another test about lions', noop));
+      suite.addTest(new Test('im a test about bears', noop));
       var newRunner = new Runner(suite);
       newRunner.grep(/lions/);
       newRunner.total.should.equal(2);
-    })
+    });
 
-    it('should update the runner.total with number of matched tests when inverted', function(){
-      suite.addTest(new Test('im a test about lions'));
-      suite.addTest(new Test('im another test about lions'));
-      suite.addTest(new Test('im a test about bears'));
+    it('should update the runner.total with number of matched tests when inverted', function() {
+      suite.addTest(new Test('im a test about lions', noop));
+      suite.addTest(new Test('im another test about lions', noop));
+      suite.addTest(new Test('im a test about bears', noop));
       var newRunner = new Runner(suite);
       newRunner.grep(/lions/, true);
       newRunner.total.should.equal(1);
-    })
-  })
-
-  describe('.grepTotal()', function(){
-    it('should return the total number of matched tests', function(){
-      suite.addTest(new Test('im a test about lions'));
-      suite.addTest(new Test('im another test about lions'));
-      suite.addTest(new Test('im a test about bears'));
+    });
+  });
+
+  describe('.grepTotal()', function() {
+    it('should return the total number of matched tests', function() {
+      suite.addTest(new Test('im a test about lions', noop));
+      suite.addTest(new Test('im another test about lions', noop));
+      suite.addTest(new Test('im a test about bears', noop));
       runner.grep(/lions/);
       runner.grepTotal(suite).should.equal(2);
-    })
+    });
 
-    it('should return the total number of matched tests when inverted', function(){
-      suite.addTest(new Test('im a test about lions'));
-      suite.addTest(new Test('im another test about lions'));
-      suite.addTest(new Test('im a test about bears'));
+    it('should return the total number of matched tests when inverted', function() {
+      suite.addTest(new Test('im a test about lions', noop));
+      suite.addTest(new Test('im another test about lions', noop));
+      suite.addTest(new Test('im a test about bears', noop));
       runner.grep(/lions/, true);
       runner.grepTotal(suite).should.equal(1);
-    })
-  })
+    });
+  });
 
-  describe('.globalProps()', function(){
+  describe('.globalProps()', function() {
     it('should include common non enumerable globals', function() {
       var props = runner.globalProps();
       props.should.containEql('setTimeout');
@@ -61,19 +65,19 @@ describe('Runner', function(){
     });
   });
 
-  describe('.globals()', function(){
-    it('should default to the known globals', function(){
+  describe('.globals()', function() {
+    it('should default to the known globals', function() {
       runner.globals().length.should.be.above(16);
-    })
+    });
 
-    it('should white-list globals', function(){
+    it('should white-list globals', function() {
       runner.globals(['foo', 'bar']);
       runner.globals().should.containEql('foo');
       runner.globals().should.containEql('bar');
-    })
-  })
+    });
+  });
 
-  describe('.checkGlobals(test)', function(){
+  describe('.checkGlobals(test)', function() {
     it('should allow variables that match a wildcard', function(done) {
       runner.globals(['foo*', 'giz*']);
       global.foo = 'baz';
@@ -81,43 +85,44 @@ describe('Runner', function(){
       runner.checkGlobals();
       delete global.foo;
       delete global.gizmo;
-      done()
-    })
+      done();
+    });
 
-    it('should emit "fail" when a new global is introduced', function(done){
+    it('should emit "fail" when a new global is introduced', function(done) {
+      var test = new Test('im a test', noop);
       runner.checkGlobals();
       global.foo = 'bar';
-      runner.on('fail', function(test, err){
-        test.should.equal('im a test');
+      runner.on('fail', function(_test, err) {
+        _test.should.equal(test);
         err.message.should.equal('global leak detected: foo');
         delete global.foo;
         done();
       });
-      runner.checkGlobals('im a test');
-    })
+      runner.checkGlobals(test);
+    });
 
     it('should emit "fail" when a single new disallowed global is introduced after a single extra global is allowed', function(done) {
       var doneCalled = false;
       runner.globals('good');
       global.bad = 1;
-      runner.on('fail', function(test, err) {
+      runner.on('fail', function() {
         delete global.bad;
         done();
         doneCalled = true;
       });
-      runner.checkGlobals('test');
+      runner.checkGlobals(new Test('yet another test', noop));
       if (!doneCalled) {
-        done(Error("Expected test failure did not occur."));
+        done(Error('Expected test failure did not occur.'));
       }
     });
 
-    it ('should not fail when a new common global is introduced', function(){
+    it('should not fail when a new common global is introduced', function() {
       // verify that the prop isn't enumerable
       delete global.XMLHttpRequest;
       global.propertyIsEnumerable('XMLHttpRequest').should.not.be.ok();
 
       // create a new runner and keep a reference to the test.
-      var test = new Test('im a test about bears');
+      var test = new Test('im a test about bears', noop);
       suite.addTest(test);
       var newRunner = new Runner(suite);
 
@@ -133,22 +138,23 @@ describe('Runner', function(){
       delete global.XMLHttpRequest;
     });
 
-    it('should pluralize the error message when several are introduced', function(done){
+    it('should pluralize the error message when several are introduced', function(done) {
+      var test = new Test('im a test', noop);
       runner.checkGlobals();
       global.foo = 'bar';
       global.bar = 'baz';
-      runner.on('fail', function(test, err){
-        test.should.equal('im a test');
+      runner.on('fail', function(_test, err) {
+        _test.should.equal(test);
         err.message.should.equal('global leaks detected: foo, bar');
         delete global.foo;
         delete global.bar;
         done();
       });
-      runner.checkGlobals('im a test');
-    })
+      runner.checkGlobals(test);
+    });
 
     it('should respect per test whitelisted globals', function() {
-      var test = new Test('im a test about lions');
+      var test = new Test('im a test about lions', noop);
       test.globals(['foo']);
 
       suite.addTest(test);
@@ -161,166 +167,179 @@ describe('Runner', function(){
       test.should.not.have.key('state');
 
       delete global.foo;
-    })
+    });
 
     it('should respect per test whitelisted globals but still detect other leaks', function(done) {
-      var test = new Test('im a test about lions');
+      var test = new Test('im a test about lions', noop);
       test.globals(['foo']);
 
       suite.addTest(test);
 
       global.foo = 'bar';
       global.bar = 'baz';
-      runner.on('fail', function(test, err){
+      runner.on('fail', function(test, err) {
         test.title.should.equal('im a test about lions');
         err.message.should.equal('global leak detected: bar');
         delete global.foo;
         done();
       });
       runner.checkGlobals(test);
-    })
+    });
 
     it('should emit "fail" when a global beginning with d is introduced', function(done) {
       global.derp = 'bar';
-      runner.on('fail', function(test, err){
+      runner.on('fail', function() {
         delete global.derp;
         done();
       });
-      runner.checkGlobals('im a test');
+      runner.checkGlobals(new Test('herp', function() {}));
     });
-  })
+  });
 
-  describe('.hook(name, fn)', function(){
-    it('should execute hooks after failed test if suite bail is true', function(done){
-      runner.fail({});
+  describe('.hook(name, fn)', function() {
+    it('should execute hooks after failed test if suite bail is true', function(done) {
+      runner.fail(new Test('failed test', noop));
       suite.bail(true);
-      suite.afterEach(function(){
+      suite.afterEach(function() {
         suite.afterAll(function() {
           done();
-        })
+        });
       });
-      runner.hook('afterEach', function(){});
-      runner.hook('afterAll', function(){});
-    })
-  })
+      runner.hook('afterEach', function() {});
+      runner.hook('afterAll', function() {});
+    });
+  });
 
-  describe('.fail(test, err)', function(){
-    it('should increment .failures', function(){
+  describe('.fail(test, err)', function() {
+    it('should increment .failures', function() {
       runner.failures.should.equal(0);
-      runner.fail({}, {});
+      runner.fail(new Test('one', noop), {});
       runner.failures.should.equal(1);
-      runner.fail({}, {});
+      runner.fail(new Test('two', noop), {});
       runner.failures.should.equal(2);
-    })
+    });
 
-    it('should set test.state to "failed"', function(){
-      var test = {};
+    it('should set test.state to "failed"', function() {
+      var test = new Test('some test', noop);
       runner.fail(test, 'some error');
       test.state.should.equal('failed');
-    })
+    });
 
-    it('should emit "fail"', function(done){
-      var test = {}, err = {};
-      runner.on('fail', function(test, err){
+    it('should emit "fail"', function(done) {
+      var test = new Test('some other test', noop);
+      var err = {};
+      runner.on('fail', function(test, err) {
         test.should.equal(test);
         err.should.equal(err);
         done();
       });
       runner.fail(test, err);
-    })
+    });
 
-    it('should emit a helpful message when failed with a string', function(done){
-      var test = {}, err = 'string';
-      runner.on('fail', function(test, err){
+    it('should emit a helpful message when failed with a string', function(done) {
+      var test = new Test('helpful test', noop);
+      var err = 'string';
+      runner.on('fail', function(test, err) {
         err.message.should.equal('the string "string" was thrown, throw an Error :)');
         done();
       });
       runner.fail(test, err);
-    })
+    });
 
-    it('should emit a the error when failed with an Error instance', function(done){
-      var test = {}, err = new Error('an error message');
-      runner.on('fail', function(test, err){
+    it('should emit a the error when failed with an Error instance', function(done) {
+      var test = new Test('a test', noop);
+      var err = new Error('an error message');
+      runner.on('fail', function(test, err) {
         err.message.should.equal('an error message');
         done();
       });
       runner.fail(test, err);
-    })
+    });
 
-    it('should emit the error when failed with an Error-like object', function(done){
-      var test = {}, err = {message: 'an error message'};
-      runner.on('fail', function(test, err){
+    it('should emit the error when failed with an Error-like object', function(done) {
+      var test = new Test('a test', noop);
+      var err = { message: 'an error message' };
+      runner.on('fail', function(test, err) {
         err.message.should.equal('an error message');
         done();
       });
       runner.fail(test, err);
-    })
+    });
 
-    it('should emit a helpful message when failed with an Object', function(done){
-      var test = {}, err = { x: 1 };
-      runner.on('fail', function(test, err){
+    it('should emit a helpful message when failed with an Object', function(done) {
+      var test = new Test('a test', noop);
+      var err = { x: 1 };
+      runner.on('fail', function(test, err) {
         err.message.should.equal('the object {\n  "x": 1\n} was thrown, throw an Error :)');
         done();
       });
       runner.fail(test, err);
-    })
+    });
 
-    it('should emit a helpful message when failed with an Array', function(done){
-      var test = {}, err = [1,2];
-      runner.on('fail', function(test, err){
+    it('should emit a helpful message when failed with an Array', function(done) {
+      var test = new Test('a test', noop);
+      var err = [
+        1,
+        2
+      ];
+      runner.on('fail', function(test, err) {
         err.message.should.equal('the array [\n  1\n  2\n] was thrown, throw an Error :)');
         done();
       });
       runner.fail(test, err);
-    })
-  })
+    });
+  });
 
-  describe('.failHook(hook, err)', function(){
-    it('should increment .failures', function(){
+  describe('.failHook(hook, err)', function() {
+    it('should increment .failures', function() {
       runner.failures.should.equal(0);
-      runner.failHook({}, {});
+      runner.failHook(new Test('fail hook 1', noop), {});
       runner.failures.should.equal(1);
-      runner.failHook({}, {});
+      runner.failHook(new Test('fail hook 2', noop), {});
       runner.failures.should.equal(2);
-    })
+    });
+
+    it('should augment hook title with current test title', function() {
+      var hook = new Hook('"before each" hook');
+      hook.ctx = { currentTest: new Test('should behave', noop) };
 
-    it('should augment hook title with current test title', function(){
-      var hook = {
-        title: '"before each" hook',
-        ctx: { currentTest: new Test('should behave') }
-      };
       runner.failHook(hook, {});
       hook.title.should.equal('"before each" hook for "should behave"');
 
-      hook.ctx.currentTest = new Test('should obey');
+      hook.ctx.currentTest = new Test('should obey', noop);
       runner.failHook(hook, {});
       hook.title.should.equal('"before each" hook for "should obey"');
-    })
+    });
 
-    it('should emit "fail"', function(done){
-      var hook = {}, err = {};
-      runner.on('fail', function(hook, err){
+    it('should emit "fail"', function(done) {
+      var hook = new Hook();
+      var err = {};
+      runner.on('fail', function(hook, err) {
         hook.should.equal(hook);
         err.should.equal(err);
         done();
       });
       runner.failHook(hook, err);
-    })
+    });
 
-    it('should emit "end" if suite bail is true', function(done){
-      var hook = {}, err = {};
+    it('should emit "end" if suite bail is true', function(done) {
+      var hook = new Hook();
+      var err = {};
       suite.bail(true);
       runner.on('end', done);
       runner.failHook(hook, err);
-    })
+    });
 
-    it('should not emit "end" if suite bail is not true', function(done){
-      var hook = {}, err = {};
+    it('should not emit "end" if suite bail is not true', function(done) {
+      var hook = new Hook();
+      var err = {};
       suite.bail(false);
-      runner.on('end', function() { throw new Error('"end" was emit, but the bail is false'); });
+      runner.on('end', function() {
+        throw new Error('"end" was emit, but the bail is false');
+      });
       runner.failHook(hook, err);
       done();
-    })
+    });
   });
 
   describe('allowUncaught', function() {
@@ -339,27 +358,29 @@ describe('Runner', function(){
   });
 
   describe('stackTrace', function() {
-    var stack = [ 'AssertionError: foo bar'
-      , 'at EventEmitter.<anonymous> (/usr/local/dev/test.js:16:12)'
-      , 'at Context.<anonymous> (/usr/local/dev/test.js:19:5)'
-      , 'Test.Runnable.run (/usr/local/lib/node_modules/mocha/lib/runnable.js:244:7)'
-      , 'Runner.runTest (/usr/local/lib/node_modules/mocha/lib/runner.js:374:10)'
-      , '/usr/local/lib/node_modules/mocha/lib/runner.js:452:12'
-      , 'next (/usr/local/lib/node_modules/mocha/lib/runner.js:299:14)'
-      , '/usr/local/lib/node_modules/mocha/lib/runner.js:309:7'
-      , 'next (/usr/local/lib/node_modules/mocha/lib/runner.js:248:23)'
-      , 'Immediate._onImmediate (/usr/local/lib/node_modules/mocha/lib/runner.js:276:5)'
-      , 'at processImmediate [as _immediateCallback] (timers.js:321:17)'];
+    var stack = [
+      'AssertionError: foo bar',
+      'at EventEmitter.<anonymous> (/usr/local/dev/test.js:16:12)',
+      'at Context.<anonymous> (/usr/local/dev/test.js:19:5)',
+      'Test.Runnable.run (/usr/local/lib/node_modules/mocha/lib/runnable.js:244:7)',
+      'Runner.runTest (/usr/local/lib/node_modules/mocha/lib/runner.js:374:10)',
+      '/usr/local/lib/node_modules/mocha/lib/runner.js:452:12',
+      'next (/usr/local/lib/node_modules/mocha/lib/runner.js:299:14)',
+      '/usr/local/lib/node_modules/mocha/lib/runner.js:309:7',
+      'next (/usr/local/lib/node_modules/mocha/lib/runner.js:248:23)',
+      'Immediate._onImmediate (/usr/local/lib/node_modules/mocha/lib/runner.js:276:5)',
+      'at processImmediate [as _immediateCallback] (timers.js:321:17)'
+    ];
 
     describe('shortStackTrace', function() {
       it('should prettify the stack-trace', function(done) {
-        var hook = {},
-            err = new Error();
+        var hook = new Hook();
+        var err = new Error();
         // Fake stack-trace
         err.stack = stack.join('\n');
 
-        runner.on('fail', function(hook, err){
-          err.stack.should.equal(stack.slice(0,3).join('\n'));
+        runner.on('fail', function(hook, err) {
+          err.stack.should.equal(stack.slice(0, 3).join('\n'));
           done();
         });
         runner.failHook(hook, err);
@@ -368,14 +389,14 @@ describe('Runner', function(){
 
     describe('longStackTrace', function() {
       it('should display the full stack-trace', function(done) {
-        var hook = {},
-            err = new Error();
+        var hook = new Hook();
+        var err = new Error();
         // Fake stack-trace
         err.stack = stack.join('\n');
         // Add --stack-trace option
         runner.fullStackTrace = true;
 
-        runner.on('fail', function(hook, err){
+        runner.on('fail', function(hook, err) {
           err.stack.should.equal(stack.join('\n'));
           done();
         });