feat(rule): prefer expect queryBy #22

emmenko · 2019-10-16T07:16:44Z

Closes #21

This a port of the implementation of the rule prefer-expect-query-by from this package.

As mentioned, we would like to eventually deprecate our own package in favor of this one, as it has more rules.

Let me know if the rule is "good enough" for now. We've already been using it in our apps and works great so far.

Belco90

Looking good so far, the rule is even autofixable! I have requested few changes, let me know if you have questions or want to discuss something.

package-lock.json

tests/lib/rules/prefer-expect-query-by.js

lib/index.js

docs/rules/prefer-expect-query-by.md

lib/rules/prefer-expect-query-by.js

tests/lib/rules/prefer-expect-query-by.js

Belco90 · 2019-10-16T08:05:09Z

tests/lib/rules/prefer-expect-query-by.js

+      errors: [{ messageId: 'expectQueryBy' }],
+    },
+  ],
+});


You also need to add some tests for autofixing feature, you can find some examples in tests/lib/rules/no-dom-import.js and again @thomlom can help you with it if you need something else.

Ah didn't know that. I added it now. Thanks

Some tests are going to be needed for sure, something like that:

{ code: ` const message = getByText('Hello') expect(message).toBeInTheDocument() `, errors: [{ messageId: 'expectQueryBy' }], output: ` const message = queryByText('Hello') expect(message).toBeInTheDocument() `, },

If auto-fixing is complex technically, I think we can leave it as an improvement.

@thomlom those specific tests for matched elements saved in a var are not necessary, but I found an issue with the current autofix so probably we need to leave it as an improvement.

@emmenko you are fixing the query used inside expect statement, but you also need to take care about if the query is imported or not.

Let's say I have this test:

const { getByText } = render(<Foo />); // ... expect(getByText('bar')).not.toBeInTheDocument();

would be replaced by

const { getByText } = render(<Foo />); // ... expect(queryByText('bar')).not.toBeInTheDocument();

but it's not 100% fixed as queryByText is not defined so you would have to destructure/get it from render (which could be harder than it seems as it should be get from destructured object or var declared by the user).

And another case:

const { getByText, queryByText } = render(<Foo />); // ... expect(getByText('bar')).not.toBeInTheDocument();

would be replaced by

const { getByText, queryByText } = render(<Foo />); // ... expect(queryByText('bar')).not.toBeInTheDocument();

so here queryByText is already defined and rule doesn't have to change anything related to this.

So here we have to 1) improve the fix function to take care of the new query being defined or not or 2) ignore fixing the rule for now. I'm happy with option 2 and leave the autofix for future improvement as implement the proper autofix here could take longer than expected.

Fixing rules do not need to account for other linting errors. As stated in the eslint docs

Since all rules are run again after the initial round of fixes is applied, it’s not necessary for a rule to check whether the code style of a fix will cause errors to be reported by another rule

So from my understanding, this is totally fine how the rule autofix is implemented regarding its concerns.

Sorry, I understand what you mean, but even if it's explained in the rule info I don't want to leave this autofix with the possibility of breaking the code as it would harm the developer experience.

I totally get what you mean and I agree on the fact that fixing rules are only useful the first time.

But this plugin is new and the testing library's ecosystem is still growing. There are some best practices emerging but I think it's still not clear for newcomers to get how to make the most of Testing Library. My concern is that people will rely on this plugin to implement best practices for them automatically, possibly break their tests and have a bad experience with it.
Even if we state it in the docs, this rule will be in the recommended ones. And I'm pretty sure there will be people who will:

Install the plugin (and don't care of the docs since they just want the recommended ones)

Run eslint --fix

See their tests break

Remove the plugin

Checkout the code

I prefer to have a limited experience that leaves you the manual work instead of a full experience that might cause your code to break.

So you want to remove/disable the autofix for now? It's fine by me, I just wanted to share my perspective on this topic before we decide on how to proceed.

Of course! This is not about what I want or something like that :) really happy to have your point of view here. Better to remove the fixing implementation (including the badge and the output in the tests, sorry about the last one as I asked that), and think about it in a future issue. For first rules we added to the plugin we also skipped the autofix for most of them as there are some cases we didn't know what to do or it took longer than expected to be implemented, so it's better to go bit by bit.

It's disabled here: 0fd4a6c

I kept it commented out, so that we have a starting point in case we want to pick it up in the future.

emmenko · 2019-10-16T08:11:16Z

Thanks for the feedback, I'll take a look at it later.

thomaslombart

Thanks a lot for this PR! 👍

There is still the case of assigning a getBy query that needs to be implemented but the rest LGTM.
If you need a hand to finish that, let me know!

tests/lib/rules/prefer-expect-query-by.js

docs/rules/prefer-expect-query-by.md

lib/rules/prefer-expect-query-by.js

thomaslombart · 2019-10-16T10:02:25Z

tests/lib/rules/prefer-expect-query-by.js

+      errors: [{ messageId: 'expectQueryBy' }],
+    },
+  ],
+});


Some tests are going to be needed for sure, something like that:

{ code: ` const message = getByText('Hello') expect(message).toBeInTheDocument() `, errors: [{ messageId: 'expectQueryBy' }], output: ` const message = queryByText('Hello') expect(message).toBeInTheDocument() `, },

If auto-fixing is complex technically, I think we can leave it as an improvement.

tests/lib/rules/prefer-expect-query-by.js

Belco90 · 2019-10-16T10:23:14Z

tests/lib/rules/prefer-expect-query-by.js

+      errors: [{ messageId: 'expectQueryBy' }],
+    },
+  ],
+});


@thomlom those specific tests for matched elements saved in a var are not necessary, but I found an issue with the current autofix so probably we need to leave it as an improvement.

@emmenko you are fixing the query used inside expect statement, but you also need to take care about if the query is imported or not.

Let's say I have this test:

const { getByText } = render(<Foo />); // ... expect(getByText('bar')).not.toBeInTheDocument();

would be replaced by

const { getByText } = render(<Foo />); // ... expect(queryByText('bar')).not.toBeInTheDocument();

but it's not 100% fixed as queryByText is not defined so you would have to destructure/get it from render (which could be harder than it seems as it should be get from destructured object or var declared by the user).

And another case:

const { getByText, queryByText } = render(<Foo />); // ... expect(getByText('bar')).not.toBeInTheDocument();

would be replaced by

const { getByText, queryByText } = render(<Foo />); // ... expect(queryByText('bar')).not.toBeInTheDocument();

so here queryByText is already defined and rule doesn't have to change anything related to this.

So here we have to 1) improve the fix function to take care of the new query being defined or not or 2) ignore fixing the rule for now. I'm happy with option 2 and leave the autofix for future improvement as implement the proper autofix here could take longer than expected.

emmenko · 2019-10-16T11:32:43Z

Alright, thanks a lot for all the feedback so far. I think this is ready for a final review.

thomaslombart

Last tiny fix, everything LGTM otherwise. Thanks again! 👍

thomaslombart · 2019-10-16T11:57:36Z

lib/rules/prefer-expect-query-by.js

+    },
+    schema: [],
+    type: 'suggestion',
+    fixable: 'code',


Suggested change

fixable: 'code',

fixable: null,

Ah thanks, I missed that

Belco90

Awesome!

Belco90 · 2019-10-16T13:50:22Z

This PR is included in version 1.2.0 🎉

emmenko · 2019-10-16T14:04:54Z

Awesome, thanks a lot! 🚀

Belco90 · 2019-10-18T09:01:45Z

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

allcontributors · 2019-10-18T09:01:55Z

@Belco90

I've put up a pull request to add @emmenko! 🎉

feat(rule): prefer expect queryBy

62ed216

Belco90 requested changes Oct 16, 2019

View reviewed changes

emmenko added 2 commits October 16, 2019 10:57

chore: regenerate lockfile

353cc77

refactor: improve docs and tests, include also findBy

093b44b

thomaslombart suggested changes Oct 16, 2019

View reviewed changes

Belco90 requested changes Oct 16, 2019

View reviewed changes

emmenko added 4 commits October 16, 2019 12:46

refactor: remove occurrences of findBy

e3e4231

refactor: generate test cases for all query methods

53a950c

refactor: disable autofix implementation

0fd4a6c

refactor: remove fixable badge

6a3f264

emmenko requested review from Belco90 and thomaslombart October 16, 2019 11:32

thomaslombart previously approved these changes Oct 16, 2019

View reviewed changes

refactor: fixable option to null

67d305c

emmenko dismissed thomaslombart’s stale review via 67d305c October 16, 2019 12:39

Belco90 approved these changes Oct 16, 2019

View reviewed changes

Belco90 merged commit 260f849 into testing-library:master Oct 16, 2019

emmenko deleted the nm-rule-prefer-expect-query-by branch October 16, 2019 14:04

allcontributors bot mentioned this pull request Oct 18, 2019

docs: add emmenko as a contributor #25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rule): prefer expect queryBy #22

feat(rule): prefer expect queryBy #22

emmenko commented Oct 16, 2019

Belco90 left a comment

Belco90 Oct 16, 2019

emmenko Oct 16, 2019

thomaslombart Oct 16, 2019

Belco90 Oct 16, 2019

emmenko Oct 16, 2019

Belco90 Oct 16, 2019

thomaslombart Oct 16, 2019

emmenko Oct 16, 2019

Belco90 Oct 16, 2019

emmenko Oct 16, 2019

emmenko commented Oct 16, 2019

thomaslombart left a comment •

edited

Loading

thomaslombart Oct 16, 2019

Belco90 Oct 16, 2019

emmenko commented Oct 16, 2019

thomaslombart left a comment

thomaslombart Oct 16, 2019

emmenko Oct 16, 2019

Belco90 left a comment

Belco90 commented Oct 16, 2019

emmenko commented Oct 16, 2019

Belco90 commented Oct 18, 2019

allcontributors bot commented Oct 18, 2019

feat(rule): prefer expect queryBy #22

feat(rule): prefer expect queryBy #22

Conversation

emmenko commented Oct 16, 2019

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

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

emmenko commented Oct 16, 2019

thomaslombart left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko commented Oct 16, 2019

thomaslombart 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 left a comment

Choose a reason for hiding this comment

Belco90 commented Oct 16, 2019

emmenko commented Oct 16, 2019

Belco90 commented Oct 18, 2019

allcontributors bot commented Oct 18, 2019

thomaslombart left a comment •

edited

Loading