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

[test] Enable "missing act()"-warnings #21802

Merged
merged 8 commits into from
Jul 15, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 15, 2020

Enables missing act warnings to enforce wrapping updates in act.

This will later be required for concurrent react and brings the test closer to how a jest-user would test.

I was wondering for the longest time why we didn't need act() all the time and it turns out these warnings are simply disabled outside of jest.

There are some cases why we shouldn't need act in my opinion (e.g. facebook/react#19318) which we can later revisit.

@eps1lon eps1lon added the test label Jul 15, 2020
@eps1lon eps1lon force-pushed the test/missing-act-warnings branch from 91818c4 to 6b12c72 Compare July 15, 2020 11:09
@mui-pr-bot
Copy link

Details of bundle changes

Generated by 🚫 dangerJS against 6b12c72

@eps1lon eps1lon marked this pull request as ready for review July 15, 2020 12:41
Comment on lines +41 to +44
act(() => {
input.focus();
fireEvent.change(document.activeElement, { target: { value: 'a' } });
});
Copy link
Member

Choose a reason for hiding this comment

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

I thought testing library already included act when firing events

Suggested change
act(() => {
input.focus();
fireEvent.change(document.activeElement, { target: { value: 'a' } });
});
act(() => {
input.focus();
});
fireEvent.change(document.activeElement, { target: { value: 'a' } });

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it does. That doesn't necessarily mean that we can't put these in a single act() because that's a difference in behavior:

act(one);
// two runs with updates flushed from one
act(two)

vs

act(() => {
  one();
  // two runs without all updates flushed from one (I think)
  two();
})

I'm not 100% certain these are different. Though I would expect that idle or offscreen updates scheduled in one are not necessarily flushed before two runs.

I'll try to wrap actions for an assertion in a single act() i.e. between each act() should be an assertion.

In the end wrapping it in a single act() is closer to the original behavior which is more important here.

Copy link
Member

@oliviertassinari oliviertassinari Jul 15, 2020

Choose a reason for hiding this comment

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

Interesting distinction, I will keep it in mind.

So, we should consider if two actions will happen during the same screen frame, in between 16ms, or not. If it's two consequent events, like mouseup/click, it should be wrapped in the same act, if it's two user actions, like key down + key up, it should be wrapped in two acts.

Correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure to be honest. It seems to me that as few acts as possible should be preferred because the "time" between two acts() can be very long

Copy link
Member

Choose a reason for hiding this comment

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

Ok, time will tell 🤷‍♂️

@eps1lon eps1lon merged commit f505b79 into mui:next Jul 15, 2020
@eps1lon eps1lon deleted the test/missing-act-warnings branch July 15, 2020 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants