From c5680ff124301e45f7a6d6986d147dc3c37a5104 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Wed, 25 Sep 2024 19:28:05 -0700 Subject: [PATCH 1/5] Re-enable integration tests (#46639) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/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 --- .../__tests__/LogBox-integration-test.js | 91 ++++++++++++++----- 1 file changed, 66 insertions(+), 25 deletions(-) diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js index 07ea7507d777..5887e772f130 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js @@ -18,13 +18,13 @@ const LogBoxData = require('../Data/LogBoxData'); const TestRenderer = require('react-test-renderer'); const installLogBox = () => { - const LogBox = require('../LogBox'); + const LogBox = require('../LogBox').default; LogBox.install(); }; const uninstallLogBox = () => { - const LogBox = require('../LogBox'); + const LogBox = require('../LogBox').default; LogBox.uninstall(); }; @@ -46,10 +46,9 @@ const cleanLog = logs => { }); }; -// 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(); @@ -57,10 +56,10 @@ describe.skip('LogBox', () => { beforeEach(() => { jest.resetModules(); jest.restoreAllMocks(); + jest.spyOn(console, 'error').mockImplementation(() => {}); mockError.mockClear(); mockWarn.mockClear(); - (console: any).error = mockError; (console: any).warn = mockWarn; }); @@ -79,7 +78,10 @@ describe.skip('LogBox', () => { // so we can assert on what React logs. jest.spyOn(console, 'error'); - const output = TestRenderer.create(); + let output; + TestRenderer.act(() => { + output = TestRenderer.create(); + }); // The key error should always be the highest severity. // In LogBox, we expect these errors to: @@ -88,16 +90,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].map(cleanPath)).toEqual([ + 'Each child in a list should have a unique "key" prop.%s%s See https://react.dev/link/warning-keys for more information.%s', + '\n\nCheck the render method of `DoesNotUseKey`.', + '', + expect.stringMatching('at DoesNotUseKey'), + ]); + expect(spy).toHaveBeenCalledWith({ + level: 'warn', + 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://react.dev/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://react.dev/link/warning-keys for more information.\n at ', + ), + ]); }); it('integrates with React and handles a fragment warning in LogBox', () => { @@ -108,7 +131,10 @@ describe.skip('LogBox', () => { // so we can assert on what React logs. jest.spyOn(console, 'error'); - const output = TestRenderer.create(); + let output; + TestRenderer.act(() => { + output = TestRenderer.create(); + }); // 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. @@ -118,15 +144,30 @@ 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].map(cleanPath)).toEqual([ + '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: 'warn', + 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. - 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: Invalid prop `invalid` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.\n at FragmentWithProp', + ), + ]); }); }); From e1932f60ea084513d4b684eee7a4975b94efde90 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Wed, 25 Sep 2024 19:28:05 -0700 Subject: [PATCH 2/5] Add integration tests for console errors + ExceptionManager (#46636) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/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 --- .../__tests__/LogBox-integration-test.js | 126 +++++++++++++++--- .../__fixtures__/ReactWarningFixtures.js | 24 ++++ 2 files changed, 129 insertions(+), 21 deletions(-) diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js index 5887e772f130..b6583f74a4a1 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js @@ -11,15 +11,18 @@ 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 ExceptionsManager = require('../../Core/ExceptionsManager.js'); + const installLogBox = () => { const LogBox = require('../LogBox').default; - LogBox.install(); }; @@ -28,24 +31,6 @@ const uninstallLogBox = () => { 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: 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', () => { @@ -60,6 +45,10 @@ describe('LogBox', () => { mockError.mockClear(); mockWarn.mockClear(); + // Reset ExceptionManager patching. + if (console._errorOriginal) { + console._errorOriginal = null; + } (console: any).error = mockError; (console: any).warn = mockWarn; }); @@ -91,7 +80,7 @@ describe('LogBox', () => { expect(output).toBeDefined(); expect(mockWarn).not.toBeCalled(); expect(console.error).toBeCalledTimes(1); - expect(console.error.mock.calls[0].map(cleanPath)).toEqual([ + expect(console.error.mock.calls[0]).toEqual([ 'Each child in a list should have a unique "key" prop.%s%s See https://react.dev/link/warning-keys for more information.%s', '\n\nCheck the render method of `DoesNotUseKey`.', '', @@ -145,7 +134,7 @@ describe('LogBox', () => { expect(output).toBeDefined(); expect(mockWarn).not.toBeCalled(); expect(console.error).toBeCalledTimes(1); - expect(console.error.mock.calls[0].map(cleanPath)).toEqual([ + expect(console.error.mock.calls[0]).toEqual([ 'Invalid prop `%s` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.%s', 'invalid', expect.stringMatching('at FragmentWithProp'), @@ -170,4 +159,99 @@ describe('LogBox', () => { ), ]); }); + + 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(); + }); + + // 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(); + }); + + // 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: 'warn', + 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. + // 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', + ), + ]); + }); }); diff --git a/packages/react-native/Libraries/LogBox/__tests__/__fixtures__/ReactWarningFixtures.js b/packages/react-native/Libraries/LogBox/__tests__/__fixtures__/ReactWarningFixtures.js index 51b85b1fbe1a..2d13e41dfe06 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/__fixtures__/ReactWarningFixtures.js +++ b/packages/react-native/Libraries/LogBox/__tests__/__fixtures__/ReactWarningFixtures.js @@ -30,3 +30,27 @@ export const FragmentWithProp = () => { ); }; + +export const ManualConsoleError = () => { + console.error('Manual console error'); + return ( + + {['foo', 'bar'].map(item => ( + {item} + ))} + + ); +}; + +export const ManualConsoleErrorWithStack = () => { + console.error( + 'Manual console error\n at ManualConsoleErrorWithStack (/path/to/ManualConsoleErrorWithStack:30:175)\n at TestApp', + ); + return ( + + {['foo', 'bar'].map(item => ( + {item} + ))} + + ); +}; From a61943ad854d2d8cbc6f75d9318eb03da0f6f37a Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Wed, 25 Sep 2024 19:28:05 -0700 Subject: [PATCH 3/5] Refactor LogBox tests to spies (#46638) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/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 --- .../Libraries/LogBox/__tests__/LogBox-test.js | 121 +++++++++++++----- 1 file changed, 91 insertions(+), 30 deletions(-) diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js index be5ea785bbb5..b44c6131ffb3 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js @@ -13,6 +13,7 @@ const LogBoxData = require('../Data/LogBoxData'); const LogBox = require('../LogBox').default; +const ExceptionsManager = require('../../Core/ExceptionsManager.js'); declare var console: any; @@ -34,15 +35,18 @@ describe('LogBox', () => { beforeEach(() => { jest.resetModules(); + jest.restoreAllMocks(); console.error = jest.fn(); - console.log = jest.fn(); console.warn = jest.fn(); }); afterEach(() => { LogBox.uninstall(); + // Reset ExceptionManager patching. + if (console._errorOriginal) { + console._errorOriginal = null; + } console.error = error; - console.log = log; console.warn = warn; }); @@ -95,7 +99,7 @@ describe('LogBox', () => { }); it('registers warnings', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); LogBox.install(); @@ -105,13 +109,14 @@ describe('LogBox', () => { }); it('reports a LogBox exception if we fail to add warnings', () => { - jest.mock('../Data/LogBoxData'); - const mockError = new Error('Simulated error'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'reportLogBoxError'); // Picking a random implementation detail to simulate throwing. - (LogBoxData.isMessageIgnored: any).mockImplementation(() => { + jest.spyOn(LogBoxData, 'isMessageIgnored').mockImplementation(() => { throw mockError; }); + const mockError = new Error('Simulated error'); LogBox.install(); @@ -123,7 +128,8 @@ describe('LogBox', () => { }); it('only registers errors beginning with "Warning: "', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); LogBox.install(); @@ -133,7 +139,8 @@ describe('LogBox', () => { }); it('registers react errors with the formatting from filter', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ finalFormat: 'Custom format', @@ -157,7 +164,8 @@ describe('LogBox', () => { }); it('registers errors with component stack as errors by default', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({}); @@ -174,7 +182,8 @@ describe('LogBox', () => { }); it('registers errors with component stack as errors by default if not found in warning filter', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ monitorEvent: 'warning_unhandled', @@ -193,10 +202,12 @@ describe('LogBox', () => { }); it('registers errors with component stack with legacy suppression as warning', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ suppressDialog_LEGACY: true, + monitorEvent: 'warning', }); LogBox.install(); @@ -211,10 +222,12 @@ describe('LogBox', () => { }); it('registers errors with component stack and a forced dialog as fatals', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ forceDialogImmediately: true, + monitorEvent: 'warning', }); LogBox.install(); @@ -229,7 +242,8 @@ describe('LogBox', () => { }); it('registers warning module errors with the formatting from filter', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ finalFormat: 'Custom format', @@ -248,7 +262,8 @@ describe('LogBox', () => { }); it('registers warning module errors as errors by default', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({}); @@ -262,10 +277,12 @@ describe('LogBox', () => { }); it('registers warning module errors with only legacy suppression as warning', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ suppressDialog_LEGACY: true, + monitorEvent: 'warning', }); LogBox.install(); @@ -277,10 +294,12 @@ describe('LogBox', () => { }); it('registers warning module errors with a forced dialog as fatals', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ forceDialogImmediately: true, + monitorEvent: 'warning', }); LogBox.install(); @@ -292,10 +311,12 @@ describe('LogBox', () => { }); it('ignores warning module errors that are suppressed completely', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); mockFilterResult({ suppressCompletely: true, + monitorEvent: 'warning', }); LogBox.install(); @@ -305,10 +326,11 @@ describe('LogBox', () => { }); it('ignores warning module errors that are pattern ignored', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'isMessageIgnored').mockReturnValue(true); + jest.spyOn(LogBoxData, 'addLog'); mockFilterResult({}); - (LogBoxData.isMessageIgnored: any).mockReturnValue(true); LogBox.install(); @@ -317,10 +339,11 @@ describe('LogBox', () => { }); it('ignores warning module errors that are from LogBox itself', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'isLogBoxErrorMessage').mockReturnValue(true); + jest.spyOn(LogBoxData, 'addLog'); mockFilterResult({}); - (LogBoxData.isLogBoxErrorMessage: any).mockReturnValue(true); LogBox.install(); @@ -329,8 +352,9 @@ describe('LogBox', () => { }); it('ignores logs that are pattern ignored"', () => { - jest.mock('../Data/LogBoxData'); - (LogBoxData.isMessageIgnored: any).mockReturnValue(true); + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'isMessageIgnored').mockReturnValue(true); + jest.spyOn(LogBoxData, 'addLog'); LogBox.install(); @@ -339,8 +363,8 @@ describe('LogBox', () => { }); it('does not add logs that are from LogBox itself"', () => { - jest.mock('../Data/LogBoxData'); - (LogBoxData.isLogBoxErrorMessage: any).mockReturnValue(true); + jest.spyOn(LogBoxData, 'isLogBoxErrorMessage').mockReturnValue(true); + jest.spyOn(LogBoxData, 'addLog'); LogBox.install(); @@ -349,7 +373,7 @@ describe('LogBox', () => { }); it('ignores logs starting with "(ADVICE)"', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); LogBox.install(); @@ -358,7 +382,7 @@ describe('LogBox', () => { }); it('does not ignore logs formatted to start with "(ADVICE)"', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); LogBox.install(); @@ -376,7 +400,7 @@ describe('LogBox', () => { }); it('ignores console methods after uninstalling', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); LogBox.install(); LogBox.uninstall(); @@ -389,7 +413,7 @@ describe('LogBox', () => { }); it('does not add logs after uninstalling', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addLog'); LogBox.install(); LogBox.uninstall(); @@ -406,7 +430,7 @@ describe('LogBox', () => { }); it('does not add exceptions after uninstalling', () => { - jest.mock('../Data/LogBoxData'); + jest.spyOn(LogBoxData, 'addException'); LogBox.install(); LogBox.uninstall(); @@ -482,4 +506,41 @@ describe('LogBox', () => { 'Custom: after installing for the second time', ); }); + it('registers errors without component stack as errors by default, when ExceptionManager is registered first', () => { + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'addException'); + + ExceptionsManager.installConsoleErrorReporter(); + LogBox.install(); + + console.error('HIT'); + + // Errors without a component stack skip the warning filter and + // fall through to the ExceptionManager, which are then reported + // back to LogBox as non-fatal exceptions, in a convuluted dance + // in the most legacy cruft way. + expect(LogBoxData.addException).toBeCalledWith( + expect.objectContaining({originalMessage: 'HIT'}), + ); + expect(LogBoxData.checkWarningFilter).not.toBeCalled(); + }); + + it('registers errors without component stack as errors by default, when ExceptionManager is registered second', () => { + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'addException'); + + LogBox.install(); + ExceptionsManager.installConsoleErrorReporter(); + + console.error('HIT'); + + // Errors without a component stack skip the warning filter and + // fall through to the ExceptionManager, which are then reported + // back to LogBox as non-fatal exceptions, in a convuluted dance + // in the most legacy cruft way. + expect(LogBoxData.addException).toBeCalledWith( + expect.objectContaining({originalMessage: 'HIT'}), + ); + expect(LogBoxData.checkWarningFilter).not.toBeCalled(); + }); }); From a5d7c455f4e2e8eee878664c3fc754d4e3e7a025 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 30 Sep 2024 09:15:04 -0700 Subject: [PATCH 4/5] 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: https://github.com/facebook/react-native/pull/46637 Reviewed By: huntie Differential Revision: D63349613 Pulled By: rickhanlonii fbshipit-source-id: 32e3fa4e2f2077114a6e9f4feac73673973ab50c --- .../Libraries/LogBox/Data/LogBoxData.js | 4 +- .../__tests__/LogBox-integration-test.js | 6 +-- .../Libraries/LogBox/__tests__/LogBox-test.js | 39 +++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/react-native/Libraries/LogBox/Data/LogBoxData.js b/packages/react-native/Libraries/LogBox/Data/LogBoxData.js index 367a78dd70db..08c631c8fec2 100644 --- a/packages/react-native/Libraries/LogBox/Data/LogBoxData.js +++ b/packages/react-native/Libraries/LogBox/Data/LogBoxData.js @@ -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, }; diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js index b6583f74a4a1..1e64e5e11efd 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js @@ -87,7 +87,7 @@ describe('LogBox', () => { expect.stringMatching('at DoesNotUseKey'), ]); expect(spy).toHaveBeenCalledWith({ - level: 'warn', + level: 'error', category: expect.stringContaining( 'Warning: Each child in a list should have a unique', ), @@ -140,7 +140,7 @@ describe('LogBox', () => { expect.stringMatching('at FragmentWithProp'), ]); expect(spy).toHaveBeenCalledWith({ - level: 'warn', + level: 'error', category: expect.stringContaining('Warning: Invalid prop'), componentStack: expect.anything(), componentStackType: expect.stringMatching(/(stack|legacy)/), @@ -236,7 +236,7 @@ describe('LogBox', () => { ), ]); expect(spy).toHaveBeenCalledWith({ - level: 'warn', + level: 'error', category: expect.stringContaining('Warning: Manual console error'), componentStack: expect.anything(), componentStackType: 'stack', diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js index b44c6131ffb3..a8a40825354a 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js @@ -506,6 +506,45 @@ describe('LogBox', () => { 'Custom: after installing for the second time', ); }); + + it('registers errors with component stack as errors by default, when ExceptionManager is registered first', () => { + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'addLog'); + + ExceptionsManager.installConsoleErrorReporter(); + LogBox.install(); + + console.error( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'error'}), + ); + expect(LogBoxData.checkWarningFilter).toBeCalledWith( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + }); + + it('registers errors with component stack as errors by default, when ExceptionManager is registered second', () => { + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'addLog'); + + LogBox.install(); + ExceptionsManager.installConsoleErrorReporter(); + + console.error( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'error'}), + ); + expect(LogBoxData.checkWarningFilter).toBeCalledWith( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + }); + it('registers errors without component stack as errors by default, when ExceptionManager is registered first', () => { jest.spyOn(LogBoxData, 'checkWarningFilter'); jest.spyOn(LogBoxData, 'addException'); From 919531875f8fa447baa5cf216fc3f5e54fd65778 Mon Sep 17 00:00:00 2001 From: Blake Friedman Date: Wed, 30 Oct 2024 12:01:49 +0000 Subject: [PATCH 5/5] [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. --- .../Libraries/LogBox/__tests__/LogBox-integration-test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js index 1e64e5e11efd..d0c836c4774d 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js @@ -81,7 +81,7 @@ describe('LogBox', () => { expect(mockWarn).not.toBeCalled(); expect(console.error).toBeCalledTimes(1); expect(console.error.mock.calls[0]).toEqual([ - 'Each child in a list should have a unique "key" prop.%s%s See https://react.dev/link/warning-keys for more information.%s', + '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'), @@ -95,7 +95,7 @@ describe('LogBox', () => { 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://react.dev/link/warning-keys for more information.', + '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}, @@ -107,7 +107,7 @@ describe('LogBox', () => { // 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://react.dev/link/warning-keys for more information.\n at ', + '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 ', ), ]); }); @@ -135,7 +135,7 @@ describe('LogBox', () => { expect(mockWarn).not.toBeCalled(); expect(console.error).toBeCalledTimes(1); expect(console.error.mock.calls[0]).toEqual([ - 'Invalid prop `%s` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.%s', + 'Warning: Invalid prop `%s` supplied to `React.Fragment`. React.Fragment can only have `key` and `children` props.%s', 'invalid', expect.stringMatching('at FragmentWithProp'), ]);