Calling done() after this.skip() results in 'done() called multiple times'

[cressie176]

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.

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?

[ScottFreeCode]

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.

[cressie176]

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.

[ScottFreeCode]

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?

[boneskull]

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:

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

But:

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 and subsequent discussion.

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

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.