Skip to content

Remove global mocks by adding a global flag #11632

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Remove global mocks by adding a global flag #11632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba89319
Remove global mocks
gaearon Nov 22, 2017
9984eac
Add a global flag to disable logging
gaearon Nov 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@
".*": "./scripts/jest/preprocessor.js"
},
"setupFiles": [
"./scripts/jest/setup.js",
"./scripts/jest/environment.js"
],
"setupTestFrameworkScriptFile": "./scripts/jest/test-framework-setup.js",
Expand Down
16 changes: 15 additions & 1 deletion packages/react-reconciler/src/ReactFiberErrorLogger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,21 @@ import type {CapturedError} from './ReactFiberScheduler';

import invariant from 'fbjs/lib/invariant';

const defaultShowDialog = (capturedError: CapturedError) => true;
declare var __REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__: void | boolean;

const defaultShowDialog = (capturedError: CapturedError) => {
// This is a somewhat hacky way to disable error logging.
// Can be handy in tests.
// We can revisit this when we offer a more comprehensive
// solution for overriding warning behavior.
if (
typeof __REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ !== 'undefined' &&
__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ === true
) {
return false;
}
Copy link
Contributor

@bvaughn bvaughn Nov 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃

return (
  typeof __REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ === 'undefined' ||
  __REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ !== true
);

Edit for clarity: This is just a nit. It seems like you could combine the if-condition-return into just a return.

return true;
};

let showDialog = defaultShowDialog;

Expand Down
71 changes: 16 additions & 55 deletions packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ describe('ReactIncrementalErrorHandling', () => {
ReactNoop = require('react-noop-renderer');
});

afterEach(() => {
jest.unmock('../ReactFiberErrorLogger');
});

function div(...children) {
children = children.map(c => (typeof c === 'string' ? {text: c} : c));
return {type: 'div', children, prop: undefined};
Expand Down Expand Up @@ -952,44 +956,14 @@ describe('ReactIncrementalErrorHandling', () => {
});

describe('ReactFiberErrorLogger', () => {
function initReactFiberErrorLoggerMock(mock) {
jest.resetModules();
if (mock) {
jest.mock('../ReactFiberErrorLogger');
} else {
jest.unmock('../ReactFiberErrorLogger');
}
React = require('react');
ReactNoop = require('react-noop-renderer');
}

beforeEach(() => {
// Assert that we're mocking this file by default.
// If this ever changes, we'll need to update this test suite anyway.
expect(
require('../ReactFiberErrorLogger').logCapturedError._isMockFunction,
).toBe(true);
});

afterEach(() => {
// Restore the default (we verified it was being mocked in beforeEach).
jest.mock('../ReactFiberErrorLogger');
});

function normalizeCodeLocInfo(str) {
return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

it('should log errors that occur during the begin phase', () => {
initReactFiberErrorLoggerMock(false);
// Note: It is globally true by default.
global.__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ = false;
// Intentionally spy in production too.
// TODO: It is confusing how these tests correctly "see" console.error()s
// from exceptions becase they called initReactFiberErrorLoggerMock(false)
// and thus unmocked the error logger. It is globally mocked in all other
// tests. We did this to silence warnings from intentionally caught errors
// in tests. But perhaps we should instead make a pass through all tests,
// intentionally assert console.error() is always called for uncaught
// exceptions, and then remove the global mock of ReactFiberErrorLogger.
spyOn(console, 'error');

class ErrorThrowingComponent extends React.Component {
Expand Down Expand Up @@ -1030,14 +1004,9 @@ describe('ReactIncrementalErrorHandling', () => {
});

it('should log errors that occur during the commit phase', () => {
initReactFiberErrorLoggerMock(false);
// TODO: It is confusing how these tests correctly "see" console.error()s
// from exceptions becase they called initReactFiberErrorLoggerMock(false)
// and thus unmocked the error logger. It is globally mocked in all other
// tests. We did this to silence warnings from intentionally caught errors
// in tests. But perhaps we should instead make a pass through all tests,
// intentionally assert console.error() is always called for uncaught
// exceptions, and then remove the global mock of ReactFiberErrorLogger.
// Note: It is globally true by default.
global.__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ = false;
// Intentionally spy in production too.
spyOn(console, 'error');

class ErrorThrowingComponent extends React.Component {
Expand Down Expand Up @@ -1078,13 +1047,10 @@ describe('ReactIncrementalErrorHandling', () => {
});

it('should ignore errors thrown in log method to prevent cycle', () => {
initReactFiberErrorLoggerMock(true);
// Intentionally spy in production too.
// Note: we're seeing console.error() even though ReactFiberErrorLogger is
// mocked because we're currently testing the console.error() call inside
// ReactFiberScheduler (for cycles). It doesn't get affected by mocking.
// TODO: mocking is confusing and we should simplify this and remove
// special cases.
jest.resetModules();
jest.mock('../ReactFiberErrorLogger');
React = require('react');
ReactNoop = require('react-noop-renderer');
spyOn(console, 'error');

class ErrorThrowingComponent extends React.Component {
Expand Down Expand Up @@ -1124,14 +1090,9 @@ describe('ReactIncrementalErrorHandling', () => {
});

it('should relay info about error boundary and retry attempts if applicable', () => {
initReactFiberErrorLoggerMock(false);
// TODO: It is confusing how these tests correctly "see" console.error()s
// from exceptions becase they called initReactFiberErrorLoggerMock(false)
// and thus unmocked the error logger. It is globally mocked in all other
// tests. We did this to silence warnings from intentionally caught errors
// in tests. But perhaps we should instead make a pass through all tests,
// intentionally assert console.error() is always called for uncaught
// exceptions, and then remove the global mock of ReactFiberErrorLogger.
// Note: It is globally true by default.
global.__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ = false;
// Intentionally spy in production too.
spyOn(console, 'error');

class ParentComponent extends React.Component {
Expand Down
12 changes: 0 additions & 12 deletions scripts/jest/setup.js
View file
Open in desktop

This file was deleted.

8 changes: 7 additions & 1 deletion scripts/jest/setupSpecEquivalenceReporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,13 @@ global.spyOnDev = function(...args) {
}
};

beforeEach(() => (numExpectations = 0));
beforeEach(() => {
numExpectations = 0;
// Suppresses additional console.error() when an error is caught by React.
// Unless error boundary catches it, React will rethrow it anyway.
// We need this in tests or they become too noisy (we often assert on errors).
global.__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ = true;
});

jasmine.currentEnv_.addReporter({
specDone: spec => {
Expand Down
5 changes: 5 additions & 0 deletions scripts/jest/test-framework-setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ if (process.env.REACT_CLASS_EQUIVALENCE_TEST) {

env.beforeEach(() => {
newMethod.__callCount = 0;

// Suppresses additional console.error() when an error is caught by React.
// Unless error boundary catches it, React will rethrow it anyway.
// We need this in tests or they become too noisy (we often assert on errors).
global.__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__ = true;
});

env.afterEach(() => {
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.cjs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module.exports = {
// Vendor specific
MSApp: true,
__REACT_DEVTOOLS_GLOBAL_HOOK__: true,
__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__: true,
// CommonJS / Node
process: true,
},
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module.exports = {
// Vendor specific
MSApp: true,
__REACT_DEVTOOLS_GLOBAL_HOOK__: true,
__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__: true,
// FB
__DEV__: true,
},
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.rn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module.exports = {
// Vendor specific
MSApp: true,
__REACT_DEVTOOLS_GLOBAL_HOOK__: true,
__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__: true,
// FB
__DEV__: true,
},
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.umd.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module.exports = {
// Vendor specific
MSApp: true,
__REACT_DEVTOOLS_GLOBAL_HOOK__: true,
__REACT_UNSTABLE_SUPPRESS_ERROR_LOGGING__: true,
// UMD wrapper code
// TODO: this is too permissive.
// Ideally we should only allow these *inside* the UMD wrapper.
Expand Down