feat: add prefer-explicit-assert rule #29

Belco90 · 2019-10-21T18:53:50Z

New rule: ~~no-get-by-assert~~ prefer-explicit-assert

This closes #28

Meemaw

Looks good 👍

Left a few smaller comments.

Meemaw · 2019-10-21T19:01:10Z

lib/rules/no-get-by-assert.js

+
+  create: function(context) {
+    return {
+      [`CallExpression > Identifier[name=${/^getBy(LabelText|PlaceholderText|Text|AltText|Title|DisplayValue|Role|TestId)$/}]`](


Could this be extendable by the user if needed? For example we have some custom selectors e.g. getByIcon that I would like to fall under the same rule but should not be part of the default configuration.

Yes, I can make it extendable so users can add their own queries. I always have doubts about just checking getBy* default queries + custom ones or check everything starting by getBy, but I think I prefer the first one and make it extendable so you and other users can add custom queries and avoid false positives with other things.

Meemaw · 2019-10-21T19:02:09Z

lib/rules/no-get-by-assert.js

+      url: getDocsUrl('no-get-by-assert'),
+    },
+    messages: {
+      noGetByAssert: 'Disallowed use of `getBy*` query as implicit assert',


Is this the message user will see on hover in VSCode for example? Should it include some information to use the expect(queryBy*)... pattern?

Yes, this is the message you'll get when linting the code (or your IDE pass the linter for you). To be honest I don't know what to put here, so I just put something simple mirroring other rules from ESLint. Suggestions are more than welcome.

I would go for something like Use queryBy assertions instead of getBy. What do you think?

Same as previous mentions to queryBy: user can't be forced to do that, we can only mention the fact that the getBy must be transformed into explicit assert.

thomaslombart · 2019-10-22T08:02:30Z

docs/rules/no-get-by-assert.md

+```js
+// wrapping the get query within a `expect` and use some matcher for
+// making the assertion more explicit
+expect(getByText('foo')).toBeDefined();


Shouldn't it be expect(queryByText) instead of expect(getByText)? 🤔
Such an example would be conflicting with the prefer-expect-query-by rule.

That's actually from a different rule so I would avoid having this one encouraging two things: asserting explicitly and asserting with queryBy*, so just leaving this rule for the former and prefer-expect-query-by for the latter.

Yes, but as prefer-expect-query-by is in the recommended rules, I found it a little bit weird to say expect(getBy) is correct (although it is for this rule technically speaking) 🙂

Well, in the example of this specific rule that's fine. It would be weird to see expect(queryByText('foo')).toBeDefined(); when the rule only talk about getBy*, so I wanted to clarify that specific use is valid for this rule.

Sure! What about putting a side note stating that though expect(getBy) is valid for this rule, it is recommended to use queryBy for asserting elements' presence? Then, the user would know it's valid to use expect(getBy) while still being aware it's not recommended. What do you think? 🙂

That sounds absolutely reasonable. I'll address the feedback here later.

My 2 cents: I understand both points. However, since this is a documentation to something that is to be considered "valid", I would expect that we write things that are "valid", which is not really the case with expect(getBy), as we also have a recommended rule for it.

At least we should mention to wrap it into an expect and then to change it to queryBy, and link it to the other rule for more reference.

thomaslombart · 2019-10-22T08:08:17Z

docs/rules/no-get-by-assert.md

@@ -0,0 +1,51 @@
+# Disallow `getBy*` queries as assertion method itself (no-get-by-assert)


The rule name sounds a bit strange to me. When it comes to preferences, ESLint rules are usually named prefer-..., I think it should be the case here.
no-get-by-assert implies here that doing assertions with getBy queries is wrong. I think something like prefer-query-by-assert is more clear to the user as discussed in the issue

I'm happy to rename this to prefer-* rule but I would avoid using query-by for same reason I commented before, so I would go for something like prefer-explicit-assert. What do you think?

prefer-explicit-assert sounds good to me!

thomaslombart · 2019-10-22T08:11:46Z

lib/rules/no-get-by-assert.js

+      url: getDocsUrl('no-get-by-assert'),
+    },
+    messages: {
+      noGetByAssert: 'Disallowed use of `getBy*` query as implicit assert',


I would go for something like Use queryBy assertions instead of getBy. What do you think?

thomaslombart · 2019-10-22T08:14:13Z

lib/rules/no-get-by-assert.js

+          !isDirectlyCalledByFunction(node) &&
+          !isReturnedByArrowFunctionExpression(node) &&
+          !isDeclared(node) &&
+          !isReturnedByReturnStatement(node)


I really like this approach. It makes the edge cases clear and readable 👌

I like it too, but I'm always concerned about if I left one edge case out 😨

Belco90 · 2019-10-22T16:30:26Z

All feedback for renaming the rule, add proper references to prefer-expect-query-by rule, add option for custom queries and improve error message have been addressed. Please let me know what you think now :)

thomaslombart

Small tiny fix to do but everything LGTM otherwise 👍

lib/rules/prefer-explicit-assert.js

emmenko

Very nice, thanks for addressing the feedback! I have one additional request, to make sure the rule works in those cases as well.

docs/rules/prefer-explicit-assert.md

emmenko · 2019-10-23T08:28:38Z

tests/lib/rules/prefer-explicit-assert.js

+ruleTester.run('prefer-explicit-assert', rule, {
+  valid: [
+    {
+      code: `getByText`,


Can you also check for when queries are not destructured?

const rendered = render(<App />) rendered.getByText('...') // ...

Nice spot! This actually found some issues in the rule, so I'm adding those tests and fixing the rule implementation.

Tests added, rule fixed and more examples added to the doc

emmenko

Thanks, looks good to me now

thomaslombart

LGTM, great job! 🎉

Belco90 · 2019-10-24T08:44:47Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Belco90 · 2019-10-24T09:27:54Z

@all-contributors please add @emmenko for review

allcontributors · 2019-10-24T09:28:06Z

@Belco90

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

feat: add no-get-by-assert rule

dc1a105

Belco90 assigned thomaslombart Oct 21, 2019

Meemaw reviewed Oct 21, 2019

View reviewed changes

thomaslombart reviewed Oct 22, 2019

View reviewed changes

Belco90 added 4 commits October 22, 2019 16:03

refactor: rename no-get-by-assert rule to prefer-explicit-assert

0b68fd5

docs(prefer-explicit-assert): add references to prefer-expect-query-by

4bd438f

feat(prefer-explicit-assert): add option for custom queries

939a1f7

feat(prefer-explicit-assert): improve error message

1366600

Belco90 changed the title ~~feat: add no-get-by-assert rule~~ feat: add prefer-explicit-assert rule Oct 22, 2019

thomaslombart previously approved these changes Oct 23, 2019

View reviewed changes

lib/rules/prefer-explicit-assert.js Outdated Show resolved Hide resolved

refactor(prefer-explicit-assert): rename error message id

bfcfe4d

Belco90 dismissed thomaslombart’s stale review via bfcfe4d October 23, 2019 08:13

emmenko suggested changes Oct 23, 2019

View reviewed changes

Belco90 added 3 commits October 23, 2019 10:42

fix(prefer-explicit-assert): rename option argument

ec5843b

fix(prefer-explicit-assert): check queries when not destructured

d519aa5

docs(prefer-explicit-assert): add non-destructured queries examples

d2a81bb

emmenko approved these changes Oct 23, 2019

View reviewed changes

thomaslombart approved these changes Oct 24, 2019

View reviewed changes

Belco90 merged commit 7da2b75 into master Oct 24, 2019

Belco90 deleted the feature/no-get-by-assert-rule branch October 24, 2019 08:39

Belco90 added the released label Oct 24, 2019

allcontributors bot mentioned this pull request Oct 24, 2019

docs: add emmenko as a contributor #32

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add prefer-explicit-assert rule #29

feat: add prefer-explicit-assert rule #29

Belco90 commented Oct 21, 2019 •

edited

Meemaw left a comment

Meemaw Oct 21, 2019

Belco90 Oct 21, 2019

Meemaw Oct 21, 2019

Belco90 Oct 21, 2019

thomaslombart Oct 22, 2019

Belco90 Oct 22, 2019

thomaslombart Oct 22, 2019

Belco90 Oct 22, 2019

thomaslombart Oct 22, 2019

Belco90 Oct 22, 2019

thomaslombart Oct 22, 2019

Belco90 Oct 22, 2019

emmenko Oct 22, 2019

thomaslombart Oct 22, 2019

Belco90 Oct 22, 2019

thomaslombart Oct 22, 2019

thomaslombart Oct 22, 2019

thomaslombart Oct 22, 2019

Belco90 Oct 22, 2019

Belco90 commented Oct 22, 2019

thomaslombart left a comment •

edited

emmenko left a comment

emmenko Oct 23, 2019

Belco90 Oct 23, 2019

Belco90 Oct 23, 2019

emmenko left a comment

thomaslombart left a comment

Belco90 commented Oct 24, 2019

Belco90 commented Oct 24, 2019

allcontributors bot commented Oct 24, 2019

		@@ -0,0 +1,51 @@
		# Disallow `getBy*` queries as assertion method itself (no-get-by-assert)

feat: add prefer-explicit-assert rule #29

feat: add prefer-explicit-assert rule #29

Conversation

Belco90 commented Oct 21, 2019 • edited

Meemaw 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

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

Belco90 commented Oct 22, 2019

thomaslombart left a comment • edited

Choose a reason for hiding this comment

emmenko 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

emmenko left a comment

Choose a reason for hiding this comment

thomaslombart left a comment

Choose a reason for hiding this comment

Belco90 commented Oct 24, 2019

Belco90 commented Oct 24, 2019

allcontributors bot commented Oct 24, 2019

Belco90 commented Oct 21, 2019 •

edited

thomaslombart left a comment •

edited