feat: add prefer-find-by rule #144

gndelia · 2020-05-22T22:29:47Z

This one tackle #127

While current code works, I still have to polish some stuff and add more scenarios (and update the docs)

Belco90 · 2020-05-23T10:26:41Z

docs/rules/prefer-find-by.md

+
+## Further Reading
+
+TBD


I would link to findBy query doc here, as it mentions that findBy is a shortcut for waitFor + getBy. I would use the text from that callout as the main inspiration for this url in the description at the top.

Belco90 · 2020-05-23T10:27:12Z

Looking good so far!

benmonro · 2020-05-23T12:57:36Z

docs/rules/prefer-find-by.md

+```js
+const submitButton = await findByText('foo');
+
+const submitButton = await screen.findAllByRole('table');


I would add some correct usages of waitFor here as well.

benmonro · 2020-05-23T12:59:21Z

docs/rules/prefer-find-by.md

+
+## When Not To Use It
+
+TBD


Suggested change

TBD

Don't use this rule if you don't care if people aren't following best practices of testing library.

I wouldn't go that direct here. Better something nicer like "Not encouraging use of findBy shortcut from testing library best practices".

benmonro · 2020-05-23T13:01:07Z

lib/rules/prefer-find-by.ts

+    docs: {
+      description: 'Suggest using find* instead of waitFor to wait for elements',
+      category: 'Best Practices',
+      recommended: false,


Why not recommend this one?

I think it should be recommended, indeed.

"warn" or "error" ?

Nice question. I think warn is enough? This rules doesn't prevent any malfunctioning code, just warns about a preferred way of writing it.

I added warn

benmonro · 2020-05-23T13:01:47Z

lib/rules/prefer-find-by.ts

+    messages: {
+      preferFindBy: 'Prefer {{queryVariant}}{{queryMethod}} method over using await {{fullQuery}}'
+    },
+    fixable: null,


Can we not make the simple ones fixable?

This is still a draft Ben. Leave the guy work on this! We are discussing in the original issue if addressing it here or in a 2nd PR.

Was just an idea. Not trying to clock this or anything...

gndelia · 2020-05-25T18:23:19Z

I pushed a new version.

CI passes: More test cases!
Docs updated: I added links, details, and more examples

still pending to make it fixable

Belco90 · 2020-05-26T07:53:43Z

lib/rules/prefer-find-by.ts

+
+type Options = [];
+export type MessageIds = 'preferFindBy';
+// TODO check if this should be under utils.ts - there are some async utils


We have ASYNC_UTILS in utils.ts but it includes more methods than here.

Maybe it's a good idea to keep all different async utils under an enumerate type and then build ASYNC_UTILS from all the enumerate options?

after giving some though, I am not sure of moving this one out. It's not a list of all wait methods, but just a list of the ones this rule cares about. Not sure how useful it could be outside of here, as it's not all the wait methods. Either way, we could centralize them in the future if we see the chance

Belco90 · 2020-05-26T07:57:18Z

I pushed a new version.
* CI passes: More test cases!

* Docs updated: I added links, details, and more examples
still pending to make it fixable

Nice work mate. It would be nice to have the fixable code here, but let me know what you think about it and if you prefer to address it in a separated PR in the future.

gndelia · 2020-05-29T23:06:54Z

This PR is ready to be reviewed.

I gave suggestions a try (instead of directly fixing the code), but the problem is that types in ESLint 6 differ from ESLint 7. I have a local branch with the suggestions which I believe are working, but the tests always pass for the suggestions; It's like I cannot force them to fail. I believe that once #138 is finished, suggestions should work better

So this PR goes without suggestions for fix available 😞

Belco90

Looking good mate, thanks for your work! I just requested couple of extra examples and tests, everything else seems fine. We are leaving the fix step for a future PR then right?

docs/rules/prefer-find-by.md

tests/lib/rules/prefer-find-by.test.ts

gndelia · 2020-05-31T19:15:47Z

BTW, I added the new rule to the main README.

Looking good mate, thanks for your work! I just requested couple of extra examples and tests, everything else seems fine. We are leaving the fix step for a future PR then right?

yes. The problem is that when testing suggestions, no matter what the expected output I write, the tests always passes. Let's look at this example.

code: `
            const submitButton = await ${waitMethod}(() => screen.${queryMethod}('foo', { name: 'baz' }))
          `,
          errors: [{
            messageId: 'preferFindBy',
            data: {
              queryVariant: queryMethod.includes('All') ? 'findAllBy': 'findBy',
              queryMethod: queryMethod.split('By')[1],
              fullQuery: `${waitMethod}(() => screen.${queryMethod}('foo', { name: 'baz' }))`,
            },
            suggestions: [{
              messageId: 'preferFindBy',
              // this expected output should match the suggestion output of the rule
              output: `const submitButton = await screen.${queryMethod.includes('All') ? 'findAllBy': 'findBy'}${queryMethod.split('By')[1]}('foo', { name: 'baz' })`
            }]
          }]
        })

No matter what output I write in the test, the tests pass, making me think that there's something wrong with the test assertion declaration.

I think the problem might be solved with ESlint 7 - furthermore, the contract changed in that version so it'll be easier to implement once that is solved.

We could probably go with that in another PR, does that sound good for you?

Belco90 · 2020-06-01T07:50:29Z

@gndelia that sounds good. I remember I had a similar problem when implementing the fix for the waitFor migration. We can wait a little bit until we find proper way to test it, but I think we should go for fix rather than suggestion as there is just single way of changing a getBy + waitFor by a findBy.

Belco90

Just a small text update in the rule description and we are ready to go!

Belco90 · 2020-06-01T07:54:57Z

docs/rules/prefer-find-by.md

@@ -0,0 +1,78 @@
+# Use `find*` query methods to wait for elements instead of waitFor (prefer-find-by)


Can you use here same description from README please?

Suggested change

# Use `find*` query methods to wait for elements instead of waitFor (prefer-find-by)

# Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries (prefer-find-by)

Belco90 · 2020-06-02T07:39:47Z

I've been thinking about this rule name. Could we regret it in the future for being so generic? Could we have in the future other cases where we prefer findBy over other options? I don't know if could be worth it to think about a better name for the rule. ESLint rules names should be short tho, but I'd like to think about it. Could something like no-get-by-wait-for-combo work? Maybe I'm overthinking around this.

benmonro · 2020-06-02T12:55:36Z

I think when it comes to rule names less is more. Either prefer-find-by or no-wait-for-get

gndelia · 2020-06-02T21:01:38Z

we could expand the rules or rename (breaking change) if we get a collision in the future. Not sure which other use case findBy* offers, to prefer it over something else...

Belco90 · 2020-06-03T08:47:36Z

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Belco90 reviewed May 23, 2020

View reviewed changes

benmonro reviewed May 23, 2020

View reviewed changes

gndelia force-pushed the feature/prefer/find-by branch from 3d9d0d8 to 8f49686 Compare May 25, 2020 18:18

Belco90 reviewed May 26, 2020

View reviewed changes

gndelia force-pushed the feature/prefer/find-by branch from 8f49686 to 38a67f7 Compare May 29, 2020 23:03

gndelia marked this pull request as ready for review May 29, 2020 23:04

Belco90 requested changes May 30, 2020

View reviewed changes

docs/rules/prefer-find-by.md Outdated Show resolved Hide resolved

docs/rules/prefer-find-by.md Show resolved Hide resolved

tests/lib/rules/prefer-find-by.test.ts Show resolved Hide resolved

gndelia force-pushed the feature/prefer/find-by branch 3 times, most recently from e649e9d to 03ed88c Compare May 31, 2020 19:11

gndelia force-pushed the feature/prefer/find-by branch from 03ed88c to df905be Compare May 31, 2020 21:36

Belco90 requested changes Jun 1, 2020

View reviewed changes

gndelia force-pushed the feature/prefer/find-by branch from df905be to 12e8b46 Compare June 2, 2020 22:56

feat: add prefer-find-by rule

6b487fe

gndelia force-pushed the feature/prefer/find-by branch from 12e8b46 to 6b487fe Compare June 2, 2020 22:56

Belco90 approved these changes Jun 3, 2020

View reviewed changes

Belco90 merged commit 7a9af37 into testing-library:master Jun 3, 2020

Belco90 added the released label Jun 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add prefer-find-by rule #144

feat: add prefer-find-by rule #144

gndelia commented May 22, 2020

Belco90 May 23, 2020

Belco90 commented May 23, 2020

benmonro May 23, 2020

benmonro May 23, 2020 •

edited

Loading

Belco90 May 24, 2020

benmonro May 23, 2020

Belco90 May 24, 2020

gndelia May 25, 2020

Belco90 May 26, 2020

gndelia May 29, 2020

benmonro May 23, 2020

Belco90 May 24, 2020

benmonro May 24, 2020

gndelia commented May 25, 2020

Belco90 May 26, 2020

Belco90 May 26, 2020 •

edited

Loading

gndelia May 31, 2020

Belco90 commented May 26, 2020

gndelia commented May 29, 2020

Belco90 left a comment

gndelia commented May 31, 2020 •

edited

Loading

Belco90 commented Jun 1, 2020

Belco90 left a comment

Belco90 Jun 1, 2020

gndelia Jun 2, 2020

Belco90 commented Jun 2, 2020

benmonro commented Jun 2, 2020

gndelia commented Jun 2, 2020 •

edited

Loading

Belco90 commented Jun 3, 2020

	TBD
	Don't use this rule if you don't care if people aren't following best practices of testing library.

		@@ -0,0 +1,78 @@
		# Use `find*` query methods to wait for elements instead of waitFor (prefer-find-by)

	# Use `find*` query methods to wait for elements instead of waitFor (prefer-find-by)
	# Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries (prefer-find-by)

feat: add prefer-find-by rule #144

feat: add prefer-find-by rule #144

Conversation

gndelia commented May 22, 2020

Choose a reason for hiding this comment

Belco90 commented May 23, 2020

Choose a reason for hiding this comment

benmonro May 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

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

Choose a reason for hiding this comment

gndelia commented May 25, 2020

Choose a reason for hiding this comment

Belco90 May 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Belco90 commented May 26, 2020

gndelia commented May 29, 2020

Belco90 left a comment

Choose a reason for hiding this comment

gndelia commented May 31, 2020 • edited Loading

Belco90 commented Jun 1, 2020

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

Belco90 commented Jun 2, 2020

benmonro commented Jun 2, 2020

gndelia commented Jun 2, 2020 • edited Loading

Belco90 commented Jun 3, 2020

benmonro May 23, 2020 •

edited

Loading

Belco90 May 26, 2020 •

edited

Loading

gndelia commented May 31, 2020 •

edited

Loading

gndelia commented Jun 2, 2020 •

edited

Loading