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
exact mode for toHaveClass #176
Comments
I feel like comparing the |
When you say "exact mode" you mean that you'd pass the actual string that you expect the If that's the case, then what's wrong with |
hmm no maybe 'exact' isn't the right word. perhaps exclusive would be better? the idea is that if I say |
I'm not going to oppose strongly to this, but I'm also not a fan. I see your point in that using But still, I think it's more explicit in your test if you add a couple of assertions, one to validate the class names you want to make sure are present, and another one to assert the ones you want to make sure are not present. It can be more explicit as to why a test fails. The opposite can make your tests fail in the future for adding new classes that are irrelevant to what you wanted to assert. Also, adding an expect(element).toHaveClass('hello', 'world', { exact: true }) |
Do you guys know if it's possible to create a modifier similar to the as in that would solve the issue of the complexity in |
I'm going to assume that that's not possible for the simple reason that it is not clear how it affects in general any given matcher it is applied to. That is, what would it mean to say expect(element).only.toBe(null)
expect(element).only.toHaveLength(3)
// etc. As for solving the issue you want to solve, like I said, I'm not fully opposed to add this, but would love to hear what others think. One alternative I can think of is to create a new matcher named // <div class="world hello hide" />
expect(element).toHaveClassList('hide', 'hello', 'world') // would return true for the above element
expect(element).toHaveClassList('hello', 'world') // would return false for the above element |
I don't think it's necessary to add a new matcher. From my perspective, making the test pass only if it match exactly provided classes is to force it to unnecessary changes in the future. That said, if you have that need, why not to check the number of classes? expect(element).toHaveClass('hello', 'world')
expect(element.classList).toHaveLength(2) // If you add a new class test must fail here This way |
I actually don't agree. I personally think it's better to have a single expect. That's kind of the point of this lib right? I could also just expect on element.className directly if I wanted. 🙂. Also this wouldn't be a new matcher just a slight mod to the existing one. Not much unlike adding {exact:false} to a getBy query... |
Ok, fair enough. Would you be up to contribute it @benmonro? It's ok if you don't, but it may speed things up. |
Yeah not sure when I'll be able to get to it. I work on Walmart grocery so things are pretty crazy at the moment but if I get some time I'll take a stab |
Yup, indeed. Maybe the social distancing period that comes ahead will give you (or me or others) the extra time. That'd be a silver lining in between all that's going on 😕 Stay safe and stay home 👍 |
I think I'll have some time these days, @benmonro if you want I can try to solve this. If you'd prefer to take care of it yourself, no problem.
👍 |
Go for it I prob won't have time anytime soon
|
I think the name toHaveClassList can lead to confusion.
In my view something like |
I'm fine with just using toHaveClass and just have the last arg be |
I initially did not like that, but now I'm considering it. Though I still like the But back to just adding @testing-library/core-maintainers what's your opinion about this? |
If we go with a new matcher (which I don't think we should) I would use toHaveExactClass() so it's more inline with what we already have. My 2 cents |
Why is order important? |
@kentcdodds yeah i mentioned above order doesn't matter, it's more about being exclusive: #176 (comment) |
I'm going to try to summarize a little bit the current state of this, in order to come to a conclusion. What we want to achieve is to check that the element contains defined classes and only those classes, if it has any more it will fail. // At the end is `toHaveClass` but with a limitation in the number of classes:
expect(element).toHaveClass('hello', 'world')
expect(element.classList).toHaveLength(2) Two solutions have been proposed:
For For the new matcher option the first pourpose was to use My pourpose is to use something similar to aproach used in
Having said that, The next step is to choose an option. If we go for If, on the other hand, we choose the matcher option, I have no problem making a PR with the change, but we would have to choose a name. I preffer something like For my part, I don't see the problem with the In any case I'm ok with both options. Too much text 😅 but I think the conversation was a little tangled and I tried to organize it a bit. |
yeah that sums it up @SergiCL and thank you for doing that PR, it actually looks great to me. I definitely don't want to see the api grow, i vote to just adopting the {exact:true} as it fits w/ the RTL syntax as well so folks will feel comfortable with it. |
Thanks @SergiCL, great sum up. At this point I am ok with the options approach added to the existing matcher. My main concern is that it can lead to confusion as to what "exact" mean. People could think it asserts on the order as well (as Kent did a few comments ago). But I do not think this issue is insurmountable, and the documentation is there to clear it up. Will review the PR as is by tomorrow. @benmonro you're also welcome to review it as well, as is anyone else who wants to. |
Closed in #217. |
Describe the feature you'd like:
exact mode for
toHaveClass()
rather than having to explicitly check for the existence & non existence of classes, it would be nice to have a simpleexact
option that would only match the provided class and fail if anything else is present.Suggested implementation:
Describe alternatives you've considered:
Teachability, Documentation, Adoption, Migration Strategy:
The text was updated successfully, but these errors were encountered: