Fixed potential false-positive in toWarnDev matcher (facebook#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
yenshih · Jan 6, 2018 · 0441e5f · 0441e5f
1 parent f3f2279
commit 0441e5f
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 78 deletions.
diff --git 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
@@ -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(
-        <div>
-          <span>
-            <ErrorThrowingComponent />
-          </span>
-        </div>,
-      );
-      ReactNoop.flushDeferredPri();
-    } catch (error) {}
+    ReactNoop.render(
+      <div>
+        <span>
+          <ErrorThrowingComponent />
+        </span>
+      </div>,
+    );
 
-    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 <ErrorThrowingComponent> 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(
-        <div>
-          <span>
-            <ErrorThrowingComponent />
-          </span>
-        </div>,
-      );
-      ReactNoop.flushDeferredPri();
-    } catch (error) {}
+    ReactNoop.render(
+      <div>
+        <span>
+          <ErrorThrowingComponent />
+        </span>
+      </div>,
+    );
 
-    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 <ErrorThrowingComponent> 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(<ParentComponent />);
-      ReactNoop.flush();
-    } catch (error) {}
+    ReactNoop.render(<ParentComponent />);
 
-    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 <ErrorThrowingComponent> 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 <ErrorThrowingComponent> 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 <ErrorThrowingComponent> 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 <ErrorThrowingComponent> 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
@@ -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.