Don't suppress jsdom error reporting in our tests (#13401)

* Don't suppress jsdom error reporting

* Address review

packages/react-dom/src/__tests__/ReactErrorLoggingRecovery-test.js

@@ -19,12 +19,6 @@ if (global.window) {
 const React = require('react');
 const ReactDOM = require('react-dom');
 
-// Unlike other tests, we want to enable error logging.
-// Note this is not a real Error prototype property,
-// it's only set in our Jest environment.
-// eslint-disable-next-line no-extend-native
-Error.prototype.suppressReactErrorLogging = false;
-
 // Initialize JSDOM separately.
 // We don't use our normal JSDOM setup because we want to load React first.
 const {JSDOM} = require('jsdom');

packages/react-reconciler/src/ReactFiberErrorLogger.js

@@ -21,11 +21,6 @@ export function logCapturedError(capturedError: CapturedError): void {
   }
 
   const error = (capturedError.error: any);
-  const suppressLogging = error && error.suppressReactErrorLogging;
-  if (suppressLogging) {
-    return;
-  }
-
   if (__DEV__) {
     const {
       componentName,

packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js

@@ -1,194 +0,0 @@
-/**
- * Copyright (c) 2013-present, Facebook, Inc.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- * @emails react-core
- * @jest-environment node
- */
-
-'use strict';
-
-let React;
-let ReactFeatureFlags;
-let ReactNoop;
-
-describe('ReactIncrementalErrorLogging', () => {
-  beforeEach(() => {
-    jest.resetModules();
-    ReactFeatureFlags = require('shared/ReactFeatureFlags');
-    ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
-    ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
-    React = require('react');
-    ReactNoop = require('react-noop-renderer');
-  });
-
-  it('should log errors that occur during the begin phase', () => {
-    // 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 {
-      UNSAFE_componentWillMount() {
-        const error = new Error('componentWillMount error');
-        // Note: it's `true` on the Error prototype our test environment.
-        // That lets us avoid asserting on warnings for each expected error.
-        // Here we intentionally shadow it to test logging, like in real apps.
-        error.suppressReactErrorLogging = undefined;
-        throw error;
-      }
-      render() {
-        return <div />;
-      }
-    }
-
-    ReactNoop.render(
-      <div>
-        <span>
-          <ErrorThrowingComponent />
-        </span>
-      </div>,
-    );
-
-    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 **)\n\n' +
-          'Consider adding an error boundary to your tree to customize error handling behavior.',
-      );
-    }).toThrowError('componentWillMount error');
-  });
-
-  it('should log errors that occur during the commit phase', () => {
-    // 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() {
-        const error = new Error('componentDidMount error');
-        // Note: it's `true` on the Error prototype our test environment.
-        // That lets us avoid asserting on warnings for each expected error.
-        // Here we intentionally shadow it to test logging, like in real apps.
-        error.suppressReactErrorLogging = undefined;
-        throw error;
-      }
-      render() {
-        return <div />;
-      }
-    }
-
-    ReactNoop.render(
-      <div>
-        <span>
-          <ErrorThrowingComponent />
-        </span>
-      </div>,
-    );
-
-    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 **)\n\n' +
-          'Consider adding an error boundary to your tree to customize error handling behavior.',
-      );
-    }).toThrowError('componentDidMount error');
-  });
-
-  it('should ignore errors thrown in log method to prevent cycle', () => {
-    jest.resetModules();
-    jest.mock('../ReactFiberErrorLogger');
-    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 {
-        render() {
-          throw new Error('render error');
-        }
-      }
-
-      const logCapturedErrorCalls = [];
-
-      const ReactFiberErrorLogger = require('../ReactFiberErrorLogger');
-      ReactFiberErrorLogger.logCapturedError.mockImplementation(
-        capturedError => {
-          logCapturedErrorCalls.push(capturedError);
-          const error = new Error('logCapturedError error');
-          // Note: it's `true` on the Error prototype our test environment.
-          // That lets us avoid asserting on warnings for each expected error.
-          // Here we intentionally shadow it to test logging, like in real apps.
-          error.suppressReactErrorLogging = undefined;
-          throw error;
-        },
-      );
-
-      try {
-        ReactNoop.render(
-          <div>
-            <span>
-              <ErrorThrowingComponent />
-            </span>
-          </div>,
-        );
-        ReactNoop.flushDeferredPri();
-      } catch (error) {}
-
-      expect(logCapturedErrorCalls.length).toBe(1);
-
-      // The error thrown in logCapturedError should be rethrown with a clean stack
-      expect(() => {
-        jest.runAllTimers();
-      }).toThrow('logCapturedError error');
-    } finally {
-      jest.unmock('../ReactFiberErrorLogger');
-    }
-  });
-
-  it('resets instance variables before unmounting failed node', () => {
-    class ErrorBoundary extends React.Component {
-      state = {error: null};
-      componentDidCatch(error) {
-        this.setState({error});
-      }
-      render() {
-        return this.state.error ? null : this.props.children;
-      }
-    }
-    class Foo extends React.Component {
-      state = {step: 0};
-      componentDidMount() {
-        this.setState({step: 1});
-      }
-      componentWillUnmount() {
-        ReactNoop.yield('componentWillUnmount: ' + this.state.step);
-      }
-      render() {
-        ReactNoop.yield('render: ' + this.state.step);
-        if (this.state.step > 0) {
-          throw new Error('oops');
-        }
-        return null;
-      }
-    }
-
-    ReactNoop.render(
-      <ErrorBoundary>
-        <Foo />
-      </ErrorBoundary>,
-    );
-    expect(ReactNoop.flush()).toEqual([
-      'render: 0',
-      'render: 1',
-      'componentWillUnmount: 0',
-    ]);
-  });
-});

packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js

@@ -0,0 +1,211 @@
+/**
+ * Copyright (c) 2013-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @emails react-core
+ * @jest-environment node
+ */
+
+'use strict';
+
+let React;
+let ReactNoop;
+
+describe('ReactIncrementalErrorLogging', () => {
+  beforeEach(() => {
+    jest.resetModules();
+    React = require('react');
+    ReactNoop = require('react-noop-renderer');
+  });
+
+  // Note: in this test file we won't be using toWarnDev() matchers
+  // because they filter out precisely the messages we want to test for.
+  let oldConsoleError;
+  beforeEach(() => {
+    oldConsoleError = console.error;
+    console.error = jest.fn();
+  });
+
+  afterEach(() => {
+    console.error = oldConsoleError;
+    oldConsoleError = null;
+  });
+
+  it('should log errors that occur during the begin phase', () => {
+    class ErrorThrowingComponent extends React.Component {
+      constructor(props) {
+        super(props);
+        throw new Error('constructor error');
+      }
+      render() {
+        return <div />;
+      }
+    }
+    ReactNoop.render(
+      <div>
+        <span>
+          <ErrorThrowingComponent />
+        </span>
+      </div>,
+    );
+    expect(ReactNoop.flushDeferredPri).toThrowError('constructor error');
+    expect(console.error).toHaveBeenCalledTimes(1);
+    expect(console.error).toHaveBeenCalledWith(
+      __DEV__
+        ? expect.stringMatching(
+            new RegExp(
+              'The above error occurred in the <ErrorThrowingComponent> component:\n' +
+                '\\s+in ErrorThrowingComponent (.*)\n' +
+                '\\s+in span (.*)\n' +
+                '\\s+in div (.*)\n\n' +
+                'Consider adding an error boundary to your tree ' +
+                'to customize error handling behavior\\.',
+            ),
+          )
+        : expect.objectContaining({
+            message: 'constructor error',
+          }),
+    );
+  });
+
+  it('should log errors that occur during the commit phase', () => {
+    class ErrorThrowingComponent extends React.Component {
+      componentDidMount() {
+        throw new Error('componentDidMount error');
+      }
+      render() {
+        return <div />;
+      }
+    }
+    ReactNoop.render(
+      <div>
+        <span>
+          <ErrorThrowingComponent />
+        </span>
+      </div>,
+    );
+    expect(ReactNoop.flushDeferredPri).toThrowError('componentDidMount error');
+    expect(console.error).toHaveBeenCalledTimes(1);
+    expect(console.error).toHaveBeenCalledWith(
+      __DEV__
+        ? expect.stringMatching(
+            new RegExp(
+              'The above error occurred in the <ErrorThrowingComponent> component:\n' +
+                '\\s+in ErrorThrowingComponent (.*)\n' +
+                '\\s+in span (.*)\n' +
+                '\\s+in div (.*)\n\n' +
+                'Consider adding an error boundary to your tree ' +
+                'to customize error handling behavior\\.',
+            ),
+          )
+        : expect.objectContaining({
+            message: 'componentDidMount error',
+          }),
+    );
+  });
+
+  it('should ignore errors thrown in log method to prevent cycle', () => {
+    const logCapturedErrorCalls = [];
+    console.error.mockImplementation(error => {
+      // Test what happens when logging itself is buggy.
+      logCapturedErrorCalls.push(error);
+      throw new Error('logCapturedError error');
+    });
+    class ErrorThrowingComponent extends React.Component {
+      render() {
+        throw new Error('render error');
+      }
+    }
+    ReactNoop.render(
+      <div>
+        <span>
+          <ErrorThrowingComponent />
+        </span>
+      </div>,
+    );
+    expect(ReactNoop.flushDeferredPri).toThrow('render error');
+    expect(logCapturedErrorCalls.length).toBe(1);
+    expect(logCapturedErrorCalls[0]).toEqual(
+      __DEV__
+        ? expect.stringMatching(
+            new RegExp(
+              'The above error occurred in the <ErrorThrowingComponent> component:\n' +
+                '\\s+in ErrorThrowingComponent (.*)\n' +
+                '\\s+in span (.*)\n' +
+                '\\s+in div (.*)\n\n' +
+                'Consider adding an error boundary to your tree ' +
+                'to customize error handling behavior\\.',
+            ),
+          )
+        : expect.objectContaining({
+            message: 'render error',
+          }),
+    );
+    // The error thrown in logCapturedError should be rethrown with a clean stack
+    expect(() => {
+      jest.runAllTimers();
+    }).toThrow('logCapturedError error');
+  });
+
+  it('resets instance variables before unmounting failed node', () => {
+    class ErrorBoundary extends React.Component {
+      state = {error: null};
+      componentDidCatch(error) {
+        this.setState({error});
+      }
+      render() {
+        return this.state.error ? null : this.props.children;
+      }
+    }
+    class Foo extends React.Component {
+      state = {step: 0};
+      componentDidMount() {
+        this.setState({step: 1});
+      }
+      componentWillUnmount() {
+        ReactNoop.yield('componentWillUnmount: ' + this.state.step);
+      }
+      render() {
+        ReactNoop.yield('render: ' + this.state.step);
+        if (this.state.step > 0) {
+          throw new Error('oops');
+        }
+        return null;
+      }
+    }
+
+    ReactNoop.render(
+      <ErrorBoundary>
+        <Foo />
+      </ErrorBoundary>,
+    );
+    expect(ReactNoop.flush()).toEqual(
+      [
+        'render: 0',
+        __DEV__ && 'render: 0', // replay
+        'render: 1',
+        __DEV__ && 'render: 1', // replay
+        'componentWillUnmount: 0',
+      ].filter(Boolean),
+    );
+
+    expect(console.error).toHaveBeenCalledTimes(1);
+    expect(console.error).toHaveBeenCalledWith(
+      __DEV__
+        ? expect.stringMatching(
+            new RegExp(
+              'The above error occurred in the <Foo> component:\n' +
+                '\\s+in Foo (.*)\n' +
+                '\\s+in ErrorBoundary (.*)\n\n' +
+                'React will try to recreate this component tree from scratch ' +
+                'using the error boundary you provided, ErrorBoundary.',
+            ),
+          )
+        : expect.objectContaining({
+            message: 'oops',
+          }),
+    );
+  });
+});

scripts/jest/matchers/toWarnDev.js

@@ -2,6 +2,7 @@
 
 const jestDiff = require('jest-diff');
 const util = require('util');
+const shouldIgnoreConsoleError = require('../shouldIgnoreConsoleError');
 
 function normalizeCodeLocInfo(str) {
   return str && str.replace(/at .+?:\d+/g, 'at **');
@@ -54,6 +55,15 @@ const createMatcherFor = consoleMethod =>
         typeof message === 'string' && message.includes('\n    in ');
 
       const consoleSpy = (format, ...args) => {
+        // Ignore uncaught errors reported by jsdom
+        // and React addendums because they're too noisy.
+        if (
+          consoleMethod === 'error' &&
+          shouldIgnoreConsoleError(format, args)
+        ) {
+          return;
+        }
+
         const message = util.format(format, ...args);
         const normalizedMessage = normalizeCodeLocInfo(message);
 

scripts/jest/setupEnvironment.js

@@ -7,14 +7,6 @@ if (NODE_ENV !== 'development' && NODE_ENV !== 'production') {
 global.__DEV__ = NODE_ENV === 'development';
 global.__PROFILE__ = NODE_ENV === 'development';
 
-// By default React console.error()'s any errors, caught or uncaught.
-// However it is annoying to assert that a warning fired each time
-// we assert that there is an exception in our tests. This lets us
-// opt out of extra console error reporting for most tests except
-// for the few that specifically test the logging by shadowing this
-// property. In real apps, it would usually not be defined at all.
-Error.prototype.suppressReactErrorLogging = true;
-
 if (typeof window !== 'undefined') {
   global.requestAnimationFrame = function(callback) {
     setTimeout(callback);
@@ -33,16 +25,4 @@ if (typeof window !== 'undefined') {
   global.cancelIdleCallback = function(callbackID) {
     clearTimeout(callbackID);
   };
-
-  // Same as we did with Error.prototype above.
-  DOMException.prototype.suppressReactErrorLogging = true;
-
-  // Also prevent JSDOM from logging intentionally thrown errors.
-  // TODO: it might make sense to do it the other way around.
-  // https://github.com/facebook/react/issues/11098#issuecomment-355032539
-  window.addEventListener('error', event => {
-    if (event.error != null && event.error.suppressReactErrorLogging) {
-      event.preventDefault();
-    }
-  });
 }

scripts/jest/setupTests.js

@@ -2,6 +2,7 @@
 
 const chalk = require('chalk');
 const util = require('util');
+const shouldIgnoreConsoleError = require('./shouldIgnoreConsoleError');
 
 if (process.env.REACT_CLASS_EQUIVALENCE_TEST) {
   // Inside the class equivalence tester, we have a custom environment, let's
@@ -66,6 +67,12 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) {
   ['error', 'warn'].forEach(methodName => {
     const unexpectedConsoleCallStacks = [];
     const newMethod = function(format, ...args) {
+      // Ignore uncaught errors reported by jsdom
+      // and React addendums because they're too noisy.
+      if (methodName === 'error' && shouldIgnoreConsoleError(format, args)) {
+        return;
+      }
+
       // Capture the call stack now so we can warn about it later.
       // The call stack has helpful information for the test author.
       // Don't throw yet though b'c it might be accidentally caught and suppressed.

scripts/jest/shouldIgnoreConsoleError.js

@@ -0,0 +1,31 @@
+'use strict';
+
+module.exports = function shouldIgnoreConsoleError(format, args) {
+  if (__DEV__) {
+    if (typeof format === 'string') {
+      if (format.indexOf('Error: Uncaught [') === 0) {
+        // This looks like an uncaught error from invokeGuardedCallback() wrapper
+        // in development that is reported by jsdom. Ignore because it's noisy.
+        return true;
+      }
+      if (format.indexOf('The above error occurred') === 0) {
+        // This looks like an error addendum from ReactFiberErrorLogger.
+        // Ignore it too.
+        return true;
+      }
+    }
+  } else {
+    if (
+      format != null &&
+      typeof format.message === 'string' &&
+      typeof format.stack === 'string' &&
+      args.length === 0
+    ) {
+      // In production, ReactFiberErrorLogger logs error objects directly.
+      // They are noisy too so we'll try to ignore them.
+      return true;
+    }
+  }
+  // Looks legit
+  return false;
+};