Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cressie176 opened this issue Aug 31, 2016 · 4 comments
Closed
Labels
type: bug a defect, confirmed by a maintainer

Comments

@cressie176
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer confirmed labels Sep 14, 2016
boneskull added a commit that referenced this issue Sep 14, 2016
- also fixes lack of support for async `skip()` when `allowUncaught` option is used
- adds some debuggability into `this.skip()` calls
boneskull added a commit that referenced this issue Sep 16, 2016
- also fixes lack of support for async `skip()` when `allowUncaught` option is used
- adds some debuggability into `this.skip()` calls
1999 pushed a commit to 1999/mocha that referenced this issue Sep 19, 2016
…roperty-currentretry

* upstream/master:
  helpful error when necessary suite callback omitted; closes mochajs#1744
  Fix an issue and add relevant tests when describe and xdescribe fail when not supplied with a callback (issue mochajs#1744).
  rename more fixtures; closes mochajs#2383
  Report non-match to STDERR and exit if no files are matched (mochajs#2450)
  Exit process with correct error codes (mochajs#2445)
  fix test files not using .spec suffix fix test fixtures not using .fixture suffix
  always halt execution when async skip() called; closes mochajs#2465 (mochajs#2479)
  avoid test flake in "delay" test; closes mochajs#2469 (mochajs#2470)
  revert accidental change to bin paths
  disregard function names when testing under Node.js 6.5.0; closes mochajs#2467 (mochajs#2477)
  lints more files; add more files to lint check; closes mochajs#2457
  adds *.orig to .gitignore
  Exclude the --inspect flag
  fix check-leaks to catch a leak whose name begins with 'd'
1999 pushed a commit to 1999/mocha that referenced this issue Sep 22, 2016
…-files-cache

* upstream/master:
  attempt windows-friendly reproducible case for mochajs#2315
  fix: fix uncaught TypeError if error occurs on next tick, closes mochajs#2315 (mochajs#2439)
  helpful error when necessary suite callback omitted; closes mochajs#1744
  Fix an issue and add relevant tests when describe and xdescribe fail when not supplied with a callback (issue mochajs#1744).
  rename more fixtures; closes mochajs#2383
  Report non-match to STDERR and exit if no files are matched (mochajs#2450)
  Exit process with correct error codes (mochajs#2445)
  fix test files not using .spec suffix fix test fixtures not using .fixture suffix
  always halt execution when async skip() called; closes mochajs#2465 (mochajs#2479)
  avoid test flake in "delay" test; closes mochajs#2469 (mochajs#2470)
  revert accidental change to bin paths
  disregard function names when testing under Node.js 6.5.0; closes mochajs#2467 (mochajs#2477)
  lints more files; add more files to lint check; closes mochajs#2457
  adds *.orig to .gitignore
  Exclude the --inspect flag
  fix check-leaks to catch a leak whose name begins with 'd'
sgilroy pushed a commit to TwineHealth/mocha that referenced this issue Feb 27, 2019
…ochajs#2479)

- always halt execution when async skip() called; closes mochajs#2465
- also fixes lack of support for async `skip()` when `allowUncaught` option is used
- adds some debuggability into `this.skip()` calls
- avoid double-failure conditions when using this.skip()
- fix bad Runner tests; lint & add test/runner.js to check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

No branches or pull requests

3 participants