🚀 Feature: New test resolution behavior

[boneskull]

See #2413 and other various issues and PRs for previous discussion.

In Mocha 2.x, when a Promise is returned from an "async" test (using the done() callback), the Promise resolution is effectively ignored, though it may cause an unhandled rejection after the test has finished.

In Mocha 3.x to the present, returning a Promise from an "async" test is disallowed; tests must use either done() or return a Promise, but not both.

A better solution in this case would be to wait for both resolutions (the call of done() and Promise fulfillment) before the test can end.

Specification

When a test requests done() and returns a Promise:

• and when it has fulfilled before done() is called
  • and when the Promise has been resolved
    • the runner should wait until done() is called to conclude the test
    • and when done() is called with an error parameter
      • the test should fail
    • and when done() is called without an error parameter
      • the test should pass
  • and when the Promise has been rejected
    • the runner should not wait until done() is called to conclude the test
    • and when/if done() is eventually called, it should be ignored
    • the test should fail warning the user that if done() was called, any error parameter would have been ignored
• and when done() is called before it is fulfilled
  • and when done() is not called with an error parameter
    • the runner should wait until Promise fulfillment to conclude the test
    • and when the Promise has been rejected
      • the test should fail
    • and when the Promise has been resolved
      • the test should pass
  • and when done() is called with an error parameter
    • the runner should not wait until Promise fulfillment to conclude the test
    • and when/if the Promise fulfills, it should be ignored
    • the test should fail warning the user that if the Promise has been rejected, any error parameter would have been ignored

Normal timeout rules apply.

---

Comments, suggestions, questions, concerns?

[bitsoflogic]

I think it looks great! I think #2511 is related, since we're talking about how Mocha will respond with done and Promise. Not sure the answer to that belongs within this, but it may effect how the code is written.

[boneskull]

I'm tagging this major-release because it will affect the behavior of existing tests; some tests may newly fail or pass, depending.

[boneskull]

I'm not super excited to include it as a dependency, but asCallback and/or fromCallback would likely simplify the implementation.

[Munter]

If we add waiting for two async operations at the same time I think we need to improve the timeout handling as well. The timeout message should tell you which one of the async blockers timed out, or if both of them did. Otherwise it gets really hard to debug

[boneskull]

@Munter agree

[dasilvacontin]

Just copy/pasting this comment here:

---

Example of using both done and Promise when both are requested/given, catching an Error that would be missed otherwise:

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

  1) fires change event when calling blah.doSomethingThatFiresChange:

     Error: the `done` callback was successfully called, but the returned `Promise`
     was rejected with the following `TypeError`:

     TypeError: Cannot read property 'likes' of undefined
         at Blah.doSomethingThatFiresChange (lib/Blah.js:5:9)
         at Context.<anonymous> (test/Blah.spec.js:4:8)

     More info: https://github.com/mochajs/mocha/wiki/some-page

[tandrewnichols]

What happens if something other than a promise is returned?

[dasilvacontin]

@tandrewnichols It would get ignored, as it currently does.

[ScottFreeCode]

I actually want to weigh in against this; all of my ever-growing experience points toward mixing of promises and callbacks being worse than either one, and I think I just figured out why.

• Safer alternatives to be referenced later:
  • There are dedicated tools for converting callback APIs to promise APIs and vice versa easily and safely ("promisification").
  • Promises can easily safely branch and rejoin: Promise.all(basePromise.then(stuff), basePromise.then(otherStuff)) or ....then(value => Promise.all(stuff(value), otherStuff(value)))
    • Branching and rejoining callbacks is not as commonly familiar but is possible, the worst-case is that its unfamiliarity and lack of abstraction makes it more error prone than the promise equivalent.
• Outside of an API that rejoins a branch formed by a promise and a callback (such as the proposed Mocha behavior), which are not common, there are only two possible things that would happen in mixing promises and callbacks:
  • The callback is at the end of the promise chain. This is an ad-hoc and bug-prone equivalent of using one of the dedicated callback-promise bridges mentioned above. If and only if it is done correctly (pun not intended) in any given instance the behavior is correct; the practice is not correct, because it needlessly increases risk that it could be done incorrectly and result in incorrect behavior.
  • The callback is not at the end of the chain. This is worse. Much worse. In this case the promise chain is executing further actions and not communicating them to anything outside. This is equivalent to abandoning both callbacks and promises -- the one thing they have in common is communicating when (and with what result) an asynchronous action completes.
    • In the unlikely event that firing off an unconnected asynchronous side effect of some sort is correct behavior, developers can do so by explicitly leaving the returned promise chain -- and they should be required to be explicit, because it's not normally correct.
• So, what we've got here is the idea to use a design pattern -- mix of callbacks and promises -- that is otherwise always wrong (either "soft wrong" inasmuch as it's a crappy manual version of promisification, or a "very hard and nasty wrong" with async actions running amok without the basic sanity guarantees of even crude callback paradigms, or a "just weird wrong" where the behavior is intended but not being explicit about it makes it look like a paradigm mixup error), as a branching mechanism. In and of itself waiting for both branches could result in the desired behavior when it's intentional, but:
  • Even if we didn't know that mixing the paradigms is usually wrong, the complexity of the proposal alone suggests that such a design pattern is risky.
    • However, even if the design were simple, the fact is that it would be using, and encouraging developers to become used to writing, the same general pattern that is usually a bad idea (either erroneously or needlessly risky of error) anywhere else.
  • There's a curious similarity to the case of callback at the end of the promise chain, wherein the "correct" usage is identical to an alternative that doesn't have such risk of incorrect usage: in the best-case scenario that we correctly guide users to use a callback and a promise as a branch that Mocha correctly rejoins afterward rather than leaving either "dangling", we've just recreated Promise.all only with more associated risks.
    • (I don't know if the equivalent callback-based branching is as safe as Promise.all or as dangerous as mixing callbacks and promises, but clearly developers who are unwilling to use promises wouldn't be using the callback+promise branch approach either, so anyone considering the callback+promise branch approach as easier or safer than pure callback branching shouldn't have objections in principle to using the safe and easy promise-only branching mechanism.)
  • And finally, in a similarity to the case of sending off asynchronous actions that you won't wait for: In the unlikely event that a developer really does have a good (or at least better than the alternatives) case for using a promise and a callback as a branch, surely they can write that mechanism into their test explicitly (and have the resulting output go to Mocha through just the one or the other), which has the advantage of being clear that they really meant to do it and of not introducing all that risk to any other Mocha users.

(NB: to the extent that async functions are just nicer-looking promise chains, presumably the same logic applies. For instance, in the above example of passing Mocha's callback to a callback API that's used inside an async test function, I would create a promise from the callback API in order to either await it in or return it from the async function instead -- and if const wasCallback = new Promise(resolve => blah.on('change', resolve)) isn't clean enough, use a dedicated promisification tool.)

TL;DR: It's wrong except when it's "right" in the sense of being identical in effect to a superior alternative that doesn't have the high probability of being wrong. I realize that's an unusually categorical judgement for programming, art of tradeoffs, but I am fairly sure the logic above guarantees it. Strong opinions weakly held and all that -- if there's a flaw in the logic I'll gladly reconsider either the logic or my stance on mixing these paradigms. As always, just my two cents' worth. 😉

[dasilvacontin]

@ScottFreeCode I've finally read through your message, but I either don't understand it or I disagree.

In my head, the solution would be like, okay, I'm the runner, I've got the callback cb that I've passed, and through executing the text I've been returned a Promise testP. The callback was defined somewhat like:

let callback
const callbackP = new Promise((resolve, reject) => {
  callback = function (err) {
    if (err) reject(err)
    else resolve()
  }
}

And then we merge both promises to know when the whole thing ends:

const wholeThingP = Promise.all(callbackP, testP)

Whenever wholeThingP is fulfilled, we're done. Whenever wholeThingP is rejected, we're done.
Whenever we are done in a failed way, or the 2000 timeout occurs, inspect both promises to log their status: pending, fulfilled, resolved.

I don't feel like the "there's risk in doing this" argument is justified. Most likely because it doesn't feel complicated to me, and/or the solution I have in mind is different.

Feels nice to be engaging in the community after so long! 😊

[boneskull]

@dasilvacontin scott's long gone unfortunately

[JoshuaKGoldberg]

Now that async/await is popular & long-supported in all LTS versions of Node, the original motivation behind #2407 isn't really strong anymore. The answer we'd probably give is to use async/await with try/catch. Also, per #5027, we're trying to avoid big changes. I'm going to close this out as wontfix.

If anybody has a good reason to change this core behavior, please do file an issue with one of our new issue templates. We'd be happy to take a look. Cheers all! ❤️