add --overspec option to disable promise checking on tests that take a callback

[sixcircuit]

Great library. Thanks a ton.

Description of the Change

Currently if you take a done callback in a test and also return a promise, even if the test passes you get an error "Overspecified test, return a callback or a promise, not both."

This change adds an option (and relevant test coverage) to turn off the error. The option is off by default (leaving the status quo) and gives more sophisticated users an option to avoid what's basically protection for people who may make a mistake.

This is particularly a hassle because if you mark a function as async you can't avoid returning a promise (see below). If you're migrating a legacy codebase, this mix is inevitable, and the error stops you from doing perfectly reasonable, safe and legal things, in order to protect people from themselves. Which is a good idea, but I'd like the option to turn it off. It feels like a situation where an option or a warning would probably have been preferable in the first place.

Built it for myself, figured it may help others. Happy to make changes if you want them. Also cool if this doesn't fit the philosophy for some reason.

test('some test', async function(done){
  const foo = await asyncFunction();
  
  someOtherAsync(foo, function(err, bar){
    if(err){ return done(err); }
    assert(bar);
    done();
  });
});

Alternate Designs

I thought about arguing that anyone taking a done callback knows what they're doing, and throwing an error to try to protect them from themselves hurts more sophisticated users. I was going to pull out the error altogether. Breaking existing functionality seemed like a bad idea, so I figured an option that was off by default leaves the status quo, and gives experts an option to override the "warning" and probably doesn't ruffle too many feathers.

I could have turned it on by default (bad idea breaking existing functionality). I could have not built it at all (not really feasible for my use case).

Why should this be in core?

Can't avoid it, it's integral to how the tests are run. You throw an error on tests that would otherwise pass, any function that is marked async will need to be rewritten. You're hurting expert users who know what they're doing.

I'm giving people who know what they're doing the option to avoid an error (that probably should have been a warning or an option in the first place).

Benefits

Allows tests that take a done callback to be marked async and still run. This switch is off by default so it doesn't change existing functionality. Allows people who know what they're doing to make use of more elegant test patterns. Allows for projects to be migrated slowly to async/await without having to rewrite all the tests at once.

Very helpful in large codebase legacy scenarios (like mine).

Possible Drawbacks

Slight increase in maintenance going forward. May not get a ton of use.

Applicable issues

Patch release. Totally backwards compatible and off by default.

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage increased (+0.1%) to 92.838% when pulling e3b9faf on curious-inc:feature/overspec into 9584202 on mochajs:master.

[boneskull]

Thanks for the PR!

The language used in the message is... not the whole story. The issue is that Mocha has no idea when your test is finished.

That said, your particular example can be addressed by consuming util.promisify or just doing it manually:

test('some test', async function() {
  const foo = await asyncFunction();
  
  return new Promise((resolve, reject) => {
    someOtherAsync(foo, (err, bar) => {
      if(err){ return reject(err); }
      assert(bar); // also rejects upon failure
      resolve();
  });
});

This is inconvenient, but disabling the error message is not the "fix"--the fix would be for Mocha to understand that it needs to finish the test when both a) done is called and b) the returned Promise is fulfilled--and considering done(err) OR a rejected Promise to be a failure.

That said, I have yet to see an example of a case that has no workaround. async event emitter listeners seem to be the most awkward to deal with...