jest-dom
jest-dom copied to clipboard
testing-library
Discussion - extending matchers to handle NodeList
Describe the feature:
Would it make sense to extend the current matchers to handle NodeLists? Is there a need for this? I was thinking about queryAllBy* and querySelectorAll and wondering if it would be helpful to pass them directly to the matchers.
For example:
expect(queryAllByText('text')).toBeVisible();
expect(document.querySelectorAll('button')).toBeDisabled();
If all elements match the matcher, then it passes. If any of the elements do not match then it fails.
Sounds good. I'm in favor too.
I've got a question though: what would happen if the list of elements is empty?
The regular rules of logic would dictate that in that case an assertion passes. It gets even more interesting when you consider the negative case too, which should also pass, according to a strict logic:
// allButtons is an empty list in a page without buttons
const allButtons = document.querySelectorAll('button')
expect(allButtons).toBeDisabled(); // Should this pass?
expect(allButtons).not.toBeDisabled(); // And this?
However, I'm not sure that's what should happen. Perhaps these should all throw an exception when they get an empty collection of elements?
Thinking about what people are probably trying to do, I think that an empty list should throw an error in either case. If they want to verify the list is empty then they can add an assertion for that specifically. I think doing anything else could lead to passing tests but a broken app.
Sounds good! I will aim to take a stab at implementing the idea this week!
@gnapse and @kentcdodds, I have an HOF concept but want to make sure it is on the right track. My thought is the HOF wrapping each matcher in index.js would allow us to not have to adjust any of the matchers directly. If the HOF determines that it is not a nodelist it just runs the normal matcher.
https://github.com/smacpherson64/jest-dom/commit/17770e25c5c0a4fece84bc4b7bcfbcf4c9d0fe4d
There is a lot that needs to be cleaned up. Feedback, negative or positive, is appreciated! Thanks for letting me be part!
On a second thought, I'm a bit worried about the complexity this feature introduces in the code, compared to the return. In part I'm thinking of this now, after seeing the code, and realizing that this will make error messages more complex and difficult to be clear enough.
What are you planning to show, for instance, upon failure, as expected and received in the jest error message? If 5 out of 9 elements in a node list fail to pass the expectation, what would be a sensible way to convey all that information? And what if the user, by using an incorrect selector, accidentally matches dozens of elements, a significant part of which pass an expectation and others do not.
I do agree, it adds a bit of complexity.
When it comes to the error messages I think we have a few options to aim for clarity:
Option 1 - The First Error Only
{first error message of matcher}
Option 2 - # out of total
5 out of 10 elements in the NodeList failed, displaying first error:
{first error message of matcher}
Option 3 - Display all failing elements and their indexes
The following elements in the NodeList failed:
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
{first error message of matcher}
Option 4 - All of the Above
5 out of 10 elements in the NodeList failed:
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
The first failed test is displayed below:
{first error message of matcher}
Option 5 - All of the Above, Sampled (for large sets)
500 out of 1000 elements in the NodeList failed, displaying the first five elements below:
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
[index]: element shallow clone
...
The first failed test is displayed below:
{first error message of matcher}
Option 6 - Element Path Example
5 out of 10 elements in the NodeList failed, displaying the first failing element:
[index]: element shallow clone
html > body > div.wrapper > div.container > main.content > section#introduction > p > span
{first error message of matcher}
@gnapse, For this item, I am planning to get it into a working state and so we can see examples of real messages. I think the real messages will help see if the examples make sense or if they are too complicated / convoluted.
Sounds like a plan. I do want to see it first in action. Cause I'm not yet convinced this will be automatically useful without user-friendly messaging.
@gnapse, I have a basic implementation ready, I am putting it as a PR. It is highly likely we will have some tweaks, I just wanted to get a working concept your way for testing purposes and reviewing the code.
Below is an example of 100 elements failing the toBeEmpty matcher:
● NodeList .toBeEmpty › example failing
100 of 100 NodeList elements failed this test. Displaying subset of failing elements:
[0] <span />
html > body > main#main-content > div.wrapper > span
[1] <span />
html > body > main#main-content > div.wrapper > span
[2] <span />
html > body > main#main-content > div.wrapper > span
...
Error message for element [0]:
============================
expect(element).toBeEmpty()
Received:
"a"
============================
@benmonro in that case I'd like you to chime in on the concerns raised above.
Well My thought is to keep it simple and use @smacpherson64 's option 1. The reason for this is that currently the way to do this is to iterate on the list and expect on each element so the behavior would be the same as that.
Ok, fair enough. However, on re-reading the options, I would go for option 2. Is the same as option 1 but it gives a bit more info about the overall state of affairs.