Skip to content

Commit

Permalink
fix logbox error stack (#47303)
Browse files Browse the repository at this point in the history 
* 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]>
  • Loading branch information
@blakef @rickhanlonii
blakef and rickhanlonii authored Oct 31, 2024
1 parent dac6d50 commit bbe5e72
Show file tree
Hide file tree
Showing 4 changed files with 325 additions and 76 deletions.
4 changes: 2 additions & 2 deletions packages/react-native/Libraries/LogBox/Data/LogBoxData.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ let warningFilter: WarningFilter = function (format) {
return {
finalFormat: format,
forceDialogImmediately: false,
suppressDialog_LEGACY: true,
suppressDialog_LEGACY: false,
suppressCompletely: false,
monitorEvent: 'unknown',
monitorEvent: 'warning_unhandled',
monitorListVersion: 0,
monitorSampleRate: 1,
};
Expand Down
213 changes: 169 additions & 44 deletions packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,56 +11,44 @@
import {
DoesNotUseKey,
FragmentWithProp,
ManualConsoleError,
ManualConsoleErrorWithStack,
} from './__fixtures__/ReactWarningFixtures';
import * as React from 'react';

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

const installLogBox = () => {
const LogBox = require('../LogBox');
const ExceptionsManager = require('../../Core/ExceptionsManager.js');

const installLogBox = () => {
const LogBox = require('../LogBox').default;
LogBox.install();
};

const uninstallLogBox = () => {
const LogBox = require('../LogBox');
const LogBox = require('../LogBox').default;
LogBox.uninstall();
};

const BEFORE_SLASH_RE = /(?:\/[a-zA-Z]+\/)(.+?)(?:\/.+)\//;

const cleanPath = message => {
return message.replace(BEFORE_SLASH_RE, '/path/to/');
};

const cleanLog = logs => {
return logs.map(log => {
return {
...log,
componentStack: log.componentStack.map(stack => ({
...stack,
fileName: cleanPath(stack.fileName),
})),
};
});
};

// TODO(T71117418): Re-enable skipped LogBox integration tests once React component
// stack frames are the same internally and in open source.
// eslint-disable-next-line jest/no-disabled-tests
describe.skip('LogBox', () => {
// TODO: we can remove all the symetric matchers once OSS lands component stack frames.
// For now, the component stack parsing differs in ways we can't easily detect in this test.
describe('LogBox', () => {
const {error, warn} = console;
const mockError = jest.fn();
const mockWarn = jest.fn();

beforeEach(() => {
jest.resetModules();
jest.restoreAllMocks();
jest.spyOn(console, 'error').mockImplementation(() => {});

mockError.mockClear();
mockWarn.mockClear();

// Reset ExceptionManager patching.
if (console._errorOriginal) {
console._errorOriginal = null;
}
(console: any).error = mockError;
(console: any).warn = mockWarn;
});
Expand All @@ -79,7 +67,10 @@ describe.skip('LogBox', () => {
// so we can assert on what React logs.
jest.spyOn(console, 'error');

const output = TestRenderer.create(<DoesNotUseKey />);
let output;
TestRenderer.act(() => {
output = TestRenderer.create(<DoesNotUseKey />);
});

// The key error should always be the highest severity.
// In LogBox, we expect these errors to:
Expand All @@ -88,16 +79,37 @@ describe.skip('LogBox', () => {
// - Pass to console.error, with a "Warning" prefix so it does not pop a RedBox.
expect(output).toBeDefined();
expect(mockWarn).not.toBeCalled();
expect(console.error.mock.calls[0].map(cleanPath)).toMatchSnapshot(
'Log sent from React',
);
expect(cleanLog(spy.mock.calls[0])).toMatchSnapshot('Log added to LogBox');
expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot(
'Log passed to console error',
);
expect(console.error).toBeCalledTimes(1);
expect(console.error.mock.calls[0]).toEqual([
'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',
'\n\nCheck the render method of `DoesNotUseKey`.',
'',
expect.stringMatching('at DoesNotUseKey'),
]);
expect(spy).toHaveBeenCalledWith({
level: 'error',
category: expect.stringContaining(
'Warning: Each child in a list should have a unique',
),
componentStack: expect.anything(),
componentStackType: 'stack',
message: {
content:
'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.',
substitutions: [
{length: 45, offset: 62},
{length: 0, offset: 107},
],
},
});

// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true);
// We also interpolate the string before passing to the underlying console method.
expect(mockError.mock.calls[0]).toEqual([
expect.stringMatching(
'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 ',
),
]);
});

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

const output = TestRenderer.create(<FragmentWithProp />);
let output;
TestRenderer.act(() => {
output = TestRenderer.create(<FragmentWithProp />);
});

// The fragment warning is not as severe. For this warning we don't want to
// pop open a dialog, so we show a collapsed error UI.
Expand All @@ -118,15 +133,125 @@ describe.skip('LogBox', () => {
// - Pass to console.error, with a "Warning" prefix so it does not pop a RedBox.
expect(output).toBeDefined();
expect(mockWarn).not.toBeCalled();
expect(console.error.mock.calls[0].map(cleanPath)).toMatchSnapshot(
'Log sent from React',
);
expect(cleanLog(spy.mock.calls[0])).toMatchSnapshot('Log added to LogBox');
expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot(
'Log passed to console error',
);
expect(console.error).toBeCalledTimes(1);
expect(console.error.mock.calls[0]).toEqual([
'Warning: Invalid prop `%s` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.%s',
'invalid',
expect.stringMatching('at FragmentWithProp'),
]);
expect(spy).toHaveBeenCalledWith({
level: 'error',
category: expect.stringContaining('Warning: Invalid prop'),
componentStack: expect.anything(),
componentStackType: expect.stringMatching(/(stack|legacy)/),
message: {
content:
'Warning: Invalid prop `invalid` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.',
substitutions: [{length: 7, offset: 23}],
},
});

// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
// We also interpolate the string before passing to the underlying console method.
expect(mockError.mock.calls[0]).toEqual([
expect.stringMatching(
'Warning: Invalid prop `invalid` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.\n at FragmentWithProp',
),
]);
});

it('handles a manual console.error without a component stack in LogBox', () => {
const LogBox = require('../LogBox').default;
const spy = jest.spyOn(LogBox, 'addException');
installLogBox();

// console.error handling depends on installing the ExceptionsManager error reporter.
ExceptionsManager.installConsoleErrorReporter();

// Spy console.error after LogBox is installed
// so we can assert on what React logs.
jest.spyOn(console, 'error');

let output;
TestRenderer.act(() => {
output = TestRenderer.create(<ManualConsoleError />);
});

// Manual console errors should show a collapsed error dialog.
// When there is no component stack, we expect these errors to:
// - Go to the LogBox patch and fall through to console.error.
// - Get picked up by the ExceptionsManager console.error override.
// - Get passed back to LogBox via addException (non-fatal).
expect(output).toBeDefined();
expect(mockWarn).not.toBeCalled();
expect(spy).toBeCalledTimes(1);
expect(console.error).toBeCalledTimes(1);
expect(console.error.mock.calls[0]).toEqual(['Manual console error']);
expect(spy).toHaveBeenCalledWith({
id: 1,
isComponentError: false,
isFatal: false,
name: 'console.error',
originalMessage: 'Manual console error',
message: 'console.error: Manual console error',
extraData: expect.anything(),
componentStack: null,
stack: expect.anything(),
});

// No Warning: prefix is added due since this is falling through.
expect(mockError.mock.calls[0]).toEqual(['Manual console error']);
});

it('handles a manual console.error with a component stack in LogBox', () => {
const spy = jest.spyOn(LogBoxData, 'addLog');
installLogBox();

// console.error handling depends on installing the ExceptionsManager error reporter.
ExceptionsManager.installConsoleErrorReporter();

// Spy console.error after LogBox is installed
// so we can assert on what React logs.
jest.spyOn(console, 'error');

let output;
TestRenderer.act(() => {
output = TestRenderer.create(<ManualConsoleErrorWithStack />);
});

// Manual console errors should show a collapsed error dialog.
// When there is a component stack, we expect these errors to:
// - Go to the LogBox patch and be detected as a React error.
// - Check the warning filter to see if there is a fiter setting.
// - Call console.error with the parsed error.
// - Get picked up by ExceptionsManager console.error override.
// - Log to console.error.
expect(output).toBeDefined();
expect(mockWarn).not.toBeCalled();
expect(console.error).toBeCalledTimes(1);
expect(spy).toBeCalledTimes(1);
expect(console.error.mock.calls[0]).toEqual([
expect.stringContaining(
'Manual console error\n at ManualConsoleErrorWithStack',
),
]);
expect(spy).toHaveBeenCalledWith({
level: 'error',
category: expect.stringContaining('Warning: Manual console error'),
componentStack: expect.anything(),
componentStackType: 'stack',
message: {
content: 'Warning: Manual console error',
substitutions: [],
},
});

// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true);
// We also interpolate the string before passing to the underlying console method.
expect(mockError.mock.calls[0]).toEqual([
expect.stringMatching(
'Warning: Manual console error\n at ManualConsoleErrorWithStack',
),
]);
});
});
Loading

0 comments on commit bbe5e72

Please sign in to comment.