Skip to content

Update docs to remove toBeInTheDOM #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kentcdodds opened this issue Jul 18, 2018 · 12 comments
Closed

Update docs to remove toBeInTheDOM #75

kentcdodds opened this issue Jul 18, 2018 · 12 comments
Labels
documentation An update needs to be made to documentation help wanted Extra attention is needed

Comments

@kentcdodds
Copy link
Member

Regarding the deprecation of toBeInTheDOM we should either find better assertions or use toBeInTheDocument: testing-library/jest-dom#40

Anyone wanna try that?

For dom-testing-library, most of the examples aren't actually attached to document.body so toBeInTheDocument wont work. We'll have to think of something better on a case-by-case basis I think.

@kentcdodds kentcdodds added help wanted Extra attention is needed documentation An update needs to be made to documentation labels Jul 18, 2018
@gnapse
Copy link
Member

gnapse commented Jul 19, 2018

I'll take this one too, so we can merge and release this prior or alongside to the corresponding change in testing-library/react-testing-library#140.

@gnapse
Copy link
Member

gnapse commented Jul 19, 2018

Indeed toBeInTheDocument is of no use in dom-testing-library's tests as it is today. But this is just in the tests, as the render helper does not attach the generated container to the document.

  1. Is there a reason for this, or can I modify the helper to attach to the document? This may require a cleanup step though.

  2. Alternatively, I can replace all uses of toBeInTheDOM() with toBeInstanceOf(HTMLElement). I already did it and it all worked. toBeTruthy() would also work.

  3. Or perhaps it'd be more acceptable to take the time and always extract the container from what render returns, and replace all expect(query...).toBeInTheDOM() with expect(container).toContainElement(query...).

Which one would be preferable?


As for the README, in case option 1 above is not acceptable, would it be ok if in the README examples we assume the DOM being tested in each example (generally put in comments) is mounted in the document, and we replace all uses of toBeInTheDOM with toBeInTheDocument, just in the README? After all, the only reason not to use it is because of how the tests work, but the README is for general documentation.

@kentcdodds
Copy link
Member Author

I prefer: expect(container).toContainElement(query...) as it's more explicit. I think I'd prefer examples to show that as well.

@gnapse
Copy link
Member

gnapse commented Jul 19, 2018

I'm in doubt about this use of toBeInTheDOM. It seems to want to assert that there's no ... in the message of the error that's expected to be thrown, but that regex should be passed as argument to toThrowError instead of to toBeInTheDOM. And it's passing today because of course that toBeInTheDOM throws an error, albeit an unrelated one, and not the one that's being expected. Am I correct?

@kentcdodds
Copy link
Member Author

Feel free to rewrite that test. I never really liked it much.

@gnapse
Copy link
Member

gnapse commented Jul 19, 2018

Now I have another question:

query* and get* functions in dom-testing-library receive the container, rather than having it associated implicitly as is the case with react-testing-library. This makes it a bit redundant to have to put the container twice when using toContainElement:

expect(container).toContainElement(queryByTestId(container, 'ok-button'))

This happens quite a bit in the README examples. Nothing wrong about that per-se, just wanted to make sure is ok before taking the effort to change it.

@kentcdodds
Copy link
Member Author

Ah, that's fair, and kinda annoying. Maybe we should do toBeInstanceOf(HTMLElement) 🤔

@kentcdodds
Copy link
Member Author

Honestly, for me the real assertion is the get* methods which will throw a more helpful error message if it can't find the element anyway 🙃

@gnapse
Copy link
Member

gnapse commented Jul 19, 2018

Ok, I'll see what I can do.

@SavePointSam
Copy link
Contributor

Now that the library is throwing warning about using toBeInTheDOM, I've moved to toBeInTheDocument as it suggests. However, the flow typings haven't been updated so I'm now getting a bunch of type errors. :(

@kentcdodds
Copy link
Member Author

Would you like to make that update? :)

@kentcdodds
Copy link
Member Author

I think I did this at some point. In any case, they're gone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation An update needs to be made to documentation help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants