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

tests: migrate fireEvents to userEvent where applicable #63907

Merged
merged 2 commits into from
May 25, 2022

Conversation

flootr
Copy link
Contributor

@flootr flootr commented May 23, 2022

Changes proposed in this Pull Request

  • Migrate fireEvent usages to @testing-library/user-event where applicable

Testing instructions

  • Verify unit tests still pass

Related to #63409

@github-actions
Copy link

github-actions bot commented May 23, 2022

@flootr flootr force-pushed the add/testing-library-user-event branch from fd13abf to d75f940 Compare May 23, 2022 12:19
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@flootr flootr force-pushed the add/testing-library-user-event branch from d75f940 to 134cc40 Compare May 24, 2022 08:28
Base automatically changed from add/testing-library-user-event to trunk May 24, 2022 08:49
@flootr flootr force-pushed the update/fire-user-event-rtl branch from 50e64bf to 97cfcdf Compare May 24, 2022 09:01
@flootr flootr marked this pull request as ready for review May 24, 2022 09:15
@flootr flootr requested review from a team as code owners May 24, 2022 09:15
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 24, 2022
@flootr flootr requested review from noahtallen and a team May 24, 2022 09:15
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Beautiful, thank you ❤️

@@ -170,7 +171,7 @@ describe( '<EligibilityWarnings>', () => {
);
} );

it( `disables the "Continue" button if holds can't be handled automatically`, () => {
it( `disables the "Continue" button if holds can't be handled automatically`, async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this one to be async if we don't call await below?

This also raises a question: do all of those here need to be with async / await? I know it's what the library recommends, but the fact that this one passes without it makes me curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the API userEvent provides is async (methods all return Promises) I think we should await them as the docs say.

In this case, the test passes because we test that handleProceed doesn't get called in case of a click. As we don't await the click, there may be no click happening at all which also makes the test pass. I'll add a fix and add the missing await.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a really great catch by the way 👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

Adding an await makes sense 👍 Thanks for the confirmation.

Copy link
Member

Choose a reason for hiding this comment

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

Does userEvent.click really return a promise? Looking at the @testing-library/user-event internals, I see all functions return void.

When performing a DOM click, there should be nothing async inside. Internally wrapped with act(), all React updates happen synchronously.

Copy link
Member

@tyxla tyxla May 25, 2022

Choose a reason for hiding this comment

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

I might be missing something, but I can clearly see the click convenience function returns a Promise<void>:

https://github.com/testing-library/user-event/tree/e2a5f434b6e8b0ec0badddec52b54957b6f3320a/src/convenience/click.ts#L4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Release notes for v14 mention that all APIs return Promises https://github.com/testing-library/user-event/releases/tag/v14.0.0 – relevant PR is testing-library/user-event#790 (which closes testing-library/user-event#504), those might contain more in-depth information about that change.

Copy link
Member

Choose a reason for hiding this comment

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

We are both right: we're now using version 13.5.0 which returns void, and there's version 14.0.x that started returning promises 🙂 The reason for being async is that the library supports firing sequences of events, like when typing a text, with delays between them.

@sirbrillig
Copy link
Member

TIL about userEvent!

@flootr flootr force-pushed the update/fire-user-event-rtl branch from 97cfcdf to f4bfd81 Compare May 25, 2022 09:45
@flootr flootr merged commit ba53a75 into trunk May 25, 2022
@flootr flootr deleted the update/fire-user-event-rtl branch May 25, 2022 10:47
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants