Skip to content

Commit bbe5e72

Browse files
blakefrickhanlonii
andauthored
fix logbox error stack (#47303)
* Re-enable integration tests (#46639) Summary: Pull Request resolved: #46639 These tests were skipped when we were switching to component stacks, which also hid a bug later in the stack. Re-enable them. Changelog: [Internal] Reviewed By: javache Differential Revision: D63349616 fbshipit-source-id: ccde7d5bb3fcd9a27adf4af2068a160f02f7432a * Add integration tests for console errors + ExceptionManager (#46636) Summary: Pull Request resolved: #46636 Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff). Changelog: [Internal] Reviewed By: javache Differential Revision: D63349614 fbshipit-source-id: 8f5c6545b48a1ed18aea08d4ecbecd7a6b9fa05a * Refactor LogBox tests to spies (#46638) Summary: Pull Request resolved: #46638 This is annoying, but in the next diff that fixes a bug I need to test using the default warning filter instead of a mock (really, all this mocking is terrible, idk why I did it this way). Unfortunately, in Jest you can't just reset mocks from `jest.mock`, `restoreMocks` only resets spies and not mocks (wild right). So in this diff I converted all the `jest.mock` calls to `jest.spyOn`. I also corrected some of the mocks that require `monitorEvent: 'warning',` like the warning filter sets. I also added a test that works without the fix. Changelog: [Internal] Reviewed By: javache Differential Revision: D63349615 fbshipit-source-id: 4f2a5a8800c8fe1a10e3613d3c2d0ed02fca773e * Fix errors with component stacks reported as warnings (#46637) Summary: Ok so this is a doozy. ## Overview There was a report that some console.error calls were being shown as warnings in LogBox but as console.error in the console. The only time we should downlevel an error to a warning is if the custom warning filter says so (which is used for some noisy legacy warning filter warnings internally). However, in when I switched from using the `Warning: ` prefix, to using the presence of component stacks, I subtly missed the default warning filter case. In the internal warning filter, the `monitorEvent` is always set to something other than `unknown` and if it's set to `warning_unhandled` then `suppressDialog_LEGACY` is always false. However, the default values for the warning filter are that `monitorEvent = 'unknown'` and `suppressDialog_LEGACY = true`. In this case, we would downlevel the error to a warning. ## What's the fix? Change the default settings for the warning filter. ## What's the root cause? Bad configuration combinations in a fragile system that needs cleaned up, and really really bad testing practices with excessive mocking and snapshot testing (I can say that, I wrote the tests) ## How could it have been caught? It was, but I turned off the integration tests while landing the component stack changes because of mismatches between flags internally and in OSS, and never turned them back on. Changelog: [General] [Fixed] - Fix logbox reporting React errors as Warnings Pull Request resolved: #46637 Reviewed By: huntie Differential Revision: D63349613 Pulled By: rickhanlonii fbshipit-source-id: 32e3fa4e2f2077114a6e9f4feac73673973ab50c * [LOCAL] using older version of React Dev Tools - Older version has old URL, updated tests - Comments on test don't match what's being tested. Updated. --------- Co-authored-by: Rick Hanlon <[email protected]>
1 parent dac6d50 commit bbe5e72

File tree

4 files changed

+325
-76
lines changed

4 files changed

+325
-76
lines changed

packages/react-native/Libraries/LogBox/Data/LogBoxData.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,9 @@ let warningFilter: WarningFilter = function (format) {
8282
return {
8383
finalFormat: format,
8484
forceDialogImmediately: false,
85-
suppressDialog_LEGACY: true,
85+
suppressDialog_LEGACY: false,
8686
suppressCompletely: false,
87-
monitorEvent: 'unknown',
87+
monitorEvent: 'warning_unhandled',
8888
monitorListVersion: 0,
8989
monitorSampleRate: 1,
9090
};

packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js

Lines changed: 169 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -11,56 +11,44 @@
1111
import {
1212
DoesNotUseKey,
1313
FragmentWithProp,
14+
ManualConsoleError,
15+
ManualConsoleErrorWithStack,
1416
} from './__fixtures__/ReactWarningFixtures';
1517
import * as React from 'react';
1618

1719
const LogBoxData = require('../Data/LogBoxData');
1820
const TestRenderer = require('react-test-renderer');
1921

20-
const installLogBox = () => {
21-
const LogBox = require('../LogBox');
22+
const ExceptionsManager = require('../../Core/ExceptionsManager.js');
2223

24+
const installLogBox = () => {
25+
const LogBox = require('../LogBox').default;
2326
LogBox.install();
2427
};
2528

2629
const uninstallLogBox = () => {
27-
const LogBox = require('../LogBox');
30+
const LogBox = require('../LogBox').default;
2831
LogBox.uninstall();
2932
};
3033

31-
const BEFORE_SLASH_RE = /(?:\/[a-zA-Z]+\/)(.+?)(?:\/.+)\//;
32-
33-
const cleanPath = message => {
34-
return message.replace(BEFORE_SLASH_RE, '/path/to/');
35-
};
36-
37-
const cleanLog = logs => {
38-
return logs.map(log => {
39-
return {
40-
...log,
41-
componentStack: log.componentStack.map(stack => ({
42-
...stack,
43-
fileName: cleanPath(stack.fileName),
44-
})),
45-
};
46-
});
47-
};
48-
49-
// TODO(T71117418): Re-enable skipped LogBox integration tests once React component
50-
// stack frames are the same internally and in open source.
51-
// eslint-disable-next-line jest/no-disabled-tests
52-
describe.skip('LogBox', () => {
34+
// TODO: we can remove all the symetric matchers once OSS lands component stack frames.
35+
// For now, the component stack parsing differs in ways we can't easily detect in this test.
36+
describe('LogBox', () => {
5337
const {error, warn} = console;
5438
const mockError = jest.fn();
5539
const mockWarn = jest.fn();
5640

5741
beforeEach(() => {
5842
jest.resetModules();
5943
jest.restoreAllMocks();
44+
jest.spyOn(console, 'error').mockImplementation(() => {});
6045

6146
mockError.mockClear();
6247
mockWarn.mockClear();
63-
48+
// Reset ExceptionManager patching.
49+
if (console._errorOriginal) {
50+
console._errorOriginal = null;
51+
}
6452
(console: any).error = mockError;
6553
(console: any).warn = mockWarn;
6654
});
@@ -79,7 +67,10 @@ describe.skip('LogBox', () => {
7967
// so we can assert on what React logs.
8068
jest.spyOn(console, 'error');
8169

82-
const output = TestRenderer.create(<DoesNotUseKey />);
70+
let output;
71+
TestRenderer.act(() => {
72+
output = TestRenderer.create(<DoesNotUseKey />);
73+
});
8374

8475
// The key error should always be the highest severity.
8576
// In LogBox, we expect these errors to:
@@ -88,16 +79,37 @@ describe.skip('LogBox', () => {
8879
// - Pass to console.error, with a "Warning" prefix so it does not pop a RedBox.
8980
expect(output).toBeDefined();
9081
expect(mockWarn).not.toBeCalled();
91-
expect(console.error.mock.calls[0].map(cleanPath)).toMatchSnapshot(
92-
'Log sent from React',
93-
);
94-
expect(cleanLog(spy.mock.calls[0])).toMatchSnapshot('Log added to LogBox');
95-
expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot(
96-
'Log passed to console error',
97-
);
82+
expect(console.error).toBeCalledTimes(1);
83+
expect(console.error.mock.calls[0]).toEqual([
84+
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://reactjs.org/link/warning-keys for more information.%s',
85+
'\n\nCheck the render method of `DoesNotUseKey`.',
86+
'',
87+
expect.stringMatching('at DoesNotUseKey'),
88+
]);
89+
expect(spy).toHaveBeenCalledWith({
90+
level: 'error',
91+
category: expect.stringContaining(
92+
'Warning: Each child in a list should have a unique',
93+
),
94+
componentStack: expect.anything(),
95+
componentStackType: 'stack',
96+
message: {
97+
content:
98+
'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `DoesNotUseKey`. See https://reactjs.org/link/warning-keys for more information.',
99+
substitutions: [
100+
{length: 45, offset: 62},
101+
{length: 0, offset: 107},
102+
],
103+
},
104+
});
98105

99106
// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
100-
expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true);
107+
// We also interpolate the string before passing to the underlying console method.
108+
expect(mockError.mock.calls[0]).toEqual([
109+
expect.stringMatching(
110+
'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `DoesNotUseKey`. See https://reactjs.org/link/warning-keys for more information.\n at ',
111+
),
112+
]);
101113
});
102114

103115
it('integrates with React and handles a fragment warning in LogBox', () => {
@@ -108,7 +120,10 @@ describe.skip('LogBox', () => {
108120
// so we can assert on what React logs.
109121
jest.spyOn(console, 'error');
110122

111-
const output = TestRenderer.create(<FragmentWithProp />);
123+
let output;
124+
TestRenderer.act(() => {
125+
output = TestRenderer.create(<FragmentWithProp />);
126+
});
112127

113128
// The fragment warning is not as severe. For this warning we don't want to
114129
// pop open a dialog, so we show a collapsed error UI.
@@ -118,15 +133,125 @@ describe.skip('LogBox', () => {
118133
// - Pass to console.error, with a "Warning" prefix so it does not pop a RedBox.
119134
expect(output).toBeDefined();
120135
expect(mockWarn).not.toBeCalled();
121-
expect(console.error.mock.calls[0].map(cleanPath)).toMatchSnapshot(
122-
'Log sent from React',
123-
);
124-
expect(cleanLog(spy.mock.calls[0])).toMatchSnapshot('Log added to LogBox');
125-
expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot(
126-
'Log passed to console error',
127-
);
136+
expect(console.error).toBeCalledTimes(1);
137+
expect(console.error.mock.calls[0]).toEqual([
138+
'Warning: Invalid prop `%s` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.%s',
139+
'invalid',
140+
expect.stringMatching('at FragmentWithProp'),
141+
]);
142+
expect(spy).toHaveBeenCalledWith({
143+
level: 'error',
144+
category: expect.stringContaining('Warning: Invalid prop'),
145+
componentStack: expect.anything(),
146+
componentStackType: expect.stringMatching(/(stack|legacy)/),
147+
message: {
148+
content:
149+
'Warning: Invalid prop `invalid` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.',
150+
substitutions: [{length: 7, offset: 23}],
151+
},
152+
});
153+
154+
// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
155+
// We also interpolate the string before passing to the underlying console method.
156+
expect(mockError.mock.calls[0]).toEqual([
157+
expect.stringMatching(
158+
'Warning: Invalid prop `invalid` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.\n at FragmentWithProp',
159+
),
160+
]);
161+
});
162+
163+
it('handles a manual console.error without a component stack in LogBox', () => {
164+
const LogBox = require('../LogBox').default;
165+
const spy = jest.spyOn(LogBox, 'addException');
166+
installLogBox();
167+
168+
// console.error handling depends on installing the ExceptionsManager error reporter.
169+
ExceptionsManager.installConsoleErrorReporter();
170+
171+
// Spy console.error after LogBox is installed
172+
// so we can assert on what React logs.
173+
jest.spyOn(console, 'error');
174+
175+
let output;
176+
TestRenderer.act(() => {
177+
output = TestRenderer.create(<ManualConsoleError />);
178+
});
179+
180+
// Manual console errors should show a collapsed error dialog.
181+
// When there is no component stack, we expect these errors to:
182+
// - Go to the LogBox patch and fall through to console.error.
183+
// - Get picked up by the ExceptionsManager console.error override.
184+
// - Get passed back to LogBox via addException (non-fatal).
185+
expect(output).toBeDefined();
186+
expect(mockWarn).not.toBeCalled();
187+
expect(spy).toBeCalledTimes(1);
188+
expect(console.error).toBeCalledTimes(1);
189+
expect(console.error.mock.calls[0]).toEqual(['Manual console error']);
190+
expect(spy).toHaveBeenCalledWith({
191+
id: 1,
192+
isComponentError: false,
193+
isFatal: false,
194+
name: 'console.error',
195+
originalMessage: 'Manual console error',
196+
message: 'console.error: Manual console error',
197+
extraData: expect.anything(),
198+
componentStack: null,
199+
stack: expect.anything(),
200+
});
201+
202+
// No Warning: prefix is added due since this is falling through.
203+
expect(mockError.mock.calls[0]).toEqual(['Manual console error']);
204+
});
205+
206+
it('handles a manual console.error with a component stack in LogBox', () => {
207+
const spy = jest.spyOn(LogBoxData, 'addLog');
208+
installLogBox();
209+
210+
// console.error handling depends on installing the ExceptionsManager error reporter.
211+
ExceptionsManager.installConsoleErrorReporter();
212+
213+
// Spy console.error after LogBox is installed
214+
// so we can assert on what React logs.
215+
jest.spyOn(console, 'error');
216+
217+
let output;
218+
TestRenderer.act(() => {
219+
output = TestRenderer.create(<ManualConsoleErrorWithStack />);
220+
});
221+
222+
// Manual console errors should show a collapsed error dialog.
223+
// When there is a component stack, we expect these errors to:
224+
// - Go to the LogBox patch and be detected as a React error.
225+
// - Check the warning filter to see if there is a fiter setting.
226+
// - Call console.error with the parsed error.
227+
// - Get picked up by ExceptionsManager console.error override.
228+
// - Log to console.error.
229+
expect(output).toBeDefined();
230+
expect(mockWarn).not.toBeCalled();
231+
expect(console.error).toBeCalledTimes(1);
232+
expect(spy).toBeCalledTimes(1);
233+
expect(console.error.mock.calls[0]).toEqual([
234+
expect.stringContaining(
235+
'Manual console error\n at ManualConsoleErrorWithStack',
236+
),
237+
]);
238+
expect(spy).toHaveBeenCalledWith({
239+
level: 'error',
240+
category: expect.stringContaining('Warning: Manual console error'),
241+
componentStack: expect.anything(),
242+
componentStackType: 'stack',
243+
message: {
244+
content: 'Warning: Manual console error',
245+
substitutions: [],
246+
},
247+
});
128248

129249
// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
130-
expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true);
250+
// We also interpolate the string before passing to the underlying console method.
251+
expect(mockError.mock.calls[0]).toEqual([
252+
expect.stringMatching(
253+
'Warning: Manual console error\n at ManualConsoleErrorWithStack',
254+
),
255+
]);
131256
});
132257
});

0 commit comments

Comments
 (0)