Misleading source info in "done() called multiple times" message

[scy]

The error message "done() called multiple times" will be associated with whatever part of the test suite that's currently running. This can lead to a lot of confusion for the uninitiated developer.

For example, consider the following suite:

describe("a simple test suite", function () {
    beforeEach(function (done) {
        setTimeout(done, 150);
    });
    afterEach(function (done) {
        setTimeout(done, 50);
        setTimeout(done, 100);
    });
    it("has a simple test", function (done) {
        setTimeout(done, 100);
    });
    it("has another simple test", function (done) {
        setTimeout(done, 100);
    });
});

Running mocha done-message.js will result in:

  ․․

  1 passing (356ms)
  1 failing

  1) a simple test suite "before each" hook:
     Uncaught Error: done() called multiple times
      at multiple (/usr/local/lib/node_modules/mocha/lib/runnable.js:175:31)
      at done (/usr/local/lib/node_modules/mocha/lib/runnable.js:181:26)
      at null._onTimeout (/usr/local/lib/node_modules/mocha/lib/runnable.js:197:9)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

Note how the "done() called multiple times" will be listed having the beforeEach hook as the point where it came from. This is of course wrong, the beforeEach simply happens to be the active code part when the second done() call is being run. Additionally, the second test gets marked as a failure, even though it did nothing wrong!

I don't know Mocha's internals, so I don't know whether this can be fixed easily at all. Some ideas:

• Pass a new done callback to the tests each time, that has the source information as a closure variable?
• Change the error reporting so that the done error message is not associated to any code part at all?
• Make it clear in the error message that this message can literally come from everywhere?
• Make double-done-calling a fatal error that causes the entire suite to stop and tell the user that the second call to done can come from anywhere?

Tested with Mocha 1.15.1. This issue may or may not be related to #990.

---

More examples:

describe("a simple test suite", function () {
    beforeEach(function (done) {
        setTimeout(done, 150);
    });
    afterEach(function (done) {
        setTimeout(done, 200);
        done();
    });
    it("has a simple test", function (done) {
        setTimeout(done, 100);
    });
    it("has another simple test", function (done) {
        setTimeout(done, 100);
    });
});

This will cause the error to be associated to the second test.

---

describe("a simple test suite", function () {
    beforeEach(function (done) {
        setTimeout(done, 150);
    });
    afterEach(function (done) {
        done();
        done();
    });
    it("has a simple test", function (done) {
        setTimeout(done, 100);
    });
    it("has another simple test", function (done) {
        setTimeout(done, 100);
    });
});

This will result in no error at all, even though done gets called multiple times!

[raydemandforce]

I ran into the same issue. Will be nice mocha can fix this.

[danypype]

+1

[gardner]

Seeing this in version 1.19.0 as well. This does look to be the same issue as #990 but this report has simple code examples that demonstrate the problem. Perhaps @hallas won't dismiss this issue with obstinacy.

[gardner]

For me this turned out to be a library, sailsjs (based on express), that I was using calling its done callback multiple times.

Originally, my after() block looked like:

  after(function(done) {
    app.lower(done);
  });

I added in a check and arbitrary wait time:

after(function(done) {
  var done_called = false;
  app.lower(function() {
    if (!done_called) {
      done_called = true;
      setTimeout(function() {
          sails.log.debug("inside app.lower, callback not called yet. calling.");
          done();
      }, 1000);
    } else {
      sails.log.debug("inside app.lower, callback already called.");
    }
  });
});

This is not perfect but it allows me to call done once and use the watch feature of mocha.

[gardner]

Awesome! Thanks @boneskull

[craigvl]

@gardner I used your fix for the same issue in relation to sailsjs and it has fixed my issue but is this correct or should I have done something else? Thanks.

[misantronic]

I just stumbled into the same issue and wrote a little wrapper-class for the done-function:

/**
 * Wrapper-Class for done-function
 * @param {Function} fn
 * @class {Done}
 */
function Done(fn) {
    var self   = this;
    var called = false;

    /**
     *
     * @param {*} params...
     */
    this.trigger = function (params) {
        if(called) {
            console.warn("done has already been called");
            console.trace();

            return;
        }

        fn.apply(self, arguments);
        called = true;
    };
}

Usage:

it('should test', function (fn) {
    var doneWrap = new Done(fn);

    myAsyncFn(function () {
        // ...

        doneWrap.trigger();
    });
});

[adborden]

@gardner I'm seeing the same behavior with SailsJS, do you have a bug you can reference? I can't seem to find one.

[gardner]

There's a related mocha bug: #1066

I can't find a sailsjs bug. It looks like issues are closed without much investigation on that project.

[CodeJjang]

Still happening even today.

[boneskull]

@CodeJjang How so? Can you provide code? I want to know if this hasn't been completely fixed. Note that if you call done multiple times, you should get an error that done was called multiple times.

[CodeJjang]

@boneskull of course that if you call it multiple times then you should get the message.
I had a done call in a timer in the before (before all) hook, in this manner:

.then(setTimeout(done, 50))

[Munter]

@CodeJjang That setTimeout is evaluated immediately and not as part of the .then flow. No wonder that does weird things

[CodeJjang]

Yep, indeed my bad.
changed to

.then(() => {
                setTimeout(done, 4000);
                return Promise.resolve();
            })

and worked.

[Munter]

Using done and promise resolution in the same flow also seems wierd. You could just return your promise in your hook. That works equally well, without a done callback alltogether