The Wayback Machine - https://web.archive.org/web/20201109011349/https://github.com/testing-library/jest-dom/issues/52
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

Discussion - extending matchers to handle NodeList #52

Open
smacpherson64 opened this issue Aug 17, 2018 · 17 comments
Open

Discussion - extending matchers to handle NodeList #52

smacpherson64 opened this issue Aug 17, 2018 · 17 comments

Comments

@smacpherson64
Copy link
Collaborator

@smacpherson64 smacpherson64 commented Aug 17, 2018

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.

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Aug 18, 2018

I'm in favor

@gnapse
Copy link
Member

@gnapse gnapse commented Aug 18, 2018

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?

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Aug 20, 2018

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.

@gnapse
Copy link
Member

@gnapse gnapse commented Aug 20, 2018

Exactly what I was thinking.

@smacpherson64
Copy link
Collaborator Author

@smacpherson64 smacpherson64 commented Aug 21, 2018

Sounds good! I will aim to take a stab at implementing the idea this week!

@smacpherson64
Copy link
Collaborator Author

@smacpherson64 smacpherson64 commented Aug 24, 2018

@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!

@gnapse
Copy link
Member

@gnapse gnapse commented Aug 24, 2018

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.

@smacpherson64
Copy link
Collaborator Author

@smacpherson64 smacpherson64 commented Aug 24, 2018

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}
@smacpherson64
Copy link
Collaborator Author

@smacpherson64 smacpherson64 commented Aug 29, 2018

@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.

@gnapse
Copy link
Member

@gnapse gnapse commented Aug 29, 2018

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.

@smacpherson64 smacpherson64 mentioned this issue Sep 3, 2018
0 of 5 tasks complete
@smacpherson64
Copy link
Collaborator Author

@smacpherson64 smacpherson64 commented Sep 3, 2018

@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.

@smacpherson64
Copy link
Collaborator Author

@smacpherson64 smacpherson64 commented Sep 3, 2018

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"

    ============================
@smacpherson64
Copy link
Collaborator Author

@smacpherson64 smacpherson64 commented Feb 6, 2019

Going to close this for now!

@benmonro
Copy link
Member

@benmonro benmonro commented Dec 4, 2019

what ever happened to this? I would love to see it revived

@benmonro benmonro reopened this Dec 4, 2019
@gnapse
Copy link
Member

@gnapse gnapse commented Dec 5, 2019

@benmonro in that case I'd like you to chime in on the concerns raised above.

@benmonro
Copy link
Member

@benmonro benmonro commented Dec 5, 2019

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.

@gnapse
Copy link
Member

@gnapse gnapse commented Dec 5, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.