Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow suppressing error boundary logs from intentionally thrown/caught errors #11098

Closed
gaearon opened this issue Oct 4, 2017 · 31 comments
Closed

Comments

@gaearon
Copy link
Member

gaearon commented Oct 4, 2017


(This is a repost of facebook/jest#4597 by @erikras.)


Do you want to request a feature or report a bug?

Somewhere in between?

What is the current behavior?

When I'm running tests on my library, there are some behaviors that I want to test do throw an error. These currently result in:

Consider adding an error boundary to your tree to customize error handling behavior.
You can learn more about error boundaries at https://fb.me/react-error-boundaries.

...being output to the console. This error is great in an application, but not so great for a library test.

What is the expected behavior?

It would be great if I could do something like:

expect(() => {
  TestUtils.renderIntoDocument(<DoSomething naughty/>)
})
.toThrow(/Bad developer!/)
.andCatch() // <---- prevents React 16 error boundary warning

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.

jest@21.2.1
react@16.0.0
react-dom@16.0.0

See also

@gaearon
Copy link
Member Author

gaearon commented Oct 4, 2017

cc @acdlite @sebmarkbage @bvaughn

@bvaughn
Copy link
Collaborator

bvaughn commented Oct 9, 2017

This error is great in an application, but not so great for a library test.

One way to prevent these unwanted messages from appearing in tests is:

spyOn(console, 'error'); // In tests that you expect errors

If this is too onerous you could also do it globally in a Jest setup file.

I don't think we saw these warnings. But we probably mocked something in Jest at some point and that's why.

Regarding @gaearon's comment on the original issue, I believe we don't see them because we do a mix of spying on console.error and mocking ReactFiberErrorLogger.

Unfortunately mocking out ReactFiberErrorLogger isn't really recommended outside of the context of the React project. We should come up with a cleaner story for this in order to simplify external testing.

timdorr added a commit to remix-run/react-router that referenced this issue Oct 18, 2017
Temp fixes for error boundaries until facebook/react#11098 is resolved.
@travi
Copy link
Contributor

travi commented Nov 16, 2017

i've been running into similar issues in mocha where error behavior under test is being hidden by this warning.

my issue was made somewhat worse because of the fact that i override console.error and console.warn in my tests to ensure that react warnings cause test failures. this caused those errors to be caught by this error boundary log even earlier in some cases. removing these overrides allowed me to see further error output, but resolving the newly revealed output brought me back to the boundary logs again.

if there is anything i could help with as far as insight from testing with mocha related to this issue, i'm happy to.

@gaearon
Copy link
Member Author

gaearon commented Nov 22, 2017

i've been running into similar issues in mocha where error behavior under test is being hidden by this warning.

Can you clarify what you mean by this?

When you see this warning but your test doesn't fail, it means your code is throwing somewhere but then this error gets caught and never reaches your test's stack frame. Unless you're intentionally testing error cases, this indicates a bug in your code. But this warning itself doesn't "hide" the original error. If we removed the warning your bug would be completely invisible (since something catches it). The warning surfaces it.

@gaearon
Copy link
Member Author

gaearon commented Nov 28, 2017

I see two separate issues here:

  • "The above error occurred" message is not helpful in a test environment because we're not showing the actual thrown. We're hoping the browser will show it, but jsdom isn't a browser. Maybe there's some sort of feature detection we could do to determine whether to show the full message?

  • Some people might want to disable error reporting in tests altogether. I think in general this is a bad idea. But there might be legitimate reasons to do this. Remove global mocks by adding support for "suppressReactErrorLogging" property #11636 already provides an escape hatch for this in 16.2+.

@travi
Copy link
Contributor

travi commented Dec 8, 2017

Sorry for the delayed response on this.

Can you clarify what you mean by this?

I stub console.error and console.warn so that any React warnings need to be fixed in order for our test suite to pass.

console.error = err => { throw new Error(err); };
console.warn = warning => { throw new Error(warning); };

with those stubs in place, if a warning is triggered, it is elevated to an error. that error then gets picked up by the error boundary suggestion, hiding the actual warning. for example:

Error: Error: The above error occurred in the component:
...
...
Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
...
...

instead of

Warning: Unknown event handler property onLeftIconButtonTouchTap. It will be ignored.
...
...

hopefully that helps explain that situation a bit better

@gaearon
Copy link
Member Author

gaearon commented Dec 8, 2017

Yes, this is the first issue I described in #11098 (comment). I agree we need to fix it but it’s not obvious to me how. The browser shows errors that are thrown like this but jsdom doesn’t. Why? Should we file an issue with jsdom?

mvaivre pushed a commit to mvaivre/react-router-dom that referenced this issue Dec 13, 2017
Temp fixes for error boundaries until facebook/react#11098 is resolved.
@gaearon
Copy link
Member Author

gaearon commented Jan 3, 2018

The browser shows errors that are thrown like this but jsdom doesn’t. Why? Should we file an issue with jsdom?

Answering my own question: jsdom does show these errors now. Updating to Jest 22 surfaced them in our test suite. So even if we ignored them in React, they would still show up in your tests as logs.

It turns out that you can e.preventDefault() in the error event handler for the window object to prevent the browser (or jsdom) from logging the error. I didn’t know that.

I think it would make sense to me if React did the same.
Here is a minimal patch to React that would do that:

--- a/packages/shared/invokeGuardedCallback.js
+++ b/packages/shared/invokeGuardedCallback.js
@@ -125,6 +125,7 @@ if (__DEV__) {
       // Use this to track whether the error event is ever called.
       let didSetError = false;
       let isCrossOriginError = false;
+      let shouldIgnoreError = false;
 
       function onError(event) {
         error = event.error;
@@ -132,6 +133,9 @@ if (__DEV__) {
         if (error === null && event.colno === 0 && event.lineno === 0) {
           isCrossOriginError = true;
         }
+        if (event.defaultPrevented) {
+          shouldIgnoreError = true;
+        }
       }
 
       // Create a fake event type.
@@ -172,6 +176,9 @@ if (__DEV__) {
         this._hasCaughtError = false;
         this._caughtError = null;
       }
+      if (shouldIgnoreError && error != null) {
+        error.suppressReactErrorLogging = true;
+      }

It's not great though because it relies on setting an undocumented field (suppressReactErrorLogging) on the error object, thereby mutating it. I only added this field in #11636 because I didn't know that there is a preventDefault() convention for preventing logging of intentionally swallowed errors.

What do you think about this course of action:

  • Remove support for the custom suppressReactErrorLogging property.
  • Instead, check e.defaultPrevented in our error event handler in invokeGuardedCallback.js.
  • If the default is prevented, store that the error is "muted", similar to how we currently store _hasCaughtError and _caughtError.
  • Change the logic in ReactFiberScheduler to not log the "muted" errors according to this new flag.
  • Set up an error handler in our tests that calls e.preventDefault() (and thus silences the logs).
  • Make it possible to disable that behavior in the few tests that actually verify logging. (?)

I’m probably not motivated enough to follow through with this but if somebody else wants to take it, I’m happy to discuss.

@bvaughn
Copy link
Collaborator

bvaughn commented Jan 3, 2018

I like the idea of replacing suppressReactErrorLogging with e. defaultPrevented 👍

gaearon added a commit to gaearon/react that referenced this issue Jan 3, 2018
This relies on our existing special field that we use to mute errors.
Perhaps, it would be better to instead rely on preventDefault() directly.
I outlined a possible strategy here: facebook#11098 (comment)
@gaearon gaearon changed the title How to suppress error boundary logs from intentionally caught errors in Jest tests? Allow suppressing error boundary logs from intentionally thrown/caught errors Jan 3, 2018
@gaearon
Copy link
Member Author

gaearon commented Jan 3, 2018

Tagging as a good issue to look into. Not promising we'll merge a solution, but worth investigating.
Proposed implementation plan: #11098 (comment).

@vikramcse
Copy link

vikramcse commented Jan 4, 2018

Can I take this issue for my first contribution?

@craigkovatch
Copy link

craigkovatch commented Aug 9, 2018

Thanks Dan! And sorry for my lack of response on your proposal, I'm not versed in the internals of React so didn't feel competent to comment.

@gaearon
Copy link
Member Author

gaearon commented Aug 13, 2018

All right. I have merged a fix in #13384 which I think strikes a reasonable balance between giving you control over the warning noise and preventing accidentally swallowed errors.

Specifically, in the next React release (likely 16.4.3), we won't log the extra message (The above error occurred ...) if these three conditions are all true:

  1. You're in development mode with a DOM environment, and
  2. The error was successfully caught by an error boundary, and
  3. You have called event.preventDefault() for that error in a custom error event handler

Let me unpack what this means, and how you can adjust your tests.

Short Summary: New Helper for React 16.4.3+

Starting with React 16.4.3 (not out yet), you will be able to suppress rendering errors in tests by using a special helper (or a variation of the same technique). I put the example here: https://gist.github.com/gaearon/adf9d5500e11a4e7b2c6f7ebf994fe56.

It won't generate any warnings for intentionally thrown errors when you use ReactDOM in development mode in a jsdom environment. It will, however, fail the tests for unintentionally thrown errors, even if those were silenced by nested error boundaries.

If this helper is not sufficient for some reason (e.g. if you're using test renderer instead of ReactDOM) please keep mocking console.error. There's nothing wrong about that approach.

Now, if you're curious, let me guide you through why it works this way.

Current Behavior (React 16.0.0 to 16.4.2, inclusively)

Consider this example:

const React = require('react');
const ReactDOM = require('react-dom');

function Darth() {
  throw new Error('I am your father')
}

it('errors', () => {
  const div = document.createElement('div');
  expect(() => {
    ReactDOM.render(<Darth />, div);
  }).toThrow('father');
});

Before this change and with an older version of jsdom (the one that ships in the currently stable Create React App), the output looks like this:

 PASS  src/App.test.js
  ✓ errors

  console.error node_modules/react-dom/cjs/react-dom.development.js:14227
    The above error occurred in the <Darth> component:
        in Darth (at App.test.js:12)
    
    Consider adding an error boundary to your tree to customize error handling behavior.
    Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

Not too bad although the warning is a bit annoying. However, still, before this change, it gets worse if you update to a recent version of jsdom (through a Jest update):

 PASS  src/App.test.js
  ✓ errors

  console.error node_modules/jsdom/lib/jsdom/virtual-console.js:29
    Error: Uncaught [Error: I am your father]
        at reportException (/Users/gaearon/p/testj/node_modules/jsdom/lib/jsdom/living/helpers/runtime-script-errors.js:66:24)
        <...>
        at promise.then (/Users/gaearon/p/testj/node_modules/jest-jasmine2/build/queue_runner.js:87:41)
        at <anonymous>
        at process._tickCallback (internal/process/next_tick.js:188:7)

  console.error node_modules/react-dom/cjs/react-dom.development.js:14227
    The above error occurred in the <Darth> component:
        in Darth
    
    Consider adding an error boundary to your tree to customize error handling behavior.
    Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

You can see that the test is still passing but there are two annoying warnings. Let's dig into why we see them.

What Emits These Warnings?

We see two warnings:

  • Error: Uncaught [...]
  • The above error occurred ...

The first one is coming from jsdom. The second one is coming from React.

The new jsdom warning is great and mimics what browsers do. It allows us report errors even if they were accidentally swallowed by the component code — for example:

fetchSomething()
  .then((data) => this.setState({ data })) // oops! errors from this render are swallowed
  .catch(err => this.setState({ err }))

Before React 16, this error would be swallowed, but React 16 still surfaces it to the user. This is a feature, not a bug. It's great that jsdom does it too now, and it's a good default for your tests — especially for application tests. But it can be annoying for libraries.

How to Opt Out of the jsdom Warning?

What about the cases where you don't want to see it? In the browsers, you can opt out by calling preventDefault() in a custom error event handler on the window object.

function onError(event) {
  // Note: this will swallow reports about unhandled errors!
  // Use with extreme caution.
  event.preventDefault();
}

window.addEventListener('error', onError);

This should only be used with extreme caution — it's easy to accidentally mute information about important warnings. Here's a more granular approach we can use in tests that intentionally throw errors:

const React = require('react');
const ReactDOM = require('react-dom');

function Darth() {
  throw new Error('I am your father')
}

let expectedErrors = 0;
let actualErrors = 0;
function onError(e) {
  e.preventDefault();
  actualErrors++;
}

beforeEach(() => {
  expectedErrors = 0;
  actualErrors = 0;
  window.addEventListener('error', onError);
});

afterEach(() => {
  window.removeEventListener('error', onError);
  expect(actualErrors).toBe(expectedErrors);
  expectedErrors = 0;
});

it('errors', () => {
  expectedErrors = 1; // Remember only one error was expected

  const div = document.createElement('div');
  expect(() => {
    ReactDOM.render(<Darth />, div);
  }).toThrow('father');
});

Note how I'm using preventDefault to silence the expected warnings. However, if there's a deeper component that has thrown an error which was accidentally caught an silenced, your test would still fail because you'd get an extra unexpected error.

If you use this approach, make sure that every addEventListener call has a matching removeEventListener call. I intentionally put them in beforeEach and afterEach so that even if a test fails, the calls still always match up. An alternative if you don't like beforeEach is to use try / finally blocks.

With this above test suite, we've effectively silenced the noisy jsdom warning. But we still see the component stack from React:

 PASS  src/App.test.js
  ✓ errors

  console.error node_modules/react-dom/cjs/react-dom.development.js:14227
    The above error occurred in the <Darth> component:
        in Darth
    
    Consider adding an error boundary to your tree to customize error handling behavior.
    Visit https://fb.me/react-error-boundaries to learn more about error boundaries.

Let's see how we can fix this.

How to Opt Out of Seeing React Component Stack

Note this section will only work in the next React release — most likely, 16.4.3. I'm still writing it up here for future reference.

The component stack is important to print because it helps locate the source of the error. With #13384, I changed the logic so that we won't print the stack if the error itself was silenced (as described above) and the error was handled by an error boundary. In the above example we're not using an error boundary, and that's why we see the extra stack.

We can fix this by extracting a method called expectRenderError that uses an error boundary:

const React = require('react');
const ReactDOM = require('react-dom');

function expectRenderError(element, expectedError) {
  // Noop error boundary for testing.
  class TestBoundary extends React.Component {
    constructor(props) {
      super(props);
      this.state = { didError: false };
    }
    componentDidCatch(err) {
      this.setState({ didError: true });
    }
    render() {
      return this.state.didError ? null : this.props.children;
    }
  }

  // Record all errors.
  let topLevelErrors = [];
  function handleTopLevelError(event) {
    topLevelErrors.push(event.error);
    // Prevent logging
    event.preventDefault();
  }

  const div = document.createElement('div');
  window.addEventListener('error', handleTopLevelError);
  try {
    ReactDOM.render(
      <TestBoundary>
        {element}
      </TestBoundary>,
      div
    );
  } finally {
    window.removeEventListener('error', handleTopLevelError);
  }

  expect(topLevelErrors.length).toBe(1);
  expect(topLevelErrors[0].message).toContain(expectedError);
}

I'm not suggesting to copy and paste this helper into every test. It's probably best that you define it once in your project, or even put it on npm. With this approach, our test can be very simple again:

function Darth() {
  throw new Error('I am your father')
}

it('errors', () => {
  expectRenderError(
    <Darth />,
    'father'
  );
});

As I mentioned in the beginning, I put this helper here: https://gist.github.com/gaearon/adf9d5500e11a4e7b2c6f7ebf994fe56.

Feel free to republish it on npm, turn it into a Jest matcher, etc.

Conclusion

Hope this helps! I'm sorry if it's disappointing we don't just disable this warning. But I hope you can see the rationale: we think it's extremely important to prevent app developers from accidentally swallowing errors. Even if it comes with the cost of some additional bookkeeping for library developers who want to make assertions about thrown errors.

Finally, if this approach doesn't work out for you I'd like to clarify there's nothing bad about mocking console.error either — we do it all the time in the React suite (and have written custom matchers for ourselves to assist with that). Perhaps you'd want to do something similar if you have a lot of tests around throwing errors from React components.

Cheers!

@gaearon
Copy link
Member Author

gaearon commented Sep 5, 2018

If it helps, what we do in React's own test suite is much simpler although it can also potentially filter out some valid warnings.

@hpurmann
Copy link

hpurmann commented Sep 13, 2018

Hey @gaearon! Thank you for that patch in React 16.5. I tried it in our project and it unfortunately failed. I constructed a minimal reproduction of the issue.

Importing enzyme-adapter-react-16 in the setup of the test leads to a test failure with the proposed solution. It seems that the event listener is not triggered when the adapter is present.

Do you know of a way around this?

@hpurmann
Copy link

hpurmann commented Sep 20, 2018

This issue was solved for me. When using enzyme, simulateError does also prevent the error log in the console enzymejs/enzyme#1826 (comment) 👍

@dirtyhenry
Copy link

dirtyhenry commented Nov 29, 2018

If it can spare others some burden, I gave the expectRenderError trick a try but I still get the following message:

React will try to recreate this component tree from scratch using the error boundary you provided, TestBoundary.

@sunchenguang
Copy link

sunchenguang commented Apr 9, 2019

just use simulateError method。 in my case it works fine.
https://airbnb.io/enzyme/docs/api/ReactWrapper/simulateError.html

@jedmao
Copy link

jedmao commented Aug 22, 2019

If anyone's struggling with how to test an error boundary with React Testing Library, I hope this helps:

it('renders "Something went wrong." when an error is thrown', () => {
	const spy = jest.spyOn(console, 'error')
	spy.mockImplementation(() => {})

	const Throw = () => {
		throw new Error('bad')
	}

	const { getByText } = render(
		<ErrorBoundary>
			<Throw />
		</ErrorBoundary>,
	)

	expect(getByText('Something went wrong.')).toBeDefined()

	spy.mockRestore()
})

@halis
Copy link

halis commented Feb 13, 2020

This is the dumbest error message I have ever seen. I don't want you to tell me about error boundaries, I want you to be quiet. SMH

@connelhooley
Copy link

connelhooley commented Jul 2, 2020

It's a shame this has been closed as it does provide an awful lot of noise in tests where an exception is expected to be thrown. We shouldn't have to be mocking the console to cut this noise out.

@BernardoFBBraga
Copy link

BernardoFBBraga commented Sep 30, 2020

The following error still shows for me:

 console.error ../node_modules/react-dom/cjs/react-dom.development.js:530
    Warning: ErrorCatcher: Error boundaries should implement getDerivedStateFromError(). In that method, return a state update to display an error message or fallback UI.

Docs say that this is optional but jest is warning as if it was mandatory

I was only able to get rid of it by mocking the console

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests