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.

Sign up for GitHub

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages-internal/test-utils/src/createRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ function render(
);
const result: MuiRenderResult = {
...testingLibraryRenderResult,
user: userEvent.setup(),
user: userEvent.setup({ delay: null }),
Copy link
Member

@oliviertassinari oliviertassinari Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Going off topic, but would this works instead?

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

Copy link
Member Author

@Janpot Janpot Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@oliviertassinari oliviertassinari Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@oliviertassinari oliviertassinari Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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.

Copy link
Member

@oliviertassinari oliviertassinari Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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. 🙈 🤷

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@oliviertassinari oliviertassinari Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

forceUpdate() {
traceSync('forceUpdate', () =>
testingLibraryRenderResult.rerender(
Expand Down
Loading