Skip to content
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

toBeInTheDOM name is misleading about what it really does #3

Closed
gnapse opened this issue Apr 11, 2018 · 10 comments
Closed

toBeInTheDOM name is misleading about what it really does #3

gnapse opened this issue Apr 11, 2018 · 10 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@gnapse
Copy link
Member

gnapse commented Apr 11, 2018

As reported by @sompylasar in testing-library/dom-testing-library#9 .toBeInTheDOM is not really checking for what it name implies (because it does not have the concept of what "the DOM" is to being with). Right now is just a glorified .toBeInstanceOfClass(HTMLElement).

Refer to the original discussion for all suggested solutions. I'll mention here a few from there plus a new one:

// I kinda favor something like this (exact name is open for discussion)
expect(container).toContainElement(element)

// This makes me wonder if it only checks for direct parent-child relationship
// We could add it too, but not as a solution to this issue
expect(element).toBeAChildOf(parent)

// Keep .toBeInTheDOM allowing it to receive an optional element
// This still does not solve the problem when the container is not given, but I wouldn't rule it out
expect(element).toBeInTheDOM()
expect(element).toBeInTheDOM(container)
@kentcdodds
Copy link
Member

I'm in favor of all three of these matchers, especially these two:

expect(container).toContainElement(element)
expect(element).toBeInTheDOM(container)

👍

@gnapse
Copy link
Member Author

gnapse commented Apr 11, 2018

But .toBeInTheDOM receiving the container optionally, or always? I assume the former, so it does not become a breaking change, right?

@kentcdodds
Copy link
Member

Yes, it would be optional 👍

@gnapse
Copy link
Member Author

gnapse commented Apr 11, 2018

Ok, I should have time to work on some of these later this week, but if someone else can give it a crack, go for it.

(Maybe @sompylasar, since he originally reported it and was needing it?)

@gnapse
Copy link
Member Author

gnapse commented Apr 11, 2018

BTW, I'd rule this out for the time being:

expect(element).toBeAChildOf(parent)

As its name suggest, it'd be checking only for a direct parent-child relationship between two nodes. This is not as useful and it's a bit more brittle than checking a more general ancestry relationship. After all, users on the screen do not really know about direct relationships. They generally care about seeing some elements inside others, and that's what .toContainElement expressed already.

@antsmartian
Copy link

@gnapse I can give it a shot on this, if I get some time tomorrow.

@gnapse gnapse added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers and removed help wanted Extra attention is needed labels Apr 11, 2018
@gnapse
Copy link
Member Author

gnapse commented Apr 19, 2018

@antoaravinth are you still up to get this?

@antsmartian
Copy link

@gnapse Sorry was busy.. Yeah should be able to close this when I get time :)

@gnapse
Copy link
Member Author

gnapse commented Apr 19, 2018

Sure, no problem and no hurries, just wanted to check if your offer was still on.

kentcdodds pushed a commit that referenced this issue Jul 5, 2018
* #3: Moved tests to their own files for easier viewing.

* #3: Added to-contain-element

* #3: Updated to-be-in-the-dom to have container element check

* #3: Updated types to match functions

* #3: Updated documentation for toContainElement and toBeInTheDOM

* #3: Added user to contributors

* #3: Updated to-contain-element for better code coverage

* #3: Reverted tests back to one file

* Set container to be optional in types (toBeInTheDOM)
@gnapse
Copy link
Member Author

gnapse commented Jul 9, 2018

Closing as this was solved in #25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants