Expanded Timeout Error Messages

[RobertWHurst]

The timeout error now provides feedback about how the test timed out, and how Mocha expected the test to yield. The purpose of this change is to aid developers when debugging timeouts, especially the case where both done() is requested and a promise is returned. It also removes "Resolution method is overspecified" error as it had a similar effect, but could introduce problems when used with CoffeeScript or async functions from ES7.

The error messages are perhaps rough copy. I think the maintainers a best suited to re-word the errors if they see it as necessary. Add you suggestions below and I'll update them. That goes for the code too of course.

The following tests:

describe('Test timeouts', () => {

    it('Async test timeout with done()', (done) => {});

    it('Async test timeout with done() and promise', (done) => {
        return new Promise(() => {});
    });

    it('Async test timeout with promise', () => {
        return new Promise(() => {});
    });
});

Results in the following test output:

  !!!

  0 passing (615ms)
  3 failing

  1) Test timeouts Async test timeout with done():
     Error: timeout of 200ms exceeded. Expected done() callback to be executed but it never was.
      at null.<anonymous> (lib/runnable.js:244:19)

  2) Test timeouts Async test timeout with done() and promise:
     Error: timeout of 200ms exceeded. Expected done() callback to be executed but it never was. A Promise was returned, but ignored because done() was requested. Remove done() from your test arguments to use the promise instead.
      at null.<anonymous> (lib/runnable.js:244:19)

  3) Test timeouts Async test timeout with promise:
     Error: timeout of 200ms exceeded. Expected the returned promise to resolve but it never did.
      at null.<anonymous> (lib/runnable.js:244:19)

cc: @dasilvacontin, @boneskull

🍻

[RobertWHurst]

@dasilvacontin Any thoughts? I'm curious what you think of this approach.

[dasilvacontin]

I personally prefer when code stuff is surrounded with backticks. So Promise -> Promise, and done() -> done (I don't think the parenthesis are necessary).

Also, given that this is a common error, we should consider adding an url/link to a Wiki page with some examples, and behaviour explanations, specially once we solve doubts at #1320.

[dasilvacontin]

Also, we will not remove:

  if (result && utils.isPromise(result)) {      
    return done(new Error('Resolution method is overspecified. Specify a callback *or* return a Promise; not both.'));      
  }

until we make a decision about the behaviour.

Thanks for kickstarting the improved error messages. :)

[RobertWHurst]

@dasilvacontin Sounds good. I'll split the PR (timeout messages vs yielding) and make the changes to the error messages. 👍

[boneskull]

Revert

[boneskull]

Once L368-L370 is reverted, will the logic here need to change?

[boneskull]

This appears to address #2521, in theory