Error: Resolution method is overspecified.

[teckays]

This:

before(done => {
    return Promise
        .all([])
        .then(() => Model.insert(...)) // Bookshelf model returning a Bluebird Promise
        .then(() => done())
        .catch(done)
})

will result in an error Error: Resolution method is overspecified. Specify a callback *or* return a Promise; not both.

Docs say:

In Mocha v3.0.0 and newer, returning a Promise and calling done() will result in an exception, as this is generally a mistake:

The model call is resolving with a Promise.<Object> of the newly inserted entry, however if I omit .then(() => done()) then the test timeouts.

[elado]

async functions (babel) with done also break.

[papandreou]

As the error states, you're not supposed to provide a function with an arity of >0 (meaning that it's accepting a done callback), and return a Promise.

The easiest way to fix it would be to omit the return, but since you're using promises I suggest getting rid of the done callback instead, as it'll result in a much simpler construct:

before(() => Promise.all([]).then(() => Model.insert(...)));

[elado]

Here's an example of both async (essentially a promise) and done that breaks:

it('fires change event when calling blah.doSomethingThatFiresChange', async function (done) {
  const blah = await getBlah()
  blah.on('change', () => done())
  blah.doSomethingThatFiresChange()
})

[papandreou]

@elado That's an interesting use case, although from mocha's point of view it's the same situation -- a function that both takes a done callback and returns a promise. I'd rewrite it to be fully promise-based:

it('fires change event when calling blah.doSomethingThatFiresChange', async function () {
  const blah = await getBlah()
  return new Promise(resolve => {
    blah.on('change', resolve)
    blah.doSomethingThatFiresChange()
  })
})

... but I guess what you're getting at is that in this example, it would be better if mocha waited for both the promise to be resolved and the callback to be called?

Unfortunately that particular combo is most often a bug in the test, which is why this error message was added in the first place.

[ScottFreeCode]

...if I omit .then(() => done()) then the test timeouts.

This seems like a bug, in any case.

[papandreou]

@ScottFreeCode Hmm, yeah, it seems to be because the "overspecified" error is issued in the function provided as the done callback: https://github.com/mochajs/mocha/blob/4944e31ff60105815f4b314996a9861e73f6bfd2/lib/runnable.js#L357-L373

... but we can of course determine that we have to fail with that error as soon as the function has returned.

[boneskull]

@ScottFreeCode What's the fix here?

[boneskull]

I'm going to guess it's "wait for the promise to resolve or reject, then reject with the 'overspecified' error"?

[ScottFreeCode]

If we're going to consider done plus promise in the same test an error, I don't see any reason not to detect the error as soon as a test function that took done returns a promise, as @papandreou suggested. It doesn't make a lot of sense to me to try to figure out what other points should trigger the error unless we intend to allow promises and done together in some cases.

[boneskull]

@ScottFreeCode I'm in agreement. So it's

1. Detect problem; instantiate the Error but do not call done() with it
2. Wait until Promise fulfillment
3. Reject with the Error

Bonus question: What to do with the result of the Promise fulfillment?

[ScottFreeCode]

Ah, I think I get it -- even when we detect the error, we need to let the test run so it's not still running its code during the next test and, maybe, so we can also report the test result.

Could a test also end up calling done without resolving or rejecting the promise? If so, that's another end case we'll have to handle.

My inclination, re. what to do with the result of the test, is that if it times out (without having either called done or resolved/rejected the promise) we should just report the use of done with the promise (because confusion over the combination could very well be why it never got to either one and timed out instead), but if it does manage to finish by either means then presumably the result is valid (or at least somehow meaningful) and we should report both the possibly erroneous use of done and promise together and also the test result in hope that it's at least somewhat helpful.

That's the best I can come up with at the moment, anyway, but I might have more insight if I can find the time to dig into this problem.

[boneskull]

Well, we can do this in phases. The first would be to ensure that if a Promise is returned and a done callback is given, then Mocha will break in a friendly manner.

It's conceivable that one could do something like this (with Bluebird):

// to make this work, you need to omit `return`
it('should do something async for sure', function(done) {
  return somethingAsync()
    .then(makeAssertion)
    .asCallback(done);
});

But that's just something you could do; I have yet to determine a use case for this.

[boneskull]

I think I've confused myself. I'm not sure there's a bug here at all.

[boneskull]

This is more of a "poor UI"-type issue

[ScottFreeCode]

Timing out waiting for done to be called even though a returned promise gets resolved/rejected is definitely a bug, regardless of whether we want to disallow such tests from using done and promises together in the first place. This should either use the result of the promise and/or error because the promise and done were both utilized in the same test, but not just time out because one of the two never completed when the other one did (which is what it does currently):

it("should not time out", function(done) {
  return new Promise(function(resolve, reject) {
    setTimeout(resolve.bind(42), 50)
  })
})

1) should not time out: Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

I'll look at the PR in any case...

[boneskull]

Interested parties should watch #2413.

@briansipple

[ianaya89]

I get the same error using multiple async beforeEach hooks in version 3:

  beforeEach((cb) => connection.db.collection('users').remove({}, cb));
  beforeEach((cb) => connection.db.collection('accounts').remove({}, cb));
  beforeEach((cb) => connection.db.collection('accounts').insertOne(account1, cb));

Is not possible to still using async hooks in that way?

[ianaya89]

I figured out my issue: my methods inside each hook are returning a promise (I didn't know that because I was not using promises in those cases) and obviously I am also passing cb function.

[RobertWHurst]

I don't think this is a good idea. If one requests a callback I think they have made their intentions clear. The error here is just an annoyance. Please remove it.

[boneskull]

This isn't really a matter of intention. From #1320's description:

When both a callback function is specified and a Promise object is returned, the Runnable's resolution condition is ambiguous.

[RobertWHurst]

It's not ambiguous, the callback was requested. How is that ambiguous?

[ianaya89]

I am agree with that @RobertWHurst there is no ambiguous here.

Besides that I think this issue could be something "opinion based" and developers will have different point of views. I consider that is very extremist to do a breaking change and force people use that way.

[boneskull]

We didn't force anyone to do anything. We released a major, which by definition will have breaking changes. We didn't break semver.

[RobertWHurst]

I don't think he means you broke semver, I think he means you broke Mocha. This change doesn't make things easier for developers, it's enforcing an opinion.

beforeEach((cb) => connection.db.collection('users').remove({}, cb));
            ^^

^^ That is not ambiguous. It's pretty clear what is expected by the author. The author has gone out of their way to request a callback.

[Munter]

Actually the reason you get an exception here is the maximum avoidance of enforcing opinion. Mocha does not have an opinion on wether the returned promise or the callback are authoritative. You can't have both at the same time since that leads to ambiguous results. Ambiguous results in a test framework should be considered an error. Hence the error message to help you be aware of this and make the choice and change matching your opinion.

It might be beneficial to dial down the amount of drama. "you broke Mocha" is not helping anyone. A semver major version increase is explicitly defined as breaking changes that might require you to adjust your code. You can stay on 2.x to give you time to make the changes to fix your tests. This is an evolution, not a breakage

[RobertWHurst]

@Munter I'm still at a loss as to how requesting a callback is ambiguous. If you ask for a callback, you're expected to use it. Otherwise it's a programming error. This is an explicit action by the test author.

If you're sensing drama, I mean none. "you broke Mocha" is not meant to be hyperbolic. I just think this goes against the design of the module and breaks the original api contract.

[artyomtrityak]

As meintoned before babel async/await does not work well with new mocha@3. Example:

it('should fill userName and password', async function (done) {
    const userNameField = global.driver.wait(until.elementLocated({css: '#username'}));
    userNameField.sendKeys('admin');

    const passNameField = global.driver.findElement({css: '#password'});
    passNameField.sendKeys('*******');

    const userNameVal = await userNameField.getAttribute('value');
    const passwordVal = await passNameField.getAttribute('value');

    try {
      assert.equal(userNameVal, 'admin');
      assert.equal(passwordVal, 'changeme');
    } catch (error) {
      done(error);
      return;
    }

    done();
  });

This code works well with mocha@2 but does not with mocha@3 because inawait it returns promise

[RobertWHurst]

I think this PR is relevant here => https://github.com/mochajs/mocha/pull/2413 More complexity to deal with the edge cases of this error.

[boneskull]

@artyomtrityak That's a great example of where done is unnecessary.

[boneskull]

The detractors have said their piece. Yet, the maintainers of Mocha disagree with the argument(s) for reverting this change. Eran Hammer said (paraphrased), "As a maintainer, one of the hardest things you can do is dissappoint those who want to move the work in their direction."

I'm welcome to workarounds--more documentation (e.g. more examples of this error and how to fix them), better error messaging--but am not interested in drama, rudeness, or complaining. Contributing any of these workarounds to Mocha would help turn a negative into a positive.

If you don't like this change, and simply can't be constructive about it, it's OSS--you may fork the project and revert the changes there.

[artyomtrityak]

@boneskull it transforms to async functions which return promises, i do not need to finish my test case when promise will be fulfill but need to do some custom checks around results. As i said this code works perfectly fine with mocha@2 but with mocha@3 it does not. So my team (~20ppl) can not move to latest mocha because of this.

Currently mocha 2.x gives lot of flexibility, is there any technical reason for this change?

@artyomtrityak That's a great example of where done is unnecessary.

Can you please make an example how this should look like with babel async/await syntax and without return new Promise?

[papandreou]

@artyomtrityak How about embracing promises fully? Then you can shorten your test to:

it('should fill userName and password', async function () {
    const userNameField = global.driver.wait(until.elementLocated({css: '#username'}));
    userNameField.sendKeys('admin');

    const passNameField = global.driver.findElement({css: '#password'});
    passNameField.sendKeys('*******');

    const userNameVal = await userNameField.getAttribute('value');
    const passwordVal = await passNameField.getAttribute('value');

    assert.equal(userNameVal, 'admin');
    assert.equal(passwordVal, 'changeme');
  });

[RobertWHurst]

If you feel personally attacked, then I think you need to reconsider how emotionally invested you are in this conversation. None of the comments have been personal. I'm sure you're all great, especially considering you've donated your time to help maintain this project which is greatly appreciated and very commendable.

Most of you (the currently active maintainers) started working on Mocha around mid-2014. Mocha was already established by the point you guys started contributing. It's just my opinion, but I don't think I'm alone in thinking one shouldn't be making breaking changes to an established library unless it's justified. Though I can imagine the original justification for this change, it doesn't stand up well when one points out the following. Asking for a callback communicates a clear intention. Promises are not as clear because they are not requested, they are returned, which can happen indirectly and accidentally (returned from a 3rd party library for example). Because of these differences the two ways of yielding are not equal, and thus trying to use both is not really ambiguous. Callbacks must be written into the test arguments. You can't do that with promises, and so by asking for a callback, you've communicated your intentions explicitly. Your community raised these concerns, and instead of acknowledging the misstep you guys are doubling down. Seems you're even considering forcing tests to be async to ensure this error acts consistently. See => https://github.com/mochajs/mocha/pull/2413. Seems like a big change for an error message protecting against an unlikely mistake.

You guys have done a great job maintaining this library since @tj's departure, can you please think a bit more about this change. My concern is this could compromise the library.

[elado]

Totally agree with @RobertWHurst.

Requesting done should override the returned promise behavior. It is not likely to request done when it's not needed, and scenarios of emitted events in an async function are a perfect example.

From my comment above:

it('fires change event when calling blah.doSomethingThatFiresChange', async function (done) {
  const blah = await getBlah()
  blah.on('change', () => done())
  blah.doSomethingThatFiresChange()
})

As more people move to ES6/7+async/await, this will become a common gotcha when using Mocha.

Please reconsider this change.

[Munter]

@RobertWHurst You argue that defining a done callback is explicit intention. Is a return statement not an explicit intention? Both are defined in your code by you. How can we decide that one part of your code is intentional and another is not? If you imagine a world before () => foo any return statement would always have been explicit. The only reason you are all up in arms now is because you have started using implicit return statements, for what I can only think are aesthetical reasons.

Given that a lot of Mocha usage is by beginners who usually copy/paste examples, which very likely contain a done callback, how would you handle the case where this new user explicitly returns a promise, but get a timeout? This is the result if the change you are mad about gets reverted.

The current behavior is much more clear about what is wrong than just an unexpected timeout

[papandreou]

@Munter With async functions in the picture I think the returned promise scores lower on the explicitness scale because it's created and returned automatically, whether or not you use await:

it('should work with a async function that could have been sync', async function () {
  assert.ok(true);
});

it('should work with a legitimately async function', async function () {
  assert.equal(await getBlah(), 'blah');
});

it('should work with a fully spelled-out Promise-based test', function () {
  return getBlah().then(function (blah) {
    assert.equal(blah, 'blah');
  });
});

And then there's the controversial one:

it('should foo', async function (done) {
  assert.equal('foo', 'foo');
  done();
});

It's also easy for that teeny-weeny async to sneak in by accident, so we should think of (at least) the first example as a new kind of gotcha, as @elado points out.

[RobertWHurst]

After a bit of conversation over here => https://github.com/mochajs/mocha/pull/1320, I had an idea for an alternative solution to the problem. I've added a PR for your reviewing pleasure over here => https://github.com/mochajs/mocha/pull/2454

:beers:

[tandrewnichols]

You argue that defining a done callback is explicit intention. Is a return statement not an explicit intention?

@Munter Don't forget that coffeescript and es6 arrow function expressions return implicitly, so you can do something like

it 'should foo', (done) -> request('/foo').end (err, res, body) -> done()

and think you're safe. But this issues means you have to transform that nice one-liner to something like

it 'should foo', (done) ->
  request('/foo').end (err, res, body) -> done()
  return

[wdavidw]

This is exactly our problem with our overall codebase in each and mecano. Consider this:

it 'should foo', (done) ->
  obj =
    run: ->
      done()
      @
    then: -> "what the hell, i'm not a promise"
  obj.run()

This isn't completly specific to coffeescript but returning implicit values makes it worst. Mocha should detect valid promise instances. Also, maybe an option could disable this feature.

[mindvox]

Hey all, Ran into this issue with the following code;

describe('Page', ->
  describe('/GET home', ->
    it('it SHOULD return the homepage', (done) ->
      chai.request(app).get('/').end((err, res) ->
        res.should.have.status(200)
        res.text.should.be.a('string')
        done()
      )
    )
  )
)

Got this resolved by respecting the promise chain and omitting the callback done().

describe('Page', ->
  describe('/GET home', ->
    it('it SHOULD return the homepage', ->
      chai.request(app).get('/').then((res) ->
        res.should.have.status(200)
        res.text.should.be.a('string')
      )
    )
  )
)

I hope this helps others, who run into a similar error :smile:

P.S. :heart: Mocha btw :+1:

EDIT Remove catch() based on @Munter's comment.

[Munter]

@karlbateman Thanks. You don't need that last .catch though. A rejected promise is counted as an error by Mocha

[mindvox]

@Munter I see, thank you :smiley:

[factoidforrest]

Upon thinking about it a little more I really like the behavior of waiting for both the promise and the callback. Was any progress made with getting that implemented?

[dasilvacontin]

@light24bulbs Thanks for the feedback! No, afaik no one started working on that since it's just an idea I threw in there to see the reactions. I was just now checking if there had been any additional feedback on the idea, any cases in which it doesn't work, etc.

[amilajack]

Is there a workaround for this when using babel?

[saschanaz]

Is there a workaround for this when using babel?

I wrap twice:

it("should work", done => {
  (async () => {
    await something;
    done();
  })();
});

[SerkanSipahi]

@SaschaNaz thank you, this works in v3.2.0 :)

[benfavre]

6 months later, this is issue is still braking all modern js tests ... shame

[Munter]

@benfavre Thank you for the encouraging words that will most definitely motivate volunteers to take on the responsibility of implementing whatever solution you haven't specified in their free time rather than playing with their kids

[benfavre]

@Munter No worries glad I could Help identify a specific issue I faced as a new user of mochajs. @SaschaNaz suggested solution of wrapping twice did not help.

=> Using promises exclusively did work as it should.

Next time I guess i should just "+1" like a morron so I don't get insulted for free. There was nothing insulting in my message, furthurmore my statement remains true.

Most people will just choose another framework ... as of right now, it's just plain broken with async/await, with no clear indications anywhere on the main site, and no clear error message in the cli.

Happy new year and have fun playing with kids.