[code-infra] Optimize @testing-library/user-event

[Janpot]

See this thread

I've seen this reduce the time of running userEvent.keyboard by an order of magnitude, eliminating the motivation to avoid using this library. It comes at the cost of slightly changed semantics. But even with these different semantics, we'd be better off compared to the way we currently test.

It would be best for us to primarily optimize our tests for refactorability. Tests that require updates on every implementation detail change miss the mark. We should aim at tests that maximize interfacing with public API and minimize reliance on implementation details. Therefore, unless we specifically want to test an implementation detail, we should prefer userEvent.

Closes mui/mui-x#14668

[mui-bot]

Netlify deploy preview

https://deploy-preview-43804--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 9aaeecb

[oliviertassinari]

Going off topic, but would this works instead?

Suggested change

	user: userEvent.setup({ delay: null }),
	export const userEventInstance = userEvent.setup({ delay: null });

[Janpot]

I think you want to initialize it per test. At least that's seems to be recommended on their docs. I understand the instance hold state and you can avoid interference between parallel tests this way.

We discourage rendering or using any userEvent functions outside of the test itself - e.g. in a before/after hook

[oliviertassinari]

This is what I'm not clear about. In the source, I didn't see logic that required it to be initialized once for all the tests, this would save CI time for each test and make the DX simpler as well.

[aarongarciah]

Not sure I follow @oliviertassinari. I think we should just follow the Testing Lib recommendation here i.e. setup userEvent once per test.

[oliviertassinari]

I have found testing-library/user-event#1036 (comment) about this. It could be ok

You should use userEvent.setup() after creating the document and before you render() your component. [...] if you hit one of the rare edge cases, it might be hard to debug - especially since most environments don't create a new document for each test.

https://github.com/testing-library/user-event/blob/d0362796a33c2d39713998f82ae309020c37b385/src/setup/setup.ts#L82.

• prepareDocument bails out if it's already called. https://github.com/testing-library/user-event/blob/d0362796a33c2d39713998f82ae309020c37b385/src/document/prepareDocument.ts#L19
• attachClipboardStubToView bails out if it's already called. https://github.com/testing-library/user-event/blob/d0362796a33c2d39713998f82ae309020c37b385/src/utils/dataTransfer/Clipboard.ts#L113

So I don't really see why it's needed in our case since we share the same document between tests.

But no hard point of view. Just that if we want to be greedy, there is maybe an opportunity, but maybe not.

[oliviertassinari]

With Vitest, the need to have a document always setup for each test could become an issue, depending on how parallelism is implemented. I imagine that it's OK, but it might not.

[oliviertassinari]

Can this break MUI X tests? Do we need to release this in a way that allows some tests to configure this behavior?

[sai6855]

To add more context in terms of perfomance improvements from this thread, previously await user.keyboard('{Escape}') took 20ms (+/- 5ms). Now with this change, it's taking 10ms (+/- 4ms)

[Janpot]

Can this break MUI X tests?

Technically yes, but unlikely. We could try it out before merging.

Do we need to release this in a way that allows some tests to configure this behavior?

We could add a parameter to createRenderer, but you can also just set up your own const user = userEvent.setup() instance and use that for a specific test. IMO over time we should work towards moving things out of createRenderer and make the wrapper unnecessary.

[aarongarciah]

Not a single test failed, which is nice. Looks good but as Olivier said maybe we want to test this in MUI X before merging.

[Janpot]

🙂 Yep, trying this in X breaks the tests, but looks like it's not caused by this change, see mui/mui-x#14671

Edit: Looks like it now fails consistently

[mnajdova]

Let's hold off merging until we figure out what are the issues in X.

[LukasTy]

I've looked into failing tests on mui/mui-x#14668 and I'm not sure we should move forward with this. 🤔
Is it really such a big problem to keep with the existing setTimeout wrapping for each userEvent call? 🤔
I wouldn't like to significantly degrade the DX (I wasn't able to fix all the failing tests without resetting delay to 0 on some of them) of writing tests for a negligible performance difference. 🙈 🤷

[Janpot]

I think we could let this PR rest until we've

• made all act async
• refactor fake timers to scope to single tests only
• migrated to vitest

and then re-evaluate how broken it is.

[LukasTy]

It makes sense, let's go with it. 👍

[oliviertassinari]

Is it really such a big problem to keep with the existing setTimeout wrapping for each userEvent call?

@LukasTy I think that it boils down to this: #43757 (comment). If moving from fireEvent to userEvent x2 the CI run time (I don't know if true), then IMHO, we shouldn't. Feedback time on PR is already painfully slow, it feels more important.

I wasn't able to fix all the failing tests without resetting delay to 0 on some of them

Could you pinpoint to why? I mean Is there a technical reason why those setTimeout should be needed for tests to pass?

If the why this is needed is "race conditions", it sounds like the right move is to have { delay: null } in as many places as possible in the codebase as soon as possible to avoid regressions.

then re-evaluate how broken it is

This can work as well.

[LukasTy]

Could you pinpoint to why? I mean Is there a technical reason why those setTimeout should be needed for tests to pass?

The main problem is the TransitionGroup on Pickers tests.
There are errors thrown about mutating state, because of it.