Unhandled rejections cause test script exit

[mdr]

We're getting an error with one of our tests in our CRA app. Our test looks a bit like the following:

import React from 'react'
import { shallow } from 'enzyme'

const flushPromises = () => new Promise(resolve => setImmediate(resolve))

const fetchTheData = jest.fn()

class MyContainerComponent extends React.Component {
  state = { error: null }

  async componentDidMount() {
    try {
      await fetchTheData()
    } catch (e) {
      this.setState({ error: 'There was a problem' })
      throw e // rethrow so it gets handled by a generic unhandled promise rejection handler (e.g. Sentry)
    }
  }

  render() {
    return <p>{this.state.error}</p>
  }
}

it('should handle errors calling the API', async () => {
  fetchTheData.mockImplementation(() => Promise.reject(new Error('something bad happened')))

  const component = shallow(<MyContainerComponent />)

  await flushPromises()
  expect(component.update()).toIncludeText('There was a problem')
})

However, it blows up on npm test with:

RUNS  src/App.test.js
/Users/matt/misc-repos/promise-rejection-oddity/node_modules/react-scripts/scripts/test.js:20
  throw err;
  ^

Error: something bad happened
    at fetchTheData.mockImplementation (/Users/matt/misc-repos/promise-rejection-oddity/src/App.test.js:26:56)
    at mockConstructor (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-mock/build/index.js:288:37)
    at MyContainerComponent.componentDidMount (/Users/matt/misc-repos/promise-rejection-oddity/src/App.test.js:13:13)
    at /Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme/build/ShallowWrapper.js:126:20
    at Object.batchedUpdates (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme-adapter-react-16/build/ReactSixteenAdapter.js:342:22)
    at new ShallowWrapper (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme/build/ShallowWrapper.js:125:24)
    at shallow (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme/build/shallow.js:19:10)
    at Object.<anonymous>.it (/Users/matt/misc-repos/promise-rejection-oddity/src/App.test.js:28:41)
    at Object.asyncFn (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-jasmine2/build/jasmine-async.js:68:30)
    at resolve (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-jasmine2/build/queueRunner.js:38:12)
    at new Promise (<anonymous>)
    at mapper (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-jasmine2/build/queueRunner.js:31:21)
    at Promise.resolve.then.el (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/p-map/index.js:46:16)
npm ERR! Test failed.  See above for more details.

This behaviour appears to be because there is an unhandled rejection, and in react-scripts/scripts/test.js:

// Makes the script crash on unhandled rejections instead of silently
// ignoring them. In the future, promise rejections that are not handled will
// terminate the Node.js process with a non-zero exit code.
process.on('unhandledRejection', err => {
  throw err;
});

So, I guess I wanted to understand whether what we're trying to do in our test is bad -- we have an unhandled rejection, but that's intentional, and not a problem in the browser environment (we have a global error handler that will deal with it). Is CRA being overly enthusiastic in causing the script to crash in these cases? Should we be dealing with things in a different way?

The other oddity is that most of the time, when all the tests in our actual app's test suite are run together, the test passes. It's only when run in isolation that we trigger the unhandledRejection event listener and the script exits. Any ideas on why that might be the case?

[Timer]

I assume the test result might be cached and being skipped, thus you're not seeing the failure when you're running all tests -- or there might be a bug in jest obscuring this issue.

Nevertheless, this appears like it should be a handled rejection. You're handling it by awaiting the promise AFAIK, unless if there's a tricky nuance happening here. I'm not sure why it's complaining that it's unhandled.

Please fill out the issue template so we have all the necessary (and relevant) information to diagnose this further.

[mdr]

OK, I'm happy to go off and fill in the issue template, but I had come to the conclusion that this was indeed an unhandled rejection -- Enzyme will call componentDidMount which will result in a promise, but nothing is attaching a rejection handler to that promise as far as I can tell. The test will await flushPromises(), but that's waiting until after a setImmediate, which indirectly ensures that any outstanding promise processing is completed by virtue of the event loop, rather than directly chaining on those promises.

That's my understanding anyway, could well be incorrect! But if I'm right, that's not a bug with CRA, and my question was really whether there was a recommended way of writing the above test in CRA, particularly given that it's especially rigorous about unhandled rejections.

[mdr]

Is this a bug report?

More of a discussion/question really

Did you try recovering your dependencies?

Yes

Which terms did you search for in User Guide?

Unhandled rejection

Environment

Environment:
  OS:  macOS High Sierra 10.13.3
  Node:  10.1.0
  Yarn:  1.6.0
  npm:  5.6.0
  Watchman:  4.9.0
  Xcode:  Not Found
  Android Studio:  Not Found

Packages: (wanted => installed)
  react: ^16.4.1 => 16.4.1
  react-dom: ^16.4.1 => 16.4.1
  react-scripts: 1.1.4 => 1.1.4

Steps to Reproduce

(Write your steps here:)

1. Run npx create-react-app
2. Paste in the test code above into App.test.js
3. yarn test

Expected Behavior (and Actual Behavior)

RUNS  src/App.test.js
/Users/matt/misc-repos/promise-rejection-oddity/node_modules/react-scripts/scripts/test.js:20
  throw err;
  ^

Error: something bad happened
    at fetchTheData.mockImplementation (/Users/matt/misc-repos/promise-rejection-oddity/src/App.test.js:26:56)
    at mockConstructor (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-mock/build/index.js:288:37)
    at MyContainerComponent.componentDidMount (/Users/matt/misc-repos/promise-rejection-oddity/src/App.test.js:13:13)
    at /Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme/build/ShallowWrapper.js:126:20
    at Object.batchedUpdates (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme-adapter-react-16/build/ReactSixteenAdapter.js:342:22)
    at new ShallowWrapper (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme/build/ShallowWrapper.js:125:24)
    at shallow (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/enzyme/build/shallow.js:19:10)
    at Object.<anonymous>.it (/Users/matt/misc-repos/promise-rejection-oddity/src/App.test.js:28:41)
    at Object.asyncFn (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-jasmine2/build/jasmine-async.js:68:30)
    at resolve (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-jasmine2/build/queueRunner.js:38:12)
    at new Promise (<anonymous>)
    at mapper (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/jest-jasmine2/build/queueRunner.js:31:21)
    at Promise.resolve.then.el (/Users/matt/misc-repos/promise-rejection-oddity/node_modules/p-map/index.js:46:16)
npm ERR! Test failed.  See above for more details.

[Timer]

Oh I'm sorry, I must not have looked closely enough -- I didn't see you are rethrowing the exception:

throw e // rethrow so it gets handled by a generic unhandled promise rejection handler (e.g. Sentry)

Then yes, per your original question this is the intended behavior because Node will crash anyway. We're just getting our users used to this earlier.

If you catch an error that you want Sentry to report, you should report that error manually.

Handling the error by reporting it to Sentry shouldn't stop your application from crashing, as you probably re-throw it there anyway.

Thank you so much for filling out the template though! I should've looked more closely. 😅

[mdr]

If you catch an error that you want Sentry to report, you should report that error manually.

In the browser environment, do we think that's better than just letting the promise "crash" and have it scooped up in a global handler? I kinda see an appeal in one unified place to report errors, rather than duplicating that at various places across the codebase. And it also plays nicely with the out-of-the box CRA error reporting in dev mode.

So yeah, I appreciate that Node is getting really picky about it, so there's not really much to be done, but it seems a bit unfortunate to have to choose an error handling strategy to appease where the tests are being run, rather than the production code. Are there other reasons to report errors manually?

[Timer]

In the browser environment, do we think that's better than just letting the promise "crash" and have it scooped up in a global handler? I kinda see an appeal in one unified place to report errors, rather than duplicating that at various places across the codebase. And it also plays nicely with the out-of-the box CRA error reporting in dev mode.

I agree centralizing the handling of exceptions is nice, but I think the best way to handle this would be an error boundary (because it's not really unhandled if you're handling).

Maybe toss an ErrorBoundary on the root of your index.js (and tests)? Do you agree?

So yeah, I appreciate that Node is getting really picky about it, so there's not really much to be done, but it seems a bit unfortunate to have to choose an error handling strategy to appease where the tests are being run, rather than the production code. Are there other reasons to report errors manually?

I agree with this, but it's not easy to draw this line -- and Node plans to crash on even a handled unhandled rejection (via the on event). I think this is best fixed by an ErrorBoundary.

[mdr]

Alas, I had a try with ErrorBoundary, which is otherwise really cool, but unfortunately it doesn't catch async errors (e.g. an async componentDidMount() that ends up rejecting).

Another option is for us to change our test so that we explicitly handle componentDidMount() rejections:

  const component = shallow(<MyContainerComponent />, { disableLifecycleMethods: true })
  expect(component.instance().componentDidMount()).rejects.toMatchObject({})

[Timer]

Sigh, you're right. I'll re-open this as a proposal but I'm not sure the best path forward for this.
I wonder if async componentDidMount will be a first-class citizen when react suspense ships...

Anyway, I don't see us changing this in the near-term so I'd suggest your test to look for that rejection.