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
Brian Vaughn committed
Jan 5, 2018 at 09:44 UTC
c94b4b8b86c72a450c74ca763a509aefd481a2b4
3 files changed
+72
-78
packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js
+2
@@ -41,6 +41,8 @@ module.exports = function(initModules) {
41
if (console.error.calls && console.error.calls.reset) {
42
console.error.calls.reset();
43
} else {
44
+ // TODO: Rewrite tests that use this helper to enumerate expeceted errors.
45
+ // This will enable the helper to use the .toWarnDev() matcher instead of spying.
46
spyOnDev(console, 'error');
47
}
48
packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js
+53
-70
@@ -20,12 +20,10 @@ describe('ReactIncrementalErrorLogging', () => {
20
ReactNoop = require('react-noop-renderer');
21
});
22
23
- function normalizeCodeLocInfo(str) {
24
- return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
25
- }
26
-
23
it('should log errors that occur during the begin phase', () => {
28
- spyOnDevAndProd(console, 'error');
24
+ // Errors are redundantly logged in production mode by ReactFiberErrorLogger.
25
+ // It's okay to ignore them for the purpose of this test.
26
+ spyOnProd(console, 'error');
27
28
class ErrorThrowingComponent extends React.Component {
29
componentWillMount() {
@@ -41,36 +39,29 @@ describe('ReactIncrementalErrorLogging', () => {
39
}
40
}
41
44
- try {
45
- ReactNoop.render(
46
- <div>
47
- <span>
48
- <ErrorThrowingComponent />
49
- </span>
50
- </div>,
51
- );
52
- ReactNoop.flushDeferredPri();
53
- } catch (error) {}
42
+ ReactNoop.render(
43
+ <div>
44
+ <span>
45
+ <ErrorThrowingComponent />
46
+ </span>
47
+ </div>,
48
+ );
49
55
- expect(console.error.calls.count()).toBe(1);
56
- const errorMessage = console.error.calls.argsFor(0)[0];
57
- if (__DEV__) {
58
- expect(normalizeCodeLocInfo(errorMessage)).toContain(
50
+ expect(() => {
51
+ expect(ReactNoop.flushDeferredPri).toWarnDev(
52
'The above error occurred in the <ErrorThrowingComponent> component:\n' +
53
' in ErrorThrowingComponent (at **)\n' +
54
' in span (at **)\n' +
62
- ' in div (at **)',
55
+ ' in div (at **)\n\n' +
56
+ 'Consider adding an error boundary to your tree to customize error handling behavior.',
57
);
64
- expect(errorMessage).toContain(
65
- 'Consider adding an error boundary to your tree to customize error handling behavior.',
66
- );
67
- } else {
68
- expect(errorMessage.message).toContain('componentWillMount error');
69
- }
58
+ }).toThrowError('componentWillMount error');
59
});
60
61
it('should log errors that occur during the commit phase', () => {
73
- spyOnDevAndProd(console, 'error');
62
+ // Errors are redundantly logged in production mode by ReactFiberErrorLogger.
63
+ // It's okay to ignore them for the purpose of this test.
64
+ spyOnProd(console, 'error');
65
66
class ErrorThrowingComponent extends React.Component {
67
componentDidMount() {
@@ -86,32 +77,23 @@ describe('ReactIncrementalErrorLogging', () => {
77
}
78
}
79
89
- try {
90
- ReactNoop.render(
91
- <div>
92
- <span>
93
- <ErrorThrowingComponent />
94
- </span>
95
- </div>,
96
- );
97
- ReactNoop.flushDeferredPri();
98
- } catch (error) {}
80
+ ReactNoop.render(
81
+ <div>
82
+ <span>
83
+ <ErrorThrowingComponent />
84
+ </span>
85
+ </div>,
86
+ );
87
100
- expect(console.error.calls.count()).toBe(1);
101
- const errorMessage = console.error.calls.argsFor(0)[0];
102
- if (__DEV__) {
103
- expect(normalizeCodeLocInfo(errorMessage)).toContain(
88
+ expect(() => {
89
+ expect(ReactNoop.flushDeferredPri).toWarnDev(
90
'The above error occurred in the <ErrorThrowingComponent> component:\n' +
91
' in ErrorThrowingComponent (at **)\n' +
92
' in span (at **)\n' +
107
- ' in div (at **)',
108
- );
109
- expect(errorMessage).toContain(
110
- 'Consider adding an error boundary to your tree to customize error handling behavior.',
93
+ ' in div (at **)\n\n' +
94
+ 'Consider adding an error boundary to your tree to customize error handling behavior.',
95
);
112
- } else {
113
- expect(errorMessage.message).toBe('componentDidMount error');
114
- }
96
+ }).toThrowError('componentDidMount error');
97
});
98
99
it('should ignore errors thrown in log method to prevent cycle', () => {
@@ -120,6 +102,8 @@ describe('ReactIncrementalErrorLogging', () => {
102
try {
103
React = require('react');
104
ReactNoop = require('react-noop-renderer');
105
+
106
+ // TODO Update this test to use toWarnDev() matcher if possible
107
spyOnDevAndProd(console, 'error');
108
109
class ErrorThrowingComponent extends React.Component {
@@ -167,7 +151,9 @@ describe('ReactIncrementalErrorLogging', () => {
151
});
152
153
it('should relay info about error boundary and retry attempts if applicable', () => {
170
- spyOnDevAndProd(console, 'error');
154
+ // Errors are redundantly logged in production mode by ReactFiberErrorLogger.
155
+ // It's okay to ignore them for the purpose of this test.
156
+ spyOnProd(console, 'error');
157
158
class ParentComponent extends React.Component {
159
render() {
@@ -203,30 +189,27 @@ describe('ReactIncrementalErrorLogging', () => {
189
}
190
}
191
206
- try {
207
- ReactNoop.render(<ParentComponent />);
208
- ReactNoop.flush();
209
- } catch (error) {}
192
+ ReactNoop.render(<ParentComponent />);
193
211
- expect(renderAttempts).toBe(2);
212
- expect(handleErrorCalls.length).toBe(1);
213
- expect(console.error.calls.count()).toBe(2);
214
- if (__DEV__) {
215
- expect(console.error.calls.argsFor(0)[0]).toContain(
216
- 'The above error occurred in the <ErrorThrowingComponent> component:',
217
- );
218
- expect(console.error.calls.argsFor(0)[0]).toContain(
219
- 'React will try to recreate this component tree from scratch ' +
194
+ expect(() => {
195
+ expect(ReactNoop.flush).toWarnDev([
196
+ 'The above error occurred in the <ErrorThrowingComponent> component:\n' +
197
+ ' in ErrorThrowingComponent (at **)\n' +
198
+ ' in ErrorBoundaryComponent (at **)\n' +
199
+ ' in ParentComponent (at **)\n\n' +
200
+ 'React will try to recreate this component tree from scratch ' +
201
'using the error boundary you provided, ErrorBoundaryComponent.',
221
- );
222
- expect(console.error.calls.argsFor(1)[0]).toContain(
223
- 'The above error occurred in the <ErrorThrowingComponent> component:',
224
- );
225
- expect(console.error.calls.argsFor(1)[0]).toContain(
226
- 'This error was initially handled by the error boundary ErrorBoundaryComponent.\n' +
202
+ 'The above error occurred in the <ErrorThrowingComponent> component:\n' +
203
+ ' in ErrorThrowingComponent (at **)\n' +
204
+ ' in ErrorBoundaryComponent (at **)\n' +
205
+ ' in ParentComponent (at **)\n\n' +
206
+ 'This error was initially handled by the error boundary ErrorBoundaryComponent.\n' +
207
'Recreating the tree from scratch failed so React will unmount the tree.',
228
- );
229
- }
208
+ ]);
209
+ }).toThrowError('componentDidMount error');
210
+
211
+ expect(renderAttempts).toBe(2);
212
+ expect(handleErrorCalls.length).toBe(1);
213
});
214
});
215
scripts/jest/matchers/toWarnDev.js
+17
-8
@@ -19,6 +19,12 @@ const createMatcherFor = consoleMethod =>
19
20
const unexpectedWarnings = [];
21
22
+ // Catch errors thrown by the callback,
23
+ // But only rethrow them if all test expectations have been satisfied.
24
+ // Otherwise an Error in the callback can mask a failed expectation,
25
+ // and result in a test that passes when it shouldn't.
26
+ let caughtError;
27
+
28
const consoleSpy = message => {
29
const normalizedMessage = normalizeCodeLocInfo(message);
30
@@ -58,6 +64,11 @@ const createMatcherFor = consoleMethod =>
64
65
try {
66
callback();
67
+ } catch (error) {
68
+ caughtError = error;
69
+ } finally {
70
+ // Restore the unspied method so that unexpected errors fail tests.
71
+ console[consoleMethod] = originalMethod;
72
73
// Any unexpected warnings should be treated as a failure.
74
if (unexpectedWarnings.length > 0) {
@@ -72,20 +83,18 @@ const createMatcherFor = consoleMethod =>
83
return {
84
message: () =>
85
`Expected warning was not recorded:\n ${this.utils.printReceived(
75
- expectedMessages.join('\n')
86
+ expectedMessages[0]
87
)}`,
88
pass: false,
89
};
90
}
91
92
+ // Any unexpected Errors thrown by the callback should fail the test.
93
+ if (caughtError) {
94
+ throw caughtError;
95
+ }
96
+
97
return {pass: true};
82
- } catch (error) {
83
- // TODO Flag this error so Jest doesn't override its stack
84
- // See https://tinyurl.com/y9unakwb
85
- throw error;
86
- } finally {
87
- // Restore the unspied method so that unexpected errors fail tests.
88
- console[consoleMethod] = originalMethod;
98
}
99
} else {
100
// Any uncaught errors or warnings should fail tests in production mode.