feat(rule): add no-await-sync-events rule #240

J-Huang · 2020-10-22T08:17:22Z

Feature request:
#219

All functions in "fireEvent" and "userEvent" are synchronous, meaning it's wrong to add "await" operator for those function calls, with an exception of userEvent.type.

This new rule "no-await-sync-events" disallows adding "await" for those function calls in order to avoid errors.

Belco90 · 2020-10-22T17:50:26Z

Thanks! I'll try to review it during the weekend. Although I don't know if there is something pending since it's a draft PR.

J-Huang · 2020-10-22T17:53:17Z

@Belco90 it's in draft because would like to have @benmonro take a look first since he was the owner of the feature request. Thanks.

benmonro

LGTM thanks @J-Huang

Belco90

The rule implementation itself seems fine, but we need to improve few things:

handle userEvent.type special cases better
make tests more confident
add the rule to README

Belco90 · 2020-10-23T11:18:30Z

docs/rules/no-await-sync-events.md

+
+Functions in the event object provided by Testing Library, including
+fireEvent and userEvent, do NOT return Promise, with an exception of
+`userEvent.type`. Some examples are:


This is technically true, but you don't need to always wait for it. From user-event doc:

To be clear, userEvent.type always returns a promise, but you only need to await the promise it returns if you're using the delay option. Otherwise everything runs synchronously and you can ignore the promise.

So should this rule report about userEvent.type if no delay option passed? I think so

I agree with that ☝️ we should only await if the delay option is used. It would be great if we can cover that scenario with the rule

Agree. upgraded
the rule is, only if userEvent.type with delay option, await is valid. For all the other cases, it disallows await.

Belco90 · 2020-10-23T11:21:14Z

lib/utils.ts

@@ -63,6 +63,11 @@ const ASYNC_UTILS = [
  'waitForDomChange',
 ];

+const ASYNC_EVENTS = [


These are SYNC_EVENTS actually, right?

I wouldn't say there are events either - there are helpers or utilities.. Events would be "tab", "input", "change", "click" and so on... 🤔

initially thought it should be no-await-sync-utils, but feel not so correct...I can rename it to utils if it makes more sense.

Belco90 · 2020-10-23T11:25:20Z

tests/lib/rules/no-await-sync-events.test.ts

+});
+
+ruleTester.run(RULE_NAME, rule, {
+  valid: [


You need to include more tests for checking userEvent and fireEvent non-related to Testing Library are not reported so they are valid with await operator.

There is an ongoing refactor for v4 of the plugin which actually will check that for all rules out of the box, so I don't know if you prefer to wait for that version to be released.

@Belco90
Do you mean how about if userEvent or fireEvent are user named objects, it should not apply the rule? Or, if extra functions are added to those objects, for those cases, await may be valid?
In order to ensure that, it has to check if the property name in userEvent or fireEvent is one of the expected event names, such as click, type, tab and so on. Even those methods can be overridden. Am I misunderstanding the question?

The former, so we don't report const declared by the user with the same name. As mentioned, the internal refactor for v4 will pass some helpers to all rules to check this generically, so I'm not sure if you prefer to wait for that.

Thanks for the clarification.
will refactor the code by then if needed.

Belco90 · 2020-10-23T11:29:42Z

tests/lib/rules/no-await-sync-events.test.ts

+  invalid: [
+    // sync events with await operator are not valid
+    ...eventFunctions.map(func => ({
+      code: `() => {


Can you write more realistic code here? So you end up with something like:

import { fireEvent } from '@testing-library/framework' import userEvent from '@testing-library/user-event' import MyComponent from './MyComponent' it('should report sync event awaited', async () => { render(<MyComponent />) await ${func}('foo') await findByQuery('some element') })

and then check in the errors which line the error should appear.

Belco90

Just an extra correct example and I think it would be ready, apart from what we want to do with checking if reported objects are from Testing Library modules or not.

Belco90 · 2020-10-26T08:52:34Z

docs/rules/no-await-sync-events.md

+
+const baz = () => {
+  // ...
+  await userEvent.type(textInput, 'abc', {delay: 1000});


Can you add an extra correct example for userEvent.type without await and delay please?

J-Huang · 2020-10-26T23:57:35Z

Not sure why CI chokes. My main computer died, so I just made a commit through github web page UI to my forked repo.

README.md

Belco90 · 2020-10-29T14:36:39Z

@all-contributors please add @J-Huang for code, test and docs

allcontributors · 2020-10-29T14:36:48Z

@Belco90

I've put up a pull request to add @J-Huang! 🎉

Belco90 · 2020-10-29T22:38:22Z

🎉 This PR is included in version 3.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat(rule): add no-await-sync-events rule

8017327

benmonro previously approved these changes Oct 22, 2020

View reviewed changes

J-Huang marked this pull request as ready for review October 22, 2020 20:01

Belco90 requested changes Oct 23, 2020

View reviewed changes

feat(rule): address feedback

ce84207

J-Huang dismissed benmonro’s stale review via ce84207 October 26, 2020 04:47

feat(rule): update doc

780ac25

J-Huang requested review from Belco90 and gndelia October 26, 2020 04:56

Belco90 requested changes Oct 26, 2020

View reviewed changes

Update no-await-sync-events.md

295ba37

J-Huang requested a review from Belco90 October 26, 2020 22:58

feat(rule): clean doc

47759a6

Belco90 reviewed Oct 27, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

feat(rule): clean doc

d3c3897

J-Huang requested a review from Belco90 October 28, 2020 04:06

Belco90 added the new rule New rule to be included in the plugin label Oct 29, 2020

Belco90 approved these changes Oct 29, 2020

View reviewed changes

Belco90 merged commit 2cc2263 into testing-library:master Oct 29, 2020

allcontributors bot mentioned this pull request Oct 29, 2020

docs: add J-Huang as a contributor #245

Merged

Belco90 added the released label Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rule): add no-await-sync-events rule #240

feat(rule): add no-await-sync-events rule #240

J-Huang commented Oct 22, 2020

Belco90 commented Oct 22, 2020

J-Huang commented Oct 22, 2020

benmonro left a comment

Belco90 left a comment

Belco90 Oct 23, 2020

gndelia Oct 24, 2020

J-Huang Oct 26, 2020 •

edited

Loading

Belco90 Oct 23, 2020 •

edited

Loading

gndelia Oct 24, 2020

J-Huang Oct 26, 2020

Belco90 Oct 23, 2020

J-Huang Oct 26, 2020 •

edited

Loading

Belco90 Oct 26, 2020

J-Huang Oct 26, 2020

Belco90 Oct 23, 2020

J-Huang Oct 26, 2020

Belco90 left a comment

Belco90 Oct 26, 2020

J-Huang Oct 26, 2020

J-Huang commented Oct 26, 2020 •

edited

Loading

Belco90 commented Oct 29, 2020

allcontributors bot commented Oct 29, 2020

Belco90 commented Oct 29, 2020

feat(rule): add no-await-sync-events rule #240

feat(rule): add no-await-sync-events rule #240

Conversation

J-Huang commented Oct 22, 2020

Belco90 commented Oct 22, 2020

J-Huang commented Oct 22, 2020

benmonro left a comment

Choose a reason for hiding this comment

Belco90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-Huang Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Belco90 Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-Huang Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Belco90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-Huang commented Oct 26, 2020 • edited Loading

Belco90 commented Oct 29, 2020

allcontributors bot commented Oct 29, 2020

Belco90 commented Oct 29, 2020

J-Huang Oct 26, 2020 •

edited

Loading

Belco90 Oct 23, 2020 •

edited

Loading

J-Huang Oct 26, 2020 •

edited

Loading

J-Huang commented Oct 26, 2020 •

edited

Loading