Accept multiple classname arguments in toHaveClass

[jgoz]

What:
Allow multiple class name arguments to be provided to toHaveClass.

Why:
If class names are stored in variables (i.e., if classes are generated at compile or run time), this is a more natural way to assert that multiple classes have been applied. Without this, string concatenation, templating, or multiple assertions would have to be used.

// Before
expect(element).toHaveClass(`${styles.classOne} ${styles.classTwo}`);

// After
expect(element).toHaveClass(styles.classOne, styles.classTwo);

I debated using a variable arguments list rather than an explicit array, but the latter seemed simpler.

How:

Checked argument type for array.

Accept variable argument list, split each argument, and flatten results.

Checklist:

• Documentation
• Tests
• Ready to be merged
• Added myself to contributors table N/A

Open to suggestions on the API or happy to close the PR if this functionality is out of scope for the library.

[kentcdodds]

What if we supported this instead:

expect(element).toHaveClass(styles.classOne, styles.classTwo)

[jgoz]

@kentcdodds Definitely doable. With that API, would you expect all of the arguments to be split?

i.e.,

expect(element).toHaveClass("btn btn-one", "submit default")

This could either fail or it could assert that all 4 classes are present. I would lean toward the latter to avoid confusion. Thoughts?

[kentcdodds]

I think that should be acceptable 👍

[jgoz]

OK - updated to use variable argument list.

[kentcdodds]

LGTM!

[gnapse]

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[gnapse]

I wonder if this change makes it possible to call the matcher without arguments (i.e. an implicit empty array), and what would that mean? Not saying is bad or that we would need to do something to prevent it, just wondering.

expect(element).toHaveClass()

I think if it's true, an assertion like the one above would always pass, right?

[jgoz]

@gnapse You are correct, this is now possible and it does pass. In fact, I added a test case for this scenario in my PR, just to document the behaviour. If you would prefer to require at least one class name, I could open another PR.

[kentcdodds]

Yeah, let's throw an error if no class names are provided as that would probably be a mistake on the part of the developer.

[gnapse]

As I said before, not sure if it's something bad. I think it isn't, it just doesn't make much sense, and the only scenario I'm really worried about is that if a user is using an array also in the test, expecting for something to have some classes, and the array inadvertently for some reason is empty, the test will pass and maybe that's not what the user intended. For example:

// `classes` is an array that for some reason is empty, and that's not what's intended:
// The following test passes, giving the false sense of security that the element has
// some classes that was expected to have
expect(element).toHaveClass(...classes)

However, I'm still not sure we should be babysitting our users this much. Maybe it's ok, let's leave it upon for discussion, maybe just opening it as an issue for now.

[gnapse]

Or not, seems there's more consensus about my initial concern (Kent's comment came in while I was typing mine 😄). Then yes @jgoz, please make a PR with the fix (or is it a breaking change? 🤔)

[jgoz]

OK, I will do this today.

I think you could make an argument for calling it a breaking change, but I would consider it more of a fix for undefined/confusing behaviour.

[jgoz]

@gnapse, @kentcdodds I've got a PR ready to go for the change we discussed above, but I had another idea that you may want to consider:

• Calling .toHaveClass() (without any expected classes) passes if the element has at least one class and fails if the element has no classes
• Calling .not.toHaveClass() passes if the element has no classes and fails if the class has one or more classes

Thoughts on that?

[kentcdodds]

I would rather it fails if no classes are provided because it's probably a user error. Make them be explicit about which class it has or does not have. I suppose in the not case maybe we could allow for no arguments because maybe they're asserting it doesn't have any classes?

[jgoz]

@kentcdodds Makes sense to me. I was thinking the not case would be more useful for an empty class list too.