[material-ui][Modal] Fix event handlers overriding behaviour#43757
[material-ui][Modal] Fix event handlers overriding behaviour#43757aarongarciah merged 5 commits intomui:masterfrom
Conversation
sai6855
added
type: bug
Netlify deploy previewhttps://deploy-preview-43757--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
| </Modal>, | ||
| ); | ||
|
|
||
| await user.keyboard('{Escape}'); |
There was a problem hiding this comment.
How long does it take to run this test with this utils vs. firing a keydown event directly?
There was a problem hiding this comment.
Not sure if this is correct way.
await user.keyboard('{Escape}') took around 30ms (+/- 5ms)
fireEvent.keyDown(getByTestId('modal'), {
key: 'Escape',
});
took around 4ms (+/- 1ms)
There was a problem hiding this comment.
Ok thanks, so about +20ms. If we scale this to our 10,000 tests: https://app.circleci.com/pipelines/github/mui/material-ui/139173/workflows/97427b1f-67e7-406f-aec4-1fdcd5fe84dc/jobs/750329.
It could mean +200s, from 3 minutes to 6 minutes: https://app.circleci.com/pipelines/github/mui/material-ui/139173/workflows/97427b1f-67e7-406f-aec4-1fdcd5fe84dc/jobs/750329. So I think to be really careful with this.
cc @Janpot. We might want to limit https://github.com/testing-library/user-event to only cases where we absolute need it.
There was a problem hiding this comment.
I guess we should include clear guidelines on when to use one or the other in test/README.md. From discussions in past PRs it seemed that we were leaning towards using user more in new tests.
There was a problem hiding this comment.
We can potentially drastically speed up userEvent by setting it up with delay: null.
userEvent.setup({ delay: null })See https://testing-library.com/docs/user-event/options/#delay
It looks like the semantics will change somewhat though. i.e. it will yield to a macrotask instead of a microtask in between events. I did some preliminary testing and it seems to make up the bulk of the increase in time. The question will be whether this is going to be a problem for us or not.
I'd argue that the current tests don't care at all about the event loop when chaining events, so we're probably good here.
In the meantime I'm also opening testing-library/user-event#1233
It could mean +200s, from 3 minutes to 6 minutes
The biggest part of the runtime increase seems to be non-blocking. With parallelized tests the impact should be much smaller than what you describe here.
5 participants
closes #43738
regression was introduced in this PR #42150 (by me 😑)