docs: rework disappearance guide to provide more guidance #499

kimroen · 2020-06-12T10:17:27Z

Makes a number of changes to the disappearance guide, moving it in a direction where it recommends
more of the best practices that have appeared since the last time it was uses, as well as taking a stronger
stance of which of multiple options are recommended.

This PR came about because I was answering some questions related to appearance and disappearance, and the people I was hadn't found the answers they were looking for in this guide.

In more detail, the changes are:

Recommend findBy over waitFor
Recommend waitForElementToBeRemoved over waitFor
Use matchers from jest-dom in examples instead of checking for null and then mentioning jest-dom later
Remove inline setup-instructions for jest-dom and instead link to docs
Smaller code example changes (split up separate examples in the same code fence to multiple fences)
Use the word document instead of DOM most places
Use screen in all examples
Add explicit expect statements in all the examples for consistency

Makes a number of changes to the disappearance guide, moving it in a direction where it recommends more of the best practices that have appeared since the last time it was uses, as well as taking a stronger stance of which of multiple options are recommended. In more detail, the changes are: - Prioritize `findBy` over `waitFor` - Use matchers from `jest-dom` in examples instead of checking for `null` and then mentioning `jest-dom` later - Remove inline setup-instructions for `jest-dom` and instead link to docs - Recommend `waitForElementToBeRemoved` over `waitFor` - Smaller code example changes (split up separate examples in the same code fence to multiple fences) - Use the word `document` instead of `DOM` most places - Use `screen.` in all examples - Add explicit `expect` statements in all the examples for consistency

kimroen · 2020-06-12T10:21:25Z

docs/guide-disappearance.md

  })
 })
 ```

+Polling like this is not as efficient as observing for mutations using
+`waitForElementToBeRemoved`, but sometimes it's the best option.


Are there any good examples of when waitFor would work and waitForElementToBeRemoved wouldn't in this context? If not, then maybe it shouldn't have equal space on this page.

agree 👍

If not, I wouldn't even mention a "negated" waitFor as an alternative to waitForElementToBeRemoved

I agree, I've removed the waitFor option here ✨

afontcu

This is great! Thank you for taking the time 🤗

I left some comments here and there, but I love the general direction of this ❤️

docs/guide-disappearance.md

Changes "you can use" to "you need to use" and removes a redundant clause from the sentence. Co-authored-by: Adrià Fontcuberta <[email protected]>

MichaelDeBoey · 2020-06-12T12:16:44Z

docs/guide-disappearance.md

+
+```javascript
+test('movie title appears', async () => {
+  // element is initially not present...


Maybe add an extra assertion to check that it's not present initially?

// element is initially not present... expect(movieTitle).not.toBeInTheDocument()

Maybe, but I feel that isn't entirely relevant and detracts a little from what we're trying to show in this example. Also, asserting that things don't exist is covered further down on the page.

If we did do that though, there should also be code here to do something that would make the title appear after asserting it isn't here, and I'm not sure it makes sense to get into that here.

We could add a comment saying as much after the assertion, like this:

test('movie title appears', async () => { expect(screen.queryByText('the lion king')).not.toBeInTheDocument() // pretend we do something here that should make the title appear const movieTitle = await screen.findByText('the lion king') expect(movieTitle).toBeInTheDocument() })

But I think it might be better to alter the comment in the test to something that makes it clearer that this would be part of a longer test:

Suggested change

// element is initially not present...

// ...code setting up a page that will eventually contain a movie title

Do you think the last suggestion would help, or is there maybe something else we could do to alleviate the confusion or lack of clarity we are hoping to fix by adding this assertion?

I think the first suggestion is much clearer instead of changing the comment tbh

I think the first suggestion is much clearer instead of changing the comment tbh

Fair! In your mind, what is it that is unclear that adding this assertion solves? Or is there something else that makes it so you want it to be added?

On the other hand, you're right with not wanting to have things in here that you'll explain later, so maybe the changed comment is indeed a better option to go with here...

Maybe we can make a full example (like the second suggestion) later on the page? 🤔

I think there's value in keeping the first examples minimal, without having to decipher fake application code. Expanding on the comment and making clear this is a snippet of a larger test should be good.

docs/guide-disappearance.md

kimroen · 2020-06-16T07:40:35Z

I've been trying to think of a good example where we would want to assert that something is present, disappears and is not present, and I'm struggling. I think this is change is valuable without an extra example though, so maybe a good way forward could be to merge it and then work on an example separately if we feel that would add clarity?

@MichaelDeBoey Do you have a specific example we could add, and would you be interested in adding that in a separate PR?

@afontcu I think I have addressed all your comments - are you okay with merging this as it is?

kimroen · 2020-06-16T07:41:26Z

Actually, I'll go in and remove the waitFor variant for waiting for elements to be removed.

afontcu · 2020-06-16T08:41:11Z

docs/guide-disappearance.md

+These do more or less the same thing (`findBy` uses `waitFor` under the hood),
+but the `findBy` version results in simpler code and a better error message.


Okay, this is interesting.

I'm not sure I agree on the statement (I believe using waitFor is more explicit, and helps the test conveying meaning) but that's entirely personal. The question here is whether if docs should showcase two ways of doing the same thing 🤔

If we agree on findBy being simpler and packed with a better error message, then I'd say this is the way docs should suggest 👍

waitFor is useful for other stuff (such as waiting for XHR requests to happen, or other). It might appear somewhere else in docs.

I think this is the last bit that needs some attention, then we can merge!

kimroen · 2020-06-16T09:25:02Z

Some relevant comments by @kentcdodds over here: testing-library/react-testing-library#673 (comment)

kimroen · 2020-06-16T09:26:53Z

docs/guide-disappearance.md

+test('movie title no longer present in document', async () => {
+  // element is present
+  const movie = screen.queryByText('the mummy')
+  await waitForElementToBeRemoved(movie)


I see @kentcdodds commented over here that this should receive a callback, not the element directly: testing-library/react-testing-library#673 (comment)

I feel like I've seen that doing it this way works too though, so I'm not sure.

It can either receive a callback, an element, or an array of elements:

https://github.com/testing-library/dom-testing-library/blob/master/types/__tests__/type-tests.ts#L158-L164

https://github.com/testing-library/dom-testing-library/blob/master/types/wait-for-element-to-be-removed.d.ts

might be worth expanding on this section later with a note about working with React, because sometimes the node gets destroyed and replaced, making these types of tests either pass or fail when they shouldn't

alexkrolick · 2020-06-24T19:14:16Z

docs/guide-disappearance.md

+These do more or less the same thing (`findBy` uses `waitFor` under the hood),
+but the `findBy` version results in simpler code and a better error message.


Suggested change

These do more or less the same thing (`findBy` uses `waitFor` under the hood),

but the `findBy` version results in simpler code and a better error message.

These do more or less the same thing (`findBy` uses `waitFor` under the hood),

but the `findBy` version produces errors more like a `getBy` query,

instead of a general-purpose timeout error.

alexkrolick · 2020-06-24T19:20:50Z

docs/guide-disappearance.md

 })
 ```

-Using
+`waitForElementToBeRemoved` works by using a


I think the waitFor style should also be documented here, particularly for working with React, when the node reference itself is unstable but a retryable query inside a callback would work.

alexkrolick · 2020-06-24T19:27:10Z

docs/guide-disappearance.md

+As opposed to waiting for removal, this is for when you are at a point in your
+test where you know an element shouldn't be present.


This section can be tightened up. It's unclear what "this" refers to here. Also, we shouldn't assume jest-dom is installed and should mention the null check.

Suggested change

As opposed to waiting for removal, this is for when you are at a point in your

test where you know an element shouldn't be present.

If you want to assert that an element is not present _right now_,

rather than testing that it is present and will disappear, use

`queryBy` instead of `getBy` for the query, and then assert that

nothing was found.

alexkrolick · 2020-10-01T18:56:46Z

There are a few open items on this, but it's almost there. Adding some labels for visibility. Hoping someone can finish it off!

kimroen commented Jun 12, 2020

View reviewed changes

afontcu reviewed Jun 12, 2020

View reviewed changes

docs/guide-disappearance.md Outdated Show resolved Hide resolved

docs/guide-disappearance.md Show resolved Hide resolved

kimroen commented Jun 12, 2020

View reviewed changes

docs/guide-disappearance.md Outdated Show resolved Hide resolved

kimroen and others added 2 commits June 12, 2020 13:03

use stronger language and shorten sentence

0184988

Changes "you can use" to "you need to use" and removes a redundant clause from the sentence. Co-authored-by: Adrià Fontcuberta <[email protected]>

fix link to jest-dom

1df6369

MichaelDeBoey requested changes Jun 12, 2020

View reviewed changes

afontcu mentioned this pull request Jun 14, 2020

Unify "Async utilities" page #489

Closed

remove waitFor as a recommended way to wait for removal

6d3b3e5

kimroen requested a review from afontcu June 16, 2020 07:52

afontcu reviewed Jun 16, 2020

View reviewed changes

kimroen commented Jun 16, 2020

View reviewed changes

alexkrolick reviewed Jun 24, 2020

View reviewed changes

alexkrolick added good first issue 🏁 Good for newcomers help wanted 👋 Extra attention is needed hacktoberfest 🎃 labels Oct 1, 2020

nickserv removed the hacktoberfest 🎃 label Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: rework disappearance guide to provide more guidance #499

docs: rework disappearance guide to provide more guidance #499

kimroen commented Jun 12, 2020

kimroen Jun 12, 2020 •

edited

Loading

afontcu Jun 12, 2020

kimroen Jun 16, 2020

afontcu left a comment

MichaelDeBoey Jun 12, 2020

kimroen Jun 12, 2020 •

edited

Loading

MichaelDeBoey Jun 12, 2020

kimroen Jun 12, 2020

MichaelDeBoey Jun 12, 2020

alexkrolick Jun 24, 2020

kimroen commented Jun 16, 2020

kimroen commented Jun 16, 2020

afontcu Jun 16, 2020

afontcu Jun 24, 2020

kimroen commented Jun 16, 2020

kimroen Jun 16, 2020

afontcu Jun 16, 2020 •

edited

Loading

alexkrolick Jun 24, 2020

alexkrolick Jun 24, 2020

alexkrolick Jun 24, 2020

alexkrolick Jun 24, 2020

alexkrolick commented Oct 1, 2020

	// element is initially not present...
	// ...code setting up a page that will eventually contain a movie title

		These do more or less the same thing (`findBy` uses `waitFor` under the hood),
		but the `findBy` version results in simpler code and a better error message.

		As opposed to waiting for removal, this is for when you are at a point in your
		test where you know an element shouldn't be present.

-As opposed to waiting for removal, this is for when you are at a point in your
-test where you know an element shouldn't be present.
+If you want to assert that an element is not present _right now_,
+rather than testing that it is present and will disappear, use
+`queryBy` instead of `getBy` for the query, and then assert that
+nothing was found.

docs: rework disappearance guide to provide more guidance #499

Are you sure you want to change the base?

docs: rework disappearance guide to provide more guidance #499

Conversation

kimroen commented Jun 12, 2020

kimroen Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afontcu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimroen Jun 12, 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

kimroen commented Jun 16, 2020

kimroen commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimroen commented Jun 16, 2020

Choose a reason for hiding this comment

afontcu Jun 16, 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

alexkrolick commented Oct 1, 2020

kimroen Jun 12, 2020 •

edited

Loading

kimroen Jun 12, 2020 •

edited

Loading

afontcu Jun 16, 2020 •

edited

Loading