Replace console.error() with a throw in setTimeout() as last resort exception logging #13310
Conversation
| // It reproduces a combination of conditions that led to a problem. | ||
|
|
||
| if (global.window) { | ||
| throw new Error('This test must run in a Node environment.'); |
|
|
||
| // Unlike other tests, we want to enable error logging. | ||
| // Note that we'll later override console.error. | ||
| Error.prototype.suppressReactErrorLogging = false; |
There was a problem hiding this comment.
For other onlookers, I thought this meant that React extended the Error prototype. This only happens in the React test suite:
react/scripts/jest/setupEnvironment.js
Lines 10 to 16 in f762b3a
There was a problem hiding this comment.
Actually looks like this raises a warning on CI:
24:1 warning Error prototype is read only, properties should not be added no-extend-native
Can you add: // eslint-disable-next-line no-extend-native?
| * @jest-environment node | ||
| */ | ||
|
|
||
| // This is a regression test for https://github.com/facebook/react/issues/13188. |
There was a problem hiding this comment.
Nit: what do you think about dropping the period after this link so that it's easier to copy/paste from Github?
| expect(didCatch).toBe(true); | ||
| ReactDOM.render(<span>Hello</span>, div); | ||
| expect(div.firstChild.textContent).toBe('Hello'); | ||
| }); |
There was a problem hiding this comment.
What do you think about using expect.assertions(2), instead of assigning a didCatch variable? Or expect().toThrow, like:
expect(() => {
ReactDOM.render(<Bad />, div);
ReactDOM.render(<Bad />, div);
}).toThrow('Maximum update depth exceeded') // maybe this needs to be a regex?
gaearon
changed the title
|
I pushed a second commit that changes the last resort reporting strategy from
|
|
ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.3% Details of bundled changes.Comparing: b3b80a4...f703123 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
| // https://github.com/facebook/react/issues/13188 | ||
| setTimeout(() => { | ||
| throw e; | ||
| }); |
There was a problem hiding this comment.
Since you set suppressReactErrorLogging to false, will this execute after the test above? Should you use jest.useFakeTimers() and force it to run with something like jest.runAllTimers()?
There was a problem hiding this comment.
I didn't really intend to test this part although I guess I could. I only wanted to ensure that we don't blow up the reconciler by rethrowing.
There was a problem hiding this comment.
Cool, my primary concern was just confusion in test reporting if the error showed up randomly in the test suite because Node or Jest saw an uncaught error.
No need to chase this down too much.
There was a problem hiding this comment.
Ah cool. Posted too late! I didn't realize that fake timers were already setup, so this wouldn't have run anyway.
| // Rethrow it from a clean stack because this function is assumed to never throw. | ||
| // We can't safely call console.error() here because it could *also* throw if overriden. | ||
| // https://github.com/facebook/react/issues/13188 | ||
| setTimeout(() => { |
There was a problem hiding this comment.
I guess setTimeout could also be buggy. Let's not think about this.
This is a curious bug: #13188.
On 16.4.1, this test fails with
on the final update (which should have been gracefully rendered).
On master, this test fails by ignoring the last update and not rendering anything.
This test has a combination of interesting circumstances:
console.errorthat throwswhich might contribute to why the bug occurs. We should fix it regardless though.