Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve tests that use discrete events #20667

Merged
merged 1 commit into from
Jan 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 11 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMHooks-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
let React;
let ReactDOM;
let Scheduler;
let act;

describe('ReactDOMHooks', () => {
let container;
Expand All @@ -22,6 +23,7 @@ describe('ReactDOMHooks', () => {
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');
act = require('react-dom/test-utils').unstable_concurrentAct;

container = document.createElement('div');
document.body.appendChild(container);
Expand Down Expand Up @@ -106,7 +108,7 @@ describe('ReactDOMHooks', () => {
});

// @gate experimental
it('should not bail out when an update is scheduled from within an event handler in Concurrent Mode', () => {
it('should not bail out when an update is scheduled from within an event handler in Concurrent Mode', async () => {
const {createRef, useCallback, useState} = React;

const Example = ({inputRef, labelRef}) => {
Expand All @@ -132,11 +134,14 @@ describe('ReactDOMHooks', () => {
Scheduler.unstable_flushAll();

inputRef.current.value = 'abc';
inputRef.current.dispatchEvent(
new Event('input', {bubbles: true, cancelable: true}),
);

Scheduler.unstable_flushAll();
await act(async () => {
inputRef.current.dispatchEvent(
new Event('input', {
bubbles: true,
cancelable: true,
}),
);
});

expect(labelRef.current.innerHTML).toBe('abc');
});
Expand Down
8 changes: 4 additions & 4 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ function runActTests(label, render, unmount, rerender) {
expect(Scheduler).toHaveYielded([100]);
});

it('flushes effects on every call', () => {
it('flushes effects on every call', async () => {
function App() {
const [ctr, setCtr] = React.useState(0);
React.useEffect(() => {
Expand All @@ -209,16 +209,16 @@ function runActTests(label, render, unmount, rerender) {
button.dispatchEvent(new MouseEvent('click', {bubbles: true}));
}

act(() => {
await act(async () => {
click();
click();
click();
});
// it consolidates the 3 updates, then fires the effect
expect(Scheduler).toHaveYielded([3]);
act(click);
await act(async () => click());
expect(Scheduler).toHaveYielded([4]);
act(click);
await act(async () => click());
expect(Scheduler).toHaveYielded([5]);
expect(button.innerHTML).toBe('5');
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ describe('ChangeEventPlugin', () => {
});

// @gate experimental
it('is async for non-input events', () => {
it('is async for non-input events', async () => {
const root = ReactDOM.unstable_createRoot(container);
let input;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe('SimpleEventPlugin', function() {
let React;
let ReactDOM;
let Scheduler;
let TestUtils;

let onClick;
let container;
Expand All @@ -39,6 +40,7 @@ describe('SimpleEventPlugin', function() {
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');
TestUtils = require('react-dom/test-utils');

onClick = jest.fn();
});
Expand Down Expand Up @@ -314,7 +316,7 @@ describe('SimpleEventPlugin', function() {
});

// @gate experimental
it('end result of many interactive updates is deterministic', () => {
it('end result of many interactive updates is deterministic', async () => {
container = document.createElement('div');
const root = ReactDOM.unstable_createRoot(container);
document.body.appendChild(container);
Expand Down Expand Up @@ -361,12 +363,14 @@ describe('SimpleEventPlugin', function() {
expect(button.textContent).toEqual('Count: 0');

// Click the button many more times
click();
click();
click();
click();
click();
click();
await TestUtils.act(async () => {
click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only one I'm not sure about. Actually I'm confused why it passes. The microtasks don't fire in between each click, so I would expect this.state.count + 1 to always equal 1. What am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Each click queues a microtask to synchronously flush the update, and none of those tasks are cancelled, so there are 7 updates when the microtask queue is flushed, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right. Could you update it to flush the microtasks in between? Since that's closer to what would actually happen in the browser.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the fact that there are seven separate microtasks doesn't seem right. Should it see that there's already a task at the correct priority and then bail out?

Copy link
Collaborator

@acdlite acdlite Jan 27, 2021

Choose a reason for hiding this comment

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

click();
click();
click();
click();
click();
});

// Flush the remaining work
Scheduler.unstable_flushAll();
Expand All @@ -375,7 +379,7 @@ describe('SimpleEventPlugin', function() {
});

// @gate experimental
it('flushes discrete updates in order', () => {
it('flushes discrete updates in order', async () => {
container = document.createElement('div');
document.body.appendChild(container);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ describe('ReactIncrementalErrorHandling', () => {
expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]);
});

it("retries at a lower priority if there's additional pending work", () => {
// @gate experimental
it("retries at a lower priority if there's additional pending work", async () => {
function App(props) {
if (props.isBroken) {
Scheduler.unstable_yieldValue('error');
Expand All @@ -252,14 +253,14 @@ describe('ReactIncrementalErrorHandling', () => {
});
}

ReactNoop.discreteUpdates(() => {
ReactNoop.render(<App isBroken={true} />, onCommit);
});
ReactNoop.render(<App isBroken={true} />, onCommit);
expect(Scheduler).toFlushAndYieldThrough(['error']);
interrupt();

// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
React.unstable_startTransition(() => {
// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
});

expect(Scheduler).toFlushAndYieldThrough([
// The first render fails. But because there's a lower priority pending
Expand Down Expand Up @@ -311,16 +312,16 @@ describe('ReactIncrementalErrorHandling', () => {
});
}

ReactNoop.discreteUpdates(() => {
ReactNoop.render(<App isBroken={true} />, onCommit);
});
ReactNoop.render(<App isBroken={true} />, onCommit);
expect(Scheduler).toFlushAndYieldThrough(['error']);
interrupt();

expect(ReactNoop).toMatchRenderedOutput(null);

// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
React.unstable_startTransition(() => {
// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
});

expect(Scheduler).toFlushAndYieldThrough([
// The first render fails. But because there's a lower priority pending
Expand Down Expand Up @@ -1780,6 +1781,7 @@ describe('ReactIncrementalErrorHandling', () => {
});
}

// @gate experimental
it('uncaught errors should be discarded if the render is aborted', async () => {
const root = ReactNoop.createRoot();

Expand All @@ -1789,22 +1791,24 @@ describe('ReactIncrementalErrorHandling', () => {
}

await ReactNoop.act(async () => {
ReactNoop.discreteUpdates(() => {
root.render(<Oops />);
});
root.render(<Oops />);

// Render past the component that throws, then yield.
expect(Scheduler).toFlushAndYieldThrough(['Oops']);
expect(root).toMatchRenderedOutput(null);
// Interleaved update. When the root completes, instead of throwing the
// error, it should try rendering again. This update will cause it to
// recover gracefully.
root.render('Everything is fine.');
React.unstable_startTransition(() => {
root.render('Everything is fine.');
});
});

// Should finish without throwing.
expect(root).toMatchRenderedOutput('Everything is fine.');
});

// @gate experimental
it('uncaught errors are discarded if the render is aborted, case 2', async () => {
const {useState} = React;
const root = ReactNoop.createRoot();
Expand All @@ -1829,21 +1833,20 @@ describe('ReactIncrementalErrorHandling', () => {
});

await ReactNoop.act(async () => {
// Schedule a high pri and a low pri update on the root.
ReactNoop.discreteUpdates(() => {
root.render(<Oops />);
// Schedule a default pri and a low pri update on the root.
root.render(<Oops />);
React.unstable_startTransition(() => {
root.render(<AllGood />);
});
root.render(<AllGood />);
// Render through just the high pri update. The low pri update remains on

// Render through just the default pri update. The low pri update remains on
// the queue.
expect(Scheduler).toFlushAndYieldThrough(['Everything is fine.']);

// Schedule a high pri update on a child that triggers an error.
// Schedule a default pri update on a child that triggers an error.
// The root should capture this error. But since there's still a pending
// update on the root, the error should be suppressed.
ReactNoop.discreteUpdates(() => {
setShouldThrow(true);
});
setShouldThrow(true);
});
// Should render the final state without throwing the error.
expect(Scheduler).toHaveYielded(['Everything is fine.']);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -570,14 +570,13 @@ describe('ReactSuspenseWithNoopRenderer', () => {
);
}

// Schedule a high pri update and a low pri update, without rendering in
// between.
ReactNoop.discreteUpdates(() => {
// High pri
ReactNoop.render(<App />);
});
// Schedule a default pri update and a low pri update, without rendering in between.
// Default pri
ReactNoop.render(<App />);
// Low pri
ReactNoop.render(<App hide={true} />);
React.unstable_startTransition(() => {
ReactNoop.render(<App hide={true} />);
});

expect(Scheduler).toFlushAndYield([
// The first update suspends
Expand Down Expand Up @@ -1879,9 +1878,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
ReactNoop.render(<Foo />);
expect(Scheduler).toFlushAndYield(['Foo']);

ReactNoop.discreteUpdates(() =>
ReactNoop.render(<Foo renderContent={true} />),
);
ReactNoop.render(<Foo renderContent={true} />);
expect(Scheduler).toFlushAndYieldThrough(['Foo']);

// Advance some time.
Expand Down Expand Up @@ -3080,48 +3077,48 @@ describe('ReactSuspenseWithNoopRenderer', () => {
// Schedule an update inside the Suspense boundary that suspends.
setAppText('B');
expect(Scheduler).toFlushAndYield(['Suspend! [B]', 'Loading...']);
});

// Commit the placeholder
await advanceTimers(250);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Loading..." />
</>,
);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Loading..." />
</>,
);

// Schedule a high pri update on the boundary, and a lower pri update
// on the fallback. We're testing to make sure the fallback can still
// update even though the primary tree is suspended.{
ReactNoop.discreteUpdates(() => {
setAppText('C');
// Schedule a default pri update on the boundary, and a lower pri update
// on the fallback. We're testing to make sure the fallback can still
// update even though the primary tree is suspended.
await ReactNoop.act(async () => {
setAppText('C');
React.unstable_startTransition(() => {
setFallbackText('Still loading...');
});
setFallbackText('Still loading...');
});

expect(Scheduler).toFlushAndYield([
// First try to render the high pri update. Still suspended.
'Suspend! [C]',
'Loading...',
expect(Scheduler).toHaveYielded([
// First try to render the high pri update. Still suspended.
'Suspend! [C]',
'Loading...',

// In the expiration times model, once the high pri update suspends,
// we can't be sure if there's additional work at a lower priority
// that might unblock the tree. We do know that there's a lower
// priority update *somehwere* in the entire root, though (the update
// to the fallback). So we try rendering one more time, just in case.
// TODO: We shouldn't need to do this with lanes, because we always
// know exactly which lanes have pending work in each tree.
'Suspend! [C]',

// Then complete the update to the fallback.
'Still loading...',
]);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Still loading..." />
</>,
);
});
// In the expiration times model, once the high pri update suspends,
// we can't be sure if there's additional work at a lower priority
// that might unblock the tree. We do know that there's a lower
// priority update *somehwere* in the entire root, though (the update
// to the fallback). So we try rendering one more time, just in case.
// TODO: We shouldn't need to do this with lanes, because we always
// know exactly which lanes have pending work in each tree.
'Suspend! [C]',

// Then complete the update to the fallback.
'Still loading...',
]);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Still loading..." />
</>,
);
},
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1397,12 +1397,13 @@ describe('useMutableSource', () => {
// Now mutate A. Both hooks should update.
// This is at high priority so that it doesn't get batched with default
// priority updates that might fire during the passive effect
ReactNoop.discreteUpdates(() => {
mutateA('a1');
await ReactNoop.act(async () => {
ReactNoop.discreteUpdates(() => {
mutateA('a1');
});
});
expect(Scheduler).toFlushUntilNextPaint([]);

expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a1');
expect(root).toMatchRenderedOutput('first: a1, second: a1');
});

expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a1');
Expand Down
Loading