From 623f10cce25668aa8abd11243dcf5b0d9d2d76a2 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 5 Jan 2018 09:44:45 -0800 Subject: [PATCH] Fixed potential false-positive in toWarnDev matcher (#11898) * Warn about spying on the console * Added suppress warning flag for spyOn(console) * Nits * Removed spy-on-console guard * Fixed a potential source of false-positives in toWarnDev() matcher Also updated (most of) ReactIncrementalErrorLogging-test.internal to use the new matcher * Removed unused third param to spyOn * Improved clarity of inline comments * Removed unused normalizeCodeLocInfo() method --- .../ReactDOMServerIntegrationTestUtils.js | 2 + ...ctIncrementalErrorLogging-test.internal.js | 123 ++++++++---------- scripts/jest/matchers/toWarnDev.js | 25 ++-- 3 files changed, 72 insertions(+), 78 deletions(-) diff --git a/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js b/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js index 980cb1262f583..f24c2917dfd81 100644 --- a/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js +++ b/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js @@ -41,6 +41,8 @@ module.exports = function(initModules) { if (console.error.calls && console.error.calls.reset) { console.error.calls.reset(); } else { + // TODO: Rewrite tests that use this helper to enumerate expeceted errors. + // This will enable the helper to use the .toWarnDev() matcher instead of spying. spyOnDev(console, 'error'); } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js index 15630c1f4967e..57afa0399c779 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js @@ -20,12 +20,10 @@ describe('ReactIncrementalErrorLogging', () => { ReactNoop = require('react-noop-renderer'); }); - function normalizeCodeLocInfo(str) { - return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); - } - it('should log errors that occur during the begin phase', () => { - spyOnDevAndProd(console, 'error'); + // Errors are redundantly logged in production mode by ReactFiberErrorLogger. + // It's okay to ignore them for the purpose of this test. + spyOnProd(console, 'error'); class ErrorThrowingComponent extends React.Component { componentWillMount() { @@ -41,36 +39,29 @@ describe('ReactIncrementalErrorLogging', () => { } } - try { - ReactNoop.render( -
- - - -
, - ); - ReactNoop.flushDeferredPri(); - } catch (error) {} + ReactNoop.render( +
+ + + +
, + ); - expect(console.error.calls.count()).toBe(1); - const errorMessage = console.error.calls.argsFor(0)[0]; - if (__DEV__) { - expect(normalizeCodeLocInfo(errorMessage)).toContain( + expect(() => { + expect(ReactNoop.flushDeferredPri).toWarnDev( 'The above error occurred in the component:\n' + ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + - ' in div (at **)', + ' in div (at **)\n\n' + + 'Consider adding an error boundary to your tree to customize error handling behavior.', ); - expect(errorMessage).toContain( - 'Consider adding an error boundary to your tree to customize error handling behavior.', - ); - } else { - expect(errorMessage.message).toContain('componentWillMount error'); - } + }).toThrowError('componentWillMount error'); }); it('should log errors that occur during the commit phase', () => { - spyOnDevAndProd(console, 'error'); + // Errors are redundantly logged in production mode by ReactFiberErrorLogger. + // It's okay to ignore them for the purpose of this test. + spyOnProd(console, 'error'); class ErrorThrowingComponent extends React.Component { componentDidMount() { @@ -86,32 +77,23 @@ describe('ReactIncrementalErrorLogging', () => { } } - try { - ReactNoop.render( -
- - - -
, - ); - ReactNoop.flushDeferredPri(); - } catch (error) {} + ReactNoop.render( +
+ + + +
, + ); - expect(console.error.calls.count()).toBe(1); - const errorMessage = console.error.calls.argsFor(0)[0]; - if (__DEV__) { - expect(normalizeCodeLocInfo(errorMessage)).toContain( + expect(() => { + expect(ReactNoop.flushDeferredPri).toWarnDev( 'The above error occurred in the component:\n' + ' in ErrorThrowingComponent (at **)\n' + ' in span (at **)\n' + - ' in div (at **)', - ); - expect(errorMessage).toContain( - 'Consider adding an error boundary to your tree to customize error handling behavior.', + ' in div (at **)\n\n' + + 'Consider adding an error boundary to your tree to customize error handling behavior.', ); - } else { - expect(errorMessage.message).toBe('componentDidMount error'); - } + }).toThrowError('componentDidMount error'); }); it('should ignore errors thrown in log method to prevent cycle', () => { @@ -120,6 +102,8 @@ describe('ReactIncrementalErrorLogging', () => { try { React = require('react'); ReactNoop = require('react-noop-renderer'); + + // TODO Update this test to use toWarnDev() matcher if possible spyOnDevAndProd(console, 'error'); class ErrorThrowingComponent extends React.Component { @@ -167,7 +151,9 @@ describe('ReactIncrementalErrorLogging', () => { }); it('should relay info about error boundary and retry attempts if applicable', () => { - spyOnDevAndProd(console, 'error'); + // Errors are redundantly logged in production mode by ReactFiberErrorLogger. + // It's okay to ignore them for the purpose of this test. + spyOnProd(console, 'error'); class ParentComponent extends React.Component { render() { @@ -203,30 +189,27 @@ describe('ReactIncrementalErrorLogging', () => { } } - try { - ReactNoop.render(); - ReactNoop.flush(); - } catch (error) {} + ReactNoop.render(); - expect(renderAttempts).toBe(2); - expect(handleErrorCalls.length).toBe(1); - expect(console.error.calls.count()).toBe(2); - if (__DEV__) { - expect(console.error.calls.argsFor(0)[0]).toContain( - 'The above error occurred in the component:', - ); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'React will try to recreate this component tree from scratch ' + + expect(() => { + expect(ReactNoop.flush).toWarnDev([ + 'The above error occurred in the component:\n' + + ' in ErrorThrowingComponent (at **)\n' + + ' in ErrorBoundaryComponent (at **)\n' + + ' in ParentComponent (at **)\n\n' + + 'React will try to recreate this component tree from scratch ' + 'using the error boundary you provided, ErrorBoundaryComponent.', - ); - expect(console.error.calls.argsFor(1)[0]).toContain( - 'The above error occurred in the component:', - ); - expect(console.error.calls.argsFor(1)[0]).toContain( - 'This error was initially handled by the error boundary ErrorBoundaryComponent.\n' + + 'The above error occurred in the component:\n' + + ' in ErrorThrowingComponent (at **)\n' + + ' in ErrorBoundaryComponent (at **)\n' + + ' in ParentComponent (at **)\n\n' + + 'This error was initially handled by the error boundary ErrorBoundaryComponent.\n' + 'Recreating the tree from scratch failed so React will unmount the tree.', - ); - } + ]); + }).toThrowError('componentDidMount error'); + + expect(renderAttempts).toBe(2); + expect(handleErrorCalls.length).toBe(1); }); }); diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 538c4753c675a..fcffae5a80c9c 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -19,6 +19,12 @@ const createMatcherFor = consoleMethod => const unexpectedWarnings = []; + // Catch errors thrown by the callback, + // But only rethrow them if all test expectations have been satisfied. + // Otherwise an Error in the callback can mask a failed expectation, + // and result in a test that passes when it shouldn't. + let caughtError; + const consoleSpy = message => { const normalizedMessage = normalizeCodeLocInfo(message); @@ -58,6 +64,11 @@ const createMatcherFor = consoleMethod => try { callback(); + } catch (error) { + caughtError = error; + } finally { + // Restore the unspied method so that unexpected errors fail tests. + console[consoleMethod] = originalMethod; // Any unexpected warnings should be treated as a failure. if (unexpectedWarnings.length > 0) { @@ -72,20 +83,18 @@ const createMatcherFor = consoleMethod => return { message: () => `Expected warning was not recorded:\n ${this.utils.printReceived( - expectedMessages.join('\n') + expectedMessages[0] )}`, pass: false, }; } + // Any unexpected Errors thrown by the callback should fail the test. + if (caughtError) { + throw caughtError; + } + return {pass: true}; - } catch (error) { - // TODO Flag this error so Jest doesn't override its stack - // See https://tinyurl.com/y9unakwb - throw error; - } finally { - // Restore the unspied method so that unexpected errors fail tests. - console[consoleMethod] = originalMethod; } } else { // Any uncaught errors or warnings should fail tests in production mode.