Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
517e197
Report DOM events to reportError directly instead of rethrowing
sebmarkbage Mar 12, 2024
bec21b2
Never rethrow at the root
sebmarkbage Mar 12, 2024
742aba4
Log fatal errors as they happen
sebmarkbage Mar 22, 2024
54471d4
Report root errors to the browser so they show up as "uncaught"
sebmarkbage Mar 12, 2024
bc5374d
Polyfill dispatching error event
sebmarkbage Mar 13, 2024
c6dd65e
Remove rethrowing in the commit phase
sebmarkbage Mar 13, 2024
0d7615d
Rethrow global errors that happened during an internal act
sebmarkbage Mar 22, 2024
ef549d4
Rethrow uncaught errors from act instead of logging them
sebmarkbage Mar 22, 2024
9f8a43a
Aggregate errors in internal act
sebmarkbage Mar 22, 2024
8fe758b
Aggregate errors in act
sebmarkbage Mar 22, 2024
254af8d
Use shared queue and only track errors once for internalAct/waitFor
sebmarkbage Mar 23, 2024
b4de7d2
Test error logging recovery without act
sebmarkbage Mar 24, 2024
06e4464
Fix tests that failed due to internalAct now rethrowing non-render er…
sebmarkbage Mar 22, 2024
785c32a
Fix tests
sebmarkbage Mar 22, 2024
e32089f
Fix tests that rely on flushSync to throw
sebmarkbage Mar 22, 2024
175484e
Use internal act for prod testing
sebmarkbage Mar 25, 2024
7344587
Build lint process for the reportGlobalError polyfill
sebmarkbage Mar 25, 2024
613ae34
Fix test
sebmarkbage Mar 27, 2024
d3f0b57
Fix legacy tests
rickhanlonii Mar 26, 2024
c06e47d
Fix legacy tests in ReactDOM-test.js
rickhanlonii Mar 26, 2024
45fb81e
Add back React.Children.only
rickhanlonii Mar 26, 2024
8e3c0ae
Fix useSyncExternalStoreShared-test.js
rickhanlonii Mar 26, 2024
7a07e98
Fix ReactFresh-test.js
rickhanlonii Mar 26, 2024
0928d91
Update error messages
sebmarkbage Mar 27, 2024
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
Prev Previous commit
Next Next commit
Fix tests
  • Loading branch information
sebmarkbage committed Mar 27, 2024
commit 785c32a693fc85b610ce8ffde0f231c0b99d8484
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ let act;
const util = require('util');
const realConsoleError = console.error;

function errorHandler() {
// forward to console.error but don't fail the tests
}

describe('ReactDOMServerHydration', () => {
let container;

Expand All @@ -27,12 +31,14 @@ describe('ReactDOMServerHydration', () => {
ReactDOMServer = require('react-dom/server');
act = React.act;

window.addEventListener('error', errorHandler);
console.error = jest.fn();
container = document.createElement('div');
document.body.appendChild(container);
});

afterEach(() => {
window.removeEventListener('error', errorHandler);
document.body.removeChild(container);
console.error = realConsoleError;
});
Expand Down
8 changes: 5 additions & 3 deletions packages/react-dom/src/__tests__/ReactDOMRoot-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,11 @@ describe('ReactDOMRoot', () => {
});
container.innerHTML = '';

expect(() => {
root.unmount();
}).toThrow('The node to be removed is not a child of this node.');
await expect(async () => {
await act(() => {
root.unmount();
});
}).rejects.toThrow('The node to be removed is not a child of this node.');
});

it('opts-in to concurrent default updates', async () => {
Expand Down
8 changes: 7 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMSelect-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1448,7 +1448,13 @@ describe('ReactDOMSelect', () => {
</select>,
);
}),
).rejects.toThrowError(new TypeError('prod message'));
).rejects.toThrowError(
// eslint-disable-next-line no-undef
new AggregateError([
new TypeError('prod message'),
new TypeError('prod message'),
]),
);
}).toErrorDev([
'The provided `value` attribute is an unsupported type TemporalLike.' +
' This value must be coerced to a string before using it here.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ describe('ReactErrorBoundaries', () => {
root.render(<BrokenComponentWillUnmount />);
});
await expect(async () => {
root.unmount();
await act(() => root.unmount());
}).rejects.toThrow('Hello');
});

Expand Down Expand Up @@ -2470,7 +2470,7 @@ describe('ReactErrorBoundaries', () => {
]);
});

it('passes first error when two errors happen in commit', async () => {
it('passes an aggregate error when two errors happen in commit', async () => {
const errors = [];
let caughtError;
class Parent extends React.Component {
Expand Down Expand Up @@ -2501,15 +2501,14 @@ describe('ReactErrorBoundaries', () => {
root.render(<Parent />);
});
} catch (e) {
if (e.message !== 'parent sad' && e.message !== 'child sad') {
throw e;
}
caughtError = e;
}

expect(errors).toEqual(['child sad', 'parent sad']);
// Error should be the first thrown
expect(caughtError.message).toBe('child sad');
expect(caughtError.errors).toEqual([
expect.objectContaining({message: 'child sad'}),
expect.objectContaining({message: 'parent sad'}),
]);
});

it('propagates uncaught error inside unbatched initial mount', async () => {
Expand Down Expand Up @@ -2561,15 +2560,14 @@ describe('ReactErrorBoundaries', () => {
root.render(<Parent value={2} />);
});
} catch (e) {
if (e.message !== 'parent sad' && e.message !== 'child sad') {
throw e;
}
caughtError = e;
}

expect(errors).toEqual(['child sad', 'parent sad']);
// Error should be the first thrown
expect(caughtError.message).toBe('child sad');
expect(caughtError.errors).toEqual([
expect.objectContaining({message: 'child sad'}),
expect.objectContaining({message: 'parent sad'}),
]);
});

it('should warn if an error boundary with only componentDidCatch does not update state', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
let PropTypes;
let RCTEventEmitter;
let React;
let act;
let ReactNative;
let ResponderEventPlugin;
let UIManager;
Expand Down Expand Up @@ -67,6 +68,7 @@ beforeEach(() => {
RCTEventEmitter =
require('react-native/Libraries/ReactPrivate/ReactNativePrivateInterface').RCTEventEmitter;
React = require('react');
act = React.act;
ReactNative = require('react-native-renderer');
ResponderEventPlugin =
require('react-native-renderer/src/legacy-events/ResponderEventPlugin').default;
Expand All @@ -77,7 +79,7 @@ beforeEach(() => {
.ReactNativeViewConfigRegistry.register;
});

it('fails to register the same event name with different types', () => {
it('fails to register the same event name with different types', async () => {
const InvalidEvents = createReactNativeComponentClass('InvalidEvents', () => {
if (!__DEV__) {
// Simulate a registration error in prod.
Expand Down Expand Up @@ -109,15 +111,15 @@ it('fails to register the same event name with different types', () => {

// The first time this renders,
// we attempt to register the view config and fail.
expect(() => ReactNative.render(<InvalidEvents />, 1)).toThrow(
'Event cannot be both direct and bubbling: topChange',
);
await expect(
async () => await act(() => ReactNative.render(<InvalidEvents />, 1)),
).rejects.toThrow('Event cannot be both direct and bubbling: topChange');

// Continue to re-register the config and
// fail so that we don't mask the above failure.
expect(() => ReactNative.render(<InvalidEvents />, 1)).toThrow(
'Event cannot be both direct and bubbling: topChange',
);
await expect(
async () => await act(() => ReactNative.render(<InvalidEvents />, 1)),
).rejects.toThrow('Event cannot be both direct and bubbling: topChange');
});

it('fails if unknown/unsupported event types are dispatched', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ let createReactNativeComponentClass;
let UIManager;
let TextInputState;
let ReactNativePrivateInterface;
let act;

const DISPATCH_COMMAND_REQUIRES_HOST_COMPONENT =
"Warning: dispatchCommand was called with a ref that isn't a " +
Expand All @@ -31,6 +32,7 @@ describe('ReactNative', () => {
jest.resetModules();

React = require('react');
act = React.act;
StrictMode = React.StrictMode;
ReactNative = require('react-native-renderer');
ReactNativePrivateInterface = require('react-native/Libraries/ReactPrivate/ReactNativePrivateInterface');
Expand Down Expand Up @@ -476,7 +478,7 @@ describe('ReactNative', () => {
);
});

it('should throw for text not inside of a <Text> ancestor', () => {
it('should throw for text not inside of a <Text> ancestor', async () => {
const ScrollView = createReactNativeComponentClass('RCTScrollView', () => ({
validAttributes: {},
uiViewClassName: 'RCTScrollView',
Expand All @@ -490,18 +492,24 @@ describe('ReactNative', () => {
uiViewClassName: 'RCTView',
}));

expect(() => ReactNative.render(<View>this should warn</View>, 11)).toThrow(
await expect(async () => {
await act(() => ReactNative.render(<View>this should warn</View>, 11));
}).rejects.toThrow(
'Text strings must be rendered within a <Text> component.',
);

expect(() =>
ReactNative.render(
<Text>
<ScrollView>hi hello hi</ScrollView>
</Text>,
11,
),
).toThrow('Text strings must be rendered within a <Text> component.');
await expect(async () => {
await act(() =>
ReactNative.render(
<Text>
<ScrollView>hi hello hi</ScrollView>
</Text>,
11,
),
);
}).rejects.toThrow(
'Text strings must be rendered within a <Text> component.',
);
});

it('should not throw for text inside of an indirect <Text> ancestor', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,6 @@ describe('ReactFlushSync (AggregateError not available)', () => {
// AggregateError is not available, React throws the first error, then
// throws the remaining errors in separate tasks.
expect(error).toBe(aahh);
expect(flushFakeMicrotasks).toThrow(nooo);
await flushFakeMicrotasks();
Copy link
Member

Choose a reason for hiding this comment

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

Should we assert somewhere that this was thrown to reportError or whatever?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it's a bit weird about what we should be testing here. Because this is to test that something works even if aggregated errors is not supported by the platform. However, we only do aggregation at all in act() now. So we're testing act() and if you use act in an environment that doesn't support aggregated errors, then this does get dropped. Not logged somewhere else.

Which I guess is mostly if you use act in a non-JSDOM environment.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, we should probably update the comment on 143 then

});
});
Original file line number Diff line number Diff line change
Expand Up @@ -1899,7 +1899,7 @@ describe('ReactHooks', () => {
}).rejects.toThrow('Hello');

if (__DEV__) {
expect(console.error).toHaveBeenCalledTimes(2);
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error.mock.calls[0][0]).toContain(
'Warning: Cannot update a component (`%s`) while rendering ' +
'a different component (`%s`).',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2077,14 +2077,15 @@ describe('ReactHooksWithNoopRenderer', () => {
});
return <Text text={'Count: ' + props.count} />;
}
await act(async () => {
ReactNoop.render(<Counter count={0} />, () =>
Scheduler.log('Sync effect'),
);
await waitFor(['Count: 0', 'Sync effect']);
expect(ReactNoop).toMatchRenderedOutput(<span prop="Count: 0" />);
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
});
await expect(async () => {
await act(async () => {
ReactNoop.render(<Counter count={0} />, () =>
Scheduler.log('Sync effect'),
);
await waitFor(['Count: 0', 'Sync effect']);
expect(ReactNoop).toMatchRenderedOutput(<span prop="Count: 0" />);
});
}).rejects.toThrow('Oops');

assertLog([
'Mount A [0]',
Expand All @@ -2107,7 +2108,7 @@ describe('ReactHooksWithNoopRenderer', () => {
useEffect(() => {
if (props.count === 1) {
Scheduler.log('Oops!');
throw new Error('Oops!');
throw new Error('Oops error!');
}
Scheduler.log(`Mount B [${props.count}]`);
return () => {
Expand All @@ -2126,22 +2127,27 @@ describe('ReactHooksWithNoopRenderer', () => {
assertLog(['Mount A [0]', 'Mount B [0]']);
});

await act(async () => {
// This update will trigger an error
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.log('Sync effect'),
);
await waitFor(['Count: 1', 'Sync effect']);
expect(ReactNoop).toMatchRenderedOutput(<span prop="Count: 1" />);
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
assertLog(['Unmount A [0]', 'Unmount B [0]', 'Mount A [1]', 'Oops!']);
expect(ReactNoop).toMatchRenderedOutput(null);
});
assertLog([
// Clean up effect A runs passively on unmount.
// There's no effect B to clean-up, because it never mounted.
'Unmount A [1]',
]);
await expect(async () => {
await act(async () => {
// This update will trigger an error
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.log('Sync effect'),
);
await waitFor(['Count: 1', 'Sync effect']);
expect(ReactNoop).toMatchRenderedOutput(<span prop="Count: 1" />);
ReactNoop.flushPassiveEffects();
assertLog([
'Unmount A [0]',
'Unmount B [0]',
'Mount A [1]',
'Oops!',
// Clean up effect A runs passively on unmount.
// There's no effect B to clean-up, because it never mounted.
'Unmount A [1]',
]);
expect(ReactNoop).toMatchRenderedOutput(null);
});
}).rejects.toThrow('Oops error!');
});

it('handles errors in destroy on update', async () => {
Expand All @@ -2151,7 +2157,7 @@ describe('ReactHooksWithNoopRenderer', () => {
return () => {
Scheduler.log('Oops!');
if (props.count === 0) {
throw new Error('Oops!');
throw new Error('Oops error!');
}
};
});
Expand All @@ -2174,26 +2180,34 @@ describe('ReactHooksWithNoopRenderer', () => {
assertLog(['Mount A [0]', 'Mount B [0]']);
});

await act(async () => {
// This update will trigger an error during passive effect unmount
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.log('Sync effect'),
);
await waitFor(['Count: 1', 'Sync effect']);
expect(ReactNoop).toMatchRenderedOutput(<span prop="Count: 1" />);
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
await expect(async () => {
await act(async () => {
// This update will trigger an error during passive effect unmount
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.log('Sync effect'),
);
await waitFor(['Count: 1', 'Sync effect']);
expect(ReactNoop).toMatchRenderedOutput(<span prop="Count: 1" />);
ReactNoop.flushPassiveEffects();

// This branch enables a feature flag that flushes all passive destroys in a
// separate pass before flushing any passive creates.
// A result of this two-pass flush is that an error thrown from unmount does
// not block the subsequent create functions from being run.
assertLog(['Oops!', 'Unmount B [0]', 'Mount A [1]', 'Mount B [1]']);
});
// This branch enables a feature flag that flushes all passive destroys in a
// separate pass before flushing any passive creates.
// A result of this two-pass flush is that an error thrown from unmount does
// not block the subsequent create functions from being run.
assertLog([
'Oops!',
'Unmount B [0]',
'Mount A [1]',
'Mount B [1]',
// <Counter> gets unmounted because an error is thrown above.
// The remaining destroy functions are run later on unmount, since they're passive.
// In this case, one of them throws again (because of how the test is written).
'Oops!',
'Unmount B [1]',
]);
});
}).rejects.toThrow('Oops error!');

// <Counter> gets unmounted because an error is thrown above.
// The remaining destroy functions are run later on unmount, since they're passive.
// In this case, one of them throws again (because of how the test is written).
assertLog(['Oops!', 'Unmount B [1]']);
expect(ReactNoop).toMatchRenderedOutput(null);
});

Expand Down Expand Up @@ -3805,7 +3819,7 @@ describe('ReactHooksWithNoopRenderer', () => {
await waitForThrow(
'Rendered more hooks than during the previous render.',
);
assertLog([]);
assertLog(['Unmount A']);
}).toErrorDev([
'Warning: React has detected a change in the order of Hooks called by App. ' +
'This will lead to bugs and errors if not fixed. For more information, ' +
Expand Down
Loading