Error thrown between it clauses disappears

[javinor]

Prerequisites

• [x ] Checked that your issue isn't already filed by cross referencing issues with the common mistake label
• [ x] Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• [x ] 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• [x ] Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with:
  node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend avoiding the use of globally installed Mocha.

Description

Steps to Reproduce

git clone https://github.com/javinor/mocha-swallows-error
cd mocha-swallows-error
npm install
npm run test

Expected behavior:
Should get some indication to the error thrown from within the unhandledRejection error handler, preferably an exception and stopping the tests.

Actual behavior:
I appears that everything works fine

Reproduces how often:
100%

Versions

• OS X Sierra 10.12.6
• node 8.7.0
• mocha 4.0.1

Additional Information

In our large test suite, we catch unhandledRejections since they .shouldn't happen. I expected throwing an error would somehow get propagated so that tests will fail, with an informative message/error.

[Munter]

Source code, for the lazy ones (like me):

"use strict"

process.on('unhandledRejection', () => {
    console.log('this happens before AFTER')
    throw new Error('tsk, tsk, tsk...')
})

describe('errors thrown in event handler', function () {
    it('should let me know something went wrong', function (done) {
        new Promise((res, rej) => {
            setTimeout(() => {
                Promise.reject() 
                res()
                done()
            }, 1000)
        })
    })
    after(() => console.log('In AFTER: Should not get here?'))
})

[Munter]

I'm not quite sure what the intent of the test case is.

• You create a promise which you don't return, not using mochas built-in support for handling promises correctly
• After one second you have a statement that creates a new rejected promise and does nothing with it.
• You resolve the original promise, which you never returned, so it has no effect
• You call done, passing the test.

From my perspective mocha is doing what it is supposed to. If you remove the line that creates the unhandled rejected promise, things should work as you probably intended. I would recommend returning the first promise and dropping the done callback concept

[ORESoftware]

+1

[ScottFreeCode]

We have #2640 for Mocha failing on unhandled rejections, but it may not make a difference with this issue.

The example is pretty much equivalent to:

process.on('unhandledRejection', message => {
    throw new Error(message)
})

it('should let me know something went wrong', function () {
    Promise.reject('foobar')
})

Whereas this correctly fails the test:

process.on('unhandledRejection', message => {
    throw new Error(message)
})

it('should let me know something went wrong', function (done) {
    Promise.reject('foobar')
    setTimeout(done)
})

One key element here is that Promise.reject is actually async; this also does not report the error:

it('should let me know something went wrong', function () {
    setTimeout(function () { throw new Error('foobar') })
})

...whereas this does:

it('should let me know something went wrong', function (done) {
    setTimeout(function () { throw new Error('foobar') })
    setTimeout(done)
})

So it's probably not a promise issue specifically.

---

Typically, if the error occurs long enough after the test ends and Mocha has started running other tests or hooks, the error is misattributed (perhaps unavoidably; determining the origin of async errors is hard) to the currently running one: #967

In this case, I'd guess that it's still associated with the test, but that it's being suppressed once the test has passed; see discussion following #2890 (comment) re. suppressing errors after the test reaches some state.

---

P.S.

'In AFTER: Should not get here?'

after should run whether the test fails or not. (Although it may help narrow down what's happening to know that the log in the rejection handler does fire before the log in the after hook, i.e. the rejection is processed before the next test or hook.)

[javinor]

@Munter I'm sorry if my example was too watered down. Our application has a lot of async communication between a variety of services (servers & webworkers). Specifically I encountered a situation where an error was thrown between it clauses.

Mocha caught the exception and did nothing with it, even with the --allow-uncaught flag, which surprised me.

I think a better example is what @ScottFreeCode (thanks!) presented:

process.on('unhandledRejection', message => {
    throw new Error(message)
})

it('should let me know something went wrong', function () {
    Promise.reject('foobar')
})

I think that whether the rejection happens in the test, or somewhere down the code chain, and Mocha caught it, that tests shouldn't just pass without any notification that something bad happened. Please note that this is applies for exceptions between it clauses as well as rejections.

I hope this makes the issue clearer, and thanks for the fast response!

[boneskull]

I don't think this is actionable. Please comment if disagree

[javinor]

@boneskull, I think it is actionable. When running a tests, I'd expect not only all tests to pass, but also for no unhandled rejections (or exceptions for that matter) to be occur. I think that getting an error message at the end of the test suite stating "Yo! You have unhandled rejections. We don't know where they came from, but this probably shouldn't have happened". Maybe add a flag to allow suppressing this message. wdyt?
If you agree, I'd be more than happy to open a PR myself. I'm not sure where would be a good place to start. Can you point me in the general direction?

[boneskull]

@javinor I should have been more clear; this is more-or-less a duplicate of #2640. Feel free to send a PR there. I'll post more in the issue