Jest-dom: exact mode for toHaveClass

[benmonro]

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 simple exact 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:

[eps1lon]

I feel like comparing the className attribute is the appropriate matcher here. I don't think it's worth it to expand the API for a matcher that ignores class name order (which is important as well).

[gnapse]

When you say "exact mode" you mean that you'd pass the actual string that you expect the class attribute to have? What about order then? Isn't class="hello world" (almost) equivalent to class="world hello"? Would you want to match even that?

If that's the case, then what's wrong with .toHaveAttribute('class', 'hello world')?

[benmonro]

hmm no maybe 'exact' isn't the right word. perhaps exclusive would be better? the idea is that if I say toHaveClass("hello", "world") then hello world and world hello would pass, but hello world foo would fail

[gnapse]

I'm not going to oppose strongly to this, but I'm also not a fan.

I see your point in that using .toHaveAttribute as I suggested also does not work, because it can fail if the order of the class names changes (e.g. you assert for 'hello world' and the element has class="world hello").

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 options argument to .toHaveClass will also make its signature a bit complex, since right now this custom matcher assumes all its arguments are strings that it interprets as class names. This last remark is assuming that what you want to propose is something like this:

expect(element).toHaveClass('hello', 'world', { exact: true })

[benmonro]

Do you guys know if it's possible to create a modifier similar to the not modifier in jest? if so we could do something like only

as in expect(element).only.toHaveClass("hello", "world")

that would solve the issue of the complexity in {exact:true}

[gnapse]

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 .toHaveClassList that would allow to assert on the entire classList of a given element.

// <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

[SergiCL]

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?
Something like this:

expect(element).toHaveClass('hello', 'world')
expect(element.classList).toHaveLength(2) // If you add a new class test must fail here

This way hello word and world hello should pass the test, but if you add another class like in hello world bye it would fail.

[gnapse]

I'd say @SergiCL's suggestion is good enough, and avoids us having to increase the surface API on this. Would you agree @benmonro?

[benmonro]

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

[gnapse]

Ok, fair enough. Would you be up to contribute it @benmonro? It's ok if you don't, but it may speed things up.

[benmonro]

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

[gnapse]

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 👍

[SergiCL]

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.

Stay safe and stay home 👍

👍

[benmonro]

Go for it I prob won't have time anytime soon

[SergiCL]

I think the name toHaveClassList can lead to confusion. jest-extended library makes a distinction between array matchers that could be used here:

• .toIncludeAllMembers([members])
• .toIncludeSameMembers([members])
• .toIncludeAnyMembers([members])

In my view something like toHaveSameClass could be clearer, and leaves the door open for a possible toHaveAnyClass if necessary in the future.

[benmonro]

I'm fine with just using toHaveClass and just have the last arg be {exact: true}

[gnapse]

I initially did not like that, but now I'm considering it. Though I still like the toHaveClassList option more. It kinda expresses that you want to assert on the entire list of classes. toHaveAnyClass is what toHaveClass already does, right?

But back to just adding {exact: true} options, how could such a signature be declared in TypeScript?

@testing-library/core-maintainers what's your opinion about this?

[benmonro]

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

[kentcdodds]

Why is order important?

[benmonro]

@kentcdodds yeah i mentioned above order doesn't matter, it's more about being exclusive: https://github.com/testing-library/jest-dom/issues/176#issuecomment-580920930

[SergiCL]

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:

1. Add {exact: true} to the current toHaveClass matcher.
2. Create a new matcher.

For {exact: true} option I created a PR #217 because I misunderstood the conversation (my fault). I think with that this option is quite clear and there is no need to explain it further.

For the new matcher option the first pourpose was to usetoHaveClassList, but I think the diference with toHaveClass is not very clear.
@benmonro porpouse to use toHaveExactClass but looking at this conversation I think is not very descriptibe.

My pourpose is to use something similar to aproach used in jest-extended for arrays (previous comment for more info)

• toHaveAllClass: True if contains every class or more (current toHaveClass I'm not proposing to replace it)
• toHaveAnyClass: True if contains at least one defined class (not our bussiness at this moment).
• toHaveSameClass: True if contains EXACTLY defined classes (this could be the matcher we are looking for)

Having said that, The next step is to choose an option.

If we go for{exact: true} option
What is missing to close the PR?
Is exact the most comprehensive name?

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 toHaveSameClass or toHaveSameClassList but I'm ok with any other option.

For my part, I don't see the problem with the {exact: true} option, I think it works well and prevents the API from growing.
Maybe could be interesting to use a more descriptive name like exactLength or such as exactClassNumber?

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.

[benmonro]

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.

[gnapse]

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.

[SergiCL]

I think this can be clossed @benmonro @gnapse

[gnapse]

Closed in #217.