feat: no-get-by-for-checking-element-not-present #65

thomaslombart · 2020-01-21T11:49:18Z

Closes #61

thomaslombart · 2020-01-21T16:09:59Z

@Belco90 If you ever see something missing in the docs or misexplained, could you handle it? I have to admit I'm still not really convinced by the usefulness of that rule and I think you have the use cases in mind.

emmenko

Had a quick look, looks good so far 👌

lib/index.js

tests/lib/rules/no-get-by-for-absent-elements.js

docs/rules/no-get-by-for-absent-elements.md

Belco90

It looks like the motivation for this rule has been misunderstood, in part it was my fault because of some wrong examples I added before to previous rule doc. If you see the original issue, you can see properly the two different cases when this rule should be applied:

Asserting elements are not present: here we need to look for a expect statement and few specific matchers as .not.toBeInTheDocument(), .toBeNull() or .toBeFalsy()
Waiting for disappearance: here we do not have to look for a expect statement, but waitForElementToBeRemoved method

The rule doesn't need a huge amount of changes tho, just clarifying things in the docs and examples, add more test cases and maybe improve the rule implementation for some edge cases.

Belco90 · 2020-01-23T08:50:23Z

README.md

+| [no-dom-import](docs/rules/no-dom-import.md)                                                          | Disallow importing from DOM Testing Library                         | ![angular-badge][] ![react-badge][] ![vue-badge][]                        | ![fixable-badge][] |
+| [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md)                                        | Suggest using explicit assertions rather than just `getBy*` queries |                                                                           |                    |
+| [consistent-data-testid](docs/rules/consistent-data-testid.md)                                        | Ensure `data-testid` values match a provided regex.                 |                                                                           |                    |
+| [no-get-by-for-asserting-element-not-present](docs/rules/no-get-by-for-asserting-element-not-present) | Disallow the use of `expect(getBy*)` when elements are not present  |                                                                           |                    |


Suggested change

| [no-get-by-for-asserting-element-not-present](docs/rules/no-get-by-for-asserting-element-not-present) | Disallow the use of `expect(getBy*)` when elements are not present | | |

| [no-get-by-for-asserting-element-not-present](docs/rules/no-get-by-for-asserting-element-not-present) | Disallow the use of `getBy*` queries when checking elements are not present | | |

Belco90 · 2020-01-23T08:50:51Z

docs/rules/no-get-by-for-asserting-element-not-present.md

@@ -1,75 +1,58 @@
-# Disallow the use of `expect(getBy*)` (prefer-expect-query-by)
+# Disallow the use of `expect(getBy*)` when elements are not present (no-get-by-for-asserting-element-not-present)


Suggested change

# Disallow the use of `expect(getBy*)` when elements are not present (no-get-by-for-asserting-element-not-present)

# Disallow the use of `getBy*` queries when checking elements are not present (no-get-by-for-asserting-element-not-present)

Belco90 · 2020-01-23T08:52:34Z

docs/rules/no-get-by-for-asserting-element-not-present.md


 The (DOM) Testing Library allows to query DOM elements using different types of queries such as `getBy*` and `queryBy*`. Using `getBy*` throws an error in case the element is not found. This is useful when:

 - using method like `waitForElement`, which are `async` functions that will wait for the element to be found until a certain timeout, after that the test will fail.
 - using `getBy` queries as an assert itself, so if the element is not found the error thrown will work as the check itself within the test.

-However, when trying to assert if an element is not present or disappearance, we can't use `getBy*` as the test will fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can assert that e.g. `expect(queryByText("Foo")).not.toBeInTheDocument()`.
+However, when trying to assert if an element is not present or disappearance, using `getBy*` will make the test fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can assert that e.g. `expect(queryByText("Foo")).not.toBeInTheDocument()`.


Suggested change

However, when trying to assert if an element is not present or disappearance, using `getBy*` will make the test fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can assert that e.g. `expect(queryByText("Foo")).not.toBeInTheDocument()`.

However, when asserting if an element is not present or waiting for disappearance, using `getBy*` will make the test fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can:

- assert element does not exist: `expect(queryByText("Foo")).not.toBeInTheDocument()`

- wait for disappearance: `await waitForElementToBeRemoved(() => queryByText('the mummy'))`

Belco90 · 2020-01-23T08:57:03Z

docs/rules/no-get-by-for-asserting-element-not-present.md


 The (DOM) Testing Library allows to query DOM elements using different types of queries such as `getBy*` and `queryBy*`. Using `getBy*` throws an error in case the element is not found. This is useful when:

 - using method like `waitForElement`, which are `async` functions that will wait for the element to be found until a certain timeout, after that the test will fail.
 - using `getBy` queries as an assert itself, so if the element is not found the error thrown will work as the check itself within the test.

-However, when trying to assert if an element is not present or disappearance, we can't use `getBy*` as the test will fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can assert that e.g. `expect(queryByText("Foo")).not.toBeInTheDocument()`.
+However, when trying to assert if an element is not present or disappearance, using `getBy*` will make the test fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can assert that e.g. `expect(queryByText("Foo")).not.toBeInTheDocument()`.

 > The same applies for the `getAll*` and `queryAll*` queries too.


I'm not really sure if this rule should be applied to getAllBy. You can't check expect(getAllByText('Foo')[0]).toBeNull() as that would return undefined, so you'll need to use a different matcher. In addition, there is no reference to getAllBy in testing library docs, so I would remove this comment and apply this rule only to getBy.

Belco90 · 2020-01-23T08:59:25Z

docs/rules/no-get-by-for-asserting-element-not-present.md


 > The same applies for the `getAll*` and `queryAll*` queries too.

 ## Rule details

-This rule gives a notification whenever `expect` is used with one of the query functions that throw an error if the element is not found.
+This rule gives a notification whenever `expect` is used with one of the query functions that throws an error if the element is not found.


Suggested change

This rule gives a notification whenever `expect` is used with one of the query functions that throws an error if the element is not found.

This rule gives a notification whenever:

- `expect` is used to assert element does not exist with `.not.toBeInTheDocument()` or `.toBeNull()` matchers

- `waitForElementToBeRemoved` async util is used to wait for element to be removed from DOM

Belco90 · 2020-01-23T09:04:39Z

lib/rules/no-get-by-for-asserting-element-not-present.js

+    docs: {
+      category: 'Best Practices',
+      description:
+        'Disallow the use of getBy* queries in expect calls when elements may be absent',


Suggested change

'Disallow the use of getBy* queries in expect calls when elements may be absent',

'Disallow the use of `getBy*` queries when checking elements are not present',

Belco90 · 2020-01-23T09:06:33Z

tests/lib/rules/no-get-by-for-asserting-element-not-present.js

+      { code: `expect(${queryName}('Hello')).toBeTruthy()` },
+      { code: `expect(rendered.${queryName}('Hello')).toEqual("World")` },
+      { code: `expect(rendered.${queryName}('Hello')).not.toBeFalsy()` },
+    ],


We need valid tests for waitForElementToBeRemoved, which is the second part of the rule.

Belco90 · 2020-01-23T09:07:07Z

tests/lib/rules/no-get-by-for-asserting-element-not-present.js

+    ],
+    []
+  ),
+  invalid: getByVariants.reduce(


We should apply this only to getBy

Belco90 · 2020-01-23T09:08:19Z

tests/lib/rules/no-get-by-for-asserting-element-not-present.js

+        errors: [{ messageId: 'expectQueryBy' }],
+      },
+      {
+        code: `expect(${queryName}('Hello')).toBeUndefined()`,


I'm not sure about this one. toBeUndefined won't work even using queryBy as it returns null so we should not include this one in the rule probably.

Belco90 · 2020-01-23T09:09:43Z

tests/lib/rules/no-get-by-for-asserting-element-not-present.js

+      },
+      {
+        code: `(async () => {
+          await waitForElementToBeRemoved(() => {


I would other variants for this one having arrow function returning implicitly and having a regular function.

Belco90

Rule has to be renamed again for not mentioning asserting in the name, right?

Also: this is a breaking change as we are removing previous prefer-expect-query-by rule, so we need to add BREAKING CHANGE comment when merging this.

README.md

docs/rules/no-get-by-for-asserting-element-not-present.md

tests/lib/rules/no-get-by-for-asserting-element-not-present.js

emmenko

Thank, we're almost there 🙌

emmenko · 2020-01-23T13:04:05Z

docs/rules/no-get-by-for-asserting-element-not-present.md

+```js
+test('some test', () => {
+  const { getByText } = render(<App />);
+  expect(getByText('Foo')).toBeInTheDocument();


Suggested change

expect(getByText('Foo')).toBeInTheDocument();

expect(getByText('Foo')).toBeInTheDocument();

expect(queryByText('Foo')).not.toBeInTheDocument();

expect(queryByText('Foo')).toBeFalsy();

emmenko · 2020-01-23T13:06:05Z

lib/rules/no-get-by-for-asserting-element-not-present.js

+    },
+    messages: {
+      expectQueryBy:
+        'Use `expect(getBy*)` only when elements are present, otherwise use `expect(queryBy*)`.',


Suggested change

'Use `expect(getBy*)` only when elements are present, otherwise use `expect(queryBy*)`.',

'Use `getBy*` only when elements are present, otherwise use `queryBy*` when checking that elements are not present.',

Is when cheking that elements are not present necessary? I feel like it's too much, what about:

Use getBy* only when checking elements are present, otherwise use queryBy*

Belco90

Brilliant! Thank you so much for your effort and sorry for previous misleading comments.

emmenko

Very nice, thanks again for improving the rule! 💯

Belco90 · 2020-01-23T20:06:38Z

Awesome. Tomorrow morning I'll create the PR for await-async-utils so maybe we can wait these 2 together to release v2?

…absent-elements # Conflicts: # README.md

Belco90 · 2020-01-28T15:07:06Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

thomaslombart marked this pull request as ready for review January 21, 2020 16:09

emmenko mentioned this pull request Jan 22, 2020

chore: update jest commercetools/merchant-center-application-kit#1263

Merged

emmenko reviewed Jan 22, 2020

View reviewed changes

tdeekens reviewed Jan 22, 2020

View reviewed changes

docs/rules/no-get-by-for-absent-elements.md Outdated Show resolved Hide resolved

thomaslombart added 5 commits January 23, 2020 09:40

feat: add basic rule for no-get-by-for-absent-elements

0c48fec

feat: add support for waitForElementToBeRemoved

36c0eec

docs: add no-get-by-for-absent-elements

42abb3a

refactor: remove prefer-expect-query-by

333e1c8

docs: change rule name

0d11ebd

thomaslombart force-pushed the feat/no-get-by-for-absent-elements branch from 56e08ca to 0d11ebd Compare January 23, 2020 08:40

test: add combinations of query and rendered.query in rule

585caa1

Belco90 requested changes Jan 23, 2020

View reviewed changes

fix: address PR feedback

c976115

Belco90 changed the title ~~feat: add no-get-by-for-absent-elements~~ feat: no-get-by-for-asserting-element-not-present Jan 23, 2020

Belco90 changed the title ~~feat: no-get-by-for-asserting-element-not-present~~ feat: no-get-by-for-checking-element-not-present Jan 23, 2020

Belco90 requested changes Jan 23, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

Belco90 reviewed Jan 23, 2020

View reviewed changes

docs/rules/no-get-by-for-asserting-element-not-present.md Outdated Show resolved Hide resolved

Belco90 reviewed Jan 23, 2020

View reviewed changes

docs/rules/no-get-by-for-asserting-element-not-present.md Outdated Show resolved Hide resolved

Belco90 reviewed Jan 23, 2020

View reviewed changes

tests/lib/rules/no-get-by-for-asserting-element-not-present.js Outdated Show resolved Hide resolved

Belco90 reviewed Jan 23, 2020

View reviewed changes

tests/lib/rules/no-get-by-for-asserting-element-not-present.js Outdated Show resolved Hide resolved

emmenko reviewed Jan 23, 2020

View reviewed changes

fix: address PR feedback

31503ad

Belco90 previously approved these changes Jan 23, 2020

View reviewed changes

emmenko previously approved these changes Jan 23, 2020

View reviewed changes

Belco90 added 2 commits January 28, 2020 15:45

Merge remote-tracking branch 'origin/master' into feat/no-get-by-for-…

8f31a55

…absent-elements # Conflicts: # README.md

docs: sort supported rules alphabetically

f39276b

Belco90 dismissed stale reviews from emmenko and themself via f39276b January 28, 2020 14:47

Belco90 approved these changes Jan 28, 2020

View reviewed changes

Belco90 merged commit 1aa9238 into master Jan 28, 2020

Belco90 deleted the feat/no-get-by-for-absent-elements branch January 28, 2020 15:04

Belco90 added the released label Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: no-get-by-for-checking-element-not-present #65

feat: no-get-by-for-checking-element-not-present #65

thomaslombart commented Jan 21, 2020 •

edited

Loading

thomaslombart commented Jan 21, 2020

emmenko left a comment

Belco90 left a comment

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 Jan 23, 2020

Belco90 left a comment

emmenko left a comment

emmenko Jan 23, 2020

emmenko Jan 23, 2020

thomaslombart Jan 23, 2020

Belco90 left a comment

emmenko left a comment

Belco90 commented Jan 23, 2020

Belco90 commented Jan 28, 2020

	\| [no-get-by-for-asserting-element-not-present](docs/rules/no-get-by-for-asserting-element-not-present) \| Disallow the use of `expect(getBy*)` when elements are not present \| \| \|
	\| [no-get-by-for-asserting-element-not-present](docs/rules/no-get-by-for-asserting-element-not-present) \| Disallow the use of `getBy*` queries when checking elements are not present \| \| \|

		@@ -1,75 +1,58 @@
		# Disallow the use of `expect(getBy*)` (prefer-expect-query-by)
		# Disallow the use of `expect(getBy*)` when elements are not present (no-get-by-for-asserting-element-not-present)

	# Disallow the use of `expect(getBy*)` when elements are not present (no-get-by-for-asserting-element-not-present)
	# Disallow the use of `getBy*` queries when checking elements are not present (no-get-by-for-asserting-element-not-present)

-However, when trying to assert if an element is not present or disappearance, using `getBy*` will make the test fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can assert that e.g. `expect(queryByText("Foo")).not.toBeInTheDocument()`.
+However, when asserting if an element is not present or waiting for disappearance, using `getBy*` will make the test fail immediately. Instead it is recommended to use `queryBy*`, which does not throw and therefore we can:
+- assert element does not exist: `expect(queryByText("Foo")).not.toBeInTheDocument()`
+- wait for disappearance: `await waitForElementToBeRemoved(() => queryByText('the mummy'))`

-This rule gives a notification whenever `expect` is used with one of the query functions that throws an error if the element is not found.
+This rule gives a notification whenever:
+- `expect` is used to assert element does not exist with `.not.toBeInTheDocument()` or `.toBeNull()` matchers
+- `waitForElementToBeRemoved` async util is used to wait for element to be removed from DOM

	'Disallow the use of getBy* queries in expect calls when elements may be absent',
	'Disallow the use of `getBy*` queries when checking elements are not present',

	'Use `expect(getBy)` only when elements are present, otherwise use `expect(queryBy)`.',
	'Use `getBy` only when elements are present, otherwise use `queryBy` when checking that elements are not present.',

feat: no-get-by-for-checking-element-not-present #65

feat: no-get-by-for-checking-element-not-present #65

Conversation

thomaslombart commented Jan 21, 2020 • edited Loading

thomaslombart commented Jan 21, 2020

emmenko 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

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

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

Belco90 left a comment

Choose a reason for hiding this comment

emmenko left a comment

Choose a reason for hiding this comment

Belco90 commented Jan 23, 2020

Belco90 commented Jan 28, 2020

thomaslombart commented Jan 21, 2020 •

edited

Loading