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

mocha loses the right title when done called multiple times #1798

Closed
osher opened this issue Jul 14, 2015 · 4 comments · Fixed by gluckgames/pixi-packer#17
Closed

mocha loses the right title when done called multiple times #1798

osher opened this issue Jul 14, 2015 · 4 comments · Fixed by gluckgames/pixi-packer#17

Comments

@osher
Copy link

osher commented Jul 14, 2015

I just finished trouble-shooting a wierd "done called multiple times" message.... phewee.. here's a tricky one:

I had a before-all hook that looked as following:

{ "lib/rest" : 
  { beforeAll:
    function(done) {
        this.slow("1.1s");
        rest = require('../lib/rest')(sockets, eventsHandler, done);
        setTimeout(done, 1000)
    }
  , 

Where evidently, the done is called both from ../lib/rest and the setTimeout.
Anyway, the ../lib/rest was not calling the callback for a long time, and evidently, making it call the callback as it should - was actually what broke the build.

(I'm using mocha-ui-exports, that's why it looks like that)

However, mocha could not direct me to the right place - probably because the setTimeout handler that was fired after 1s - was called in the midst of some other suite that followed - and mocha was pointing me to the suite and the test that during it's run the timer was fired...

looking there obviously brought nothing, and pending the tests that were reported as failing just moved the failure to another test that was not pended - and that's where I started suspecting that mocha reports the wrong place, and expanded my search.

After fixing the issue (removing the setTimeout from the beforeAll hook) - the suite passes.

@danielstjules
Copy link
Contributor

Unfortunately, no portable solution exists for this right now. :( In node, you'd have to use domains, which are being deprecated (though an alternate solution doesn't yet exist) That's what I used for https://github.com/danielstjules/mocha.parallel#error-handling

AFAIK, nothing similar exists in the browser. https://github.com/bevry/domain-browser is more of a sham than a shim, since it exposes a similar API but doesn't handle async errors.

Sorry for inconvenience, I hope it wasn't too difficult to debug. In those situations, I usually hope the stacktrace points me in the right direction.

@osher
Copy link
Author

osher commented Sep 13, 2015

Well. Doesn't make sense to me:
done function, wither it's called once or more, is passed to a test function from a closure - one that remembers how many times is was called.
How about adding information about the function a done callback is passed to?

@danielstjules danielstjules reopened this Sep 13, 2015
@danielstjules
Copy link
Contributor

Sorry about that, that's what I get for closing issues when tired :) I must have been thinking about exceptions being thrown async, rather than the callback. Could you provide a small example? Wasn't able to reproduce so far with 2.3.0:

$ cat test.js
describe('suite', function() {
  it('test1', function(done) {
    done();
    setTimeout(done, 50);
  });

  it('test2', function(done) {
    setTimeout(done, 100);
  });
});

$ mocha test.js


  suite
     test1
    1) test1
     test2 (102ms)


  2 passing (108ms)
  1 failing

  1) suite test1:
     Error: done() called multiple times
$ cat test.js
describe('suite', function() {
  before(function(done) {
    setTimeout(done, 30);
    setTimeout(done, 60);
  });

  it('test', function(done) {
    setTimeout(done, 100);
  });
});

$ mocha test.js


  suite
    1) test


  0 passing (67ms)
  1 failing

  1) suite test:
     Uncaught Error: done() called multiple times

@danielstjules
Copy link
Contributor

With your reporter:

$ cat test.js
module.exports = { Example: {
    beforeAll: function(done) {
      this.slow(110);
      setTimeout(done, 50);
      setTimeout(done, 100);
    },
    'test 1': function(done) {
      setTimeout(done, 200);
    }
  }
}

$ mocha --ui mocha-ui-exports test.js


  Example
    1) test 1


  0 passing (116ms)
  1 failing

  1) Example test 1:
     Uncaught Error: done() called multiple times
      at Array.forEach (native)
      at node.js:951:3

I think I get it now. When invoked multiple times from a hook, it attributes it to the next test, rather than the hook. It works correctly when invoked multiple times from a spec.

dasilvacontin added a commit that referenced this issue Sep 14, 2015
Fix #1798: Correctly attribute mutiple done err with hooks
kevinawoo added a commit to kevinawoo/mocha that referenced this issue Oct 6, 2015
* master: (25 commits)
  Fix mochajs#1798: Correctly attribute mutiple done err with hooks
  Remove redundant harmony flag
  Cast non-string return values from err.inspect()
  Call the inspect() function if message is not set
  Revert jade to support npm < v1.3.7
  Fix eqeqeq linting errors from eslint 1.4.0 release
  Fix mochajs#1669: catch uncaught errors outside test suite execution
  Support all harmony flags
  Fix fragile xunit reporter spec
  Fix linter warning "Expected a conditional expression and instead saw an assignment"
  Stackfilter fix: Don't remove modules/components from stack trace in the browser
  Fix mochajs#1864: xunit missing output with --reporter-options output
  Fix 1875: Markdown reporter exceeds maximum call stack size
  IE<=8 no [].reduce, so use 'utils.reduce' instead
  Release v2.3.2
  remove lodash.create; closes mochajs#1868
  update package.json & component.json for v2.3.1
  update HISTORY.md
  fix package.json to use exact version of lodash; closes mochajs#1867
  Fix: Bail flag causes before() hooks to be run even after a failure
  ...

# Conflicts:
#	lib/runnable.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants