🚀 Feature: Fatal error when async functions are passed to describe and context

[alcuadrado]

Is your feature request related to a problem or a nice-to-have?? Please describe.

I'm one of the maintainers of a popular tool that uses Mocha to run our users' tests. We get periodic reports about our tool not running any test (example), and in many cases, this is because the user is passing async functions to describe.

Describe the solution you'd like

I think throwing an error if executing the callback received by describe/context (I may be missing others) returns a Promise, explaining that the callback should be synchronous would be beneficial to Mocha's users.

Describe alternatives you've considered

For typescript users, we could partially achieve this by modifying @types/mocha so that the functions I mention stop accepting async functions, but that would be a partial solution, as not everyone uses typescript.

Additional context

This is a screenshot of a modified version of Mocha that behaves as proposed.

I can send a PR if there's interest in this feature.

[juergba]

@alcuadrado Yes, you are correct. This is a known trap user occasionally get squashed in.

A PR would be appreciated. I think we already had a fix for this issue which had been reverted for some reason. Please investigate before opening an PR. Thank you.

[alcuadrado]

Thanks for your answer, @juergba!

I think we already had a fix for this issue which had been reverted for some reason. Please investigate before opening an PR. Thank you.

I did some research, and you are right. A similar feature was implemented in v6.0.0, where a warning was printed if Suites returned anything other than undefined.

It was then discovered to be inconvenient for CoffeeScript users, as this pattern started getting a warning

describe 'My Suite', () ->
  it 'run a test', () ->
    console.log 'yes'

which lead to this warning being removed in this PR.

I think my proposal doesn't have that effect, as it checks if the value returned by the Suite is a Promise. In fact, this was proposed as an alternative fix here.

The only other difference between my proposal and the previous implementation is that I proposed throwing, and the previous implementation printed a warning. It was decided to warn to avoid introducing a breaking change, and then changing it to throwing in the next major here. Which I think is a good idea.

[JoshuaKGoldberg]

inconvenient for CoffeeScript users

Heh. CoffeeScript.

Coming back a few years later: I think we can stop considering that language a high priority support target at this point. 🙂