Issues with contains, any, and all

[meeber]

While looking at #880, I had a couple of thoughts regarding contains, any, and all that were beyond the scope of that PR:

1. We should keep the behavior of contains consistent across assertions.
2. We should consider deprecating any and all because any encourages bad practices. But if we don't deprecate them, then we should keep their behavior consistent across assertions too.

Before I get in to the second point, I'll outline the various usages of contains, any, and all. Two points of clarification: 1) contains and includes are synonymous; I'll only use contains for consistency. And 2) Although have is sometimes used in our documentation as being the opposite of contains, have isn't actually a flag in Chai, and doesn't have any actual functionality; it's just a language chain. From a technical perspective, the only thing that matters is whether or not the contains flag is set.

Usage 1: contains can be used as a method assertion to assert inclusions of a value in an array, a substring in a string, or a subset of properties in an object.

• Doesn't interact with any or all flags.

Usage 2: keys asserts that a target object/map/set has any or all of the given keys.

• Interacts with any, all, and contains flags.
• When used with any, doesn't matter if contains is set or not; asserts that target has at least one of the given keys.
• When used with all and contains, target must have all the given keys, but is allowed to have more.
• When used with all but not contains, target must have all of the given keys and no others.
• Defaults to all if neither any nor all is specified.

Usage 3: members asserts that a target array has or includes the same members as the given array.

• Interacts with contains flag. Doesn't interact with any or all flags.
• When used with contains, target must have all the given members, but is allowed to have more.
• When not used with contains, target must have all the given members and no others.
• Duplicates are ignored when using contains.

Usage 4: Per #880, strings asserts that a target string contains the given strings.

• Interacts with any and all flags. Doesn't interact with contains flag.
• When used with any, asserts that target has at least one of the given strings.
• When used with all, asserts that target must have all the given strings, but is allowed to have more.
• Defaults to all if neither any or all is specified.

The reason the any flag encourages bad testing habits is because it introduces uncertainty into the test much in the same way that adding an or or either assertion to Chai would. See this comment for my objections to or and either. As for any, consider this statement: "I expect an array to contain any of these values: blue, green, and/or red". Why doesn't the tester know exactly what the array contains, and test exactly for that, instead of testing for a list of possible values? Is the test non-deterministic, or is it being reused for multiple assertions? I haven't been able to think of a situation when it's a good idea to use any.

[vieiralucas]

Thank you @meeber, as always, very detailed stuff!

IMHO Chai could deprecate both any and all. And should follow members for consistency with contains

About any:
I agree with you about how any encourages bad practices and should be deprecated.

About all:
all could be deprecated if chai always chooses the 'all behavior' unless it has the contains flag.
Which is what members is doing.

// as @meeber explained:
expect(['cat', 'dog']).to.have.members(['cat']); // this fails, it wants ALL members
expect(['cat', 'dog']).to.contains.members(['cat']); // this passes, with contains you can match partially

[meeber]

@vieiralucas Agreed except it's worth clarifying that Chai would always choose all behavior even if the contains flag is set; any and all address a different concept than contains and not-contains. In the case of any and all, the concept is whether or not every member must appear in the target. In the case of contains and not-contains, the concept is whether or not the target is allowed to have more members than those that are given.

If the team agrees that any is secretly just an or assertion, and thus should be removed from core, then I think the best deprecation strategy is to:

• Keep all but change it to a language chain with no functionality; after all, it's always active, and there's no reason to break existing tests that use it.
• Remove any but add a check for it in the proxy protection so that a useful error message can be provided if someone attempts to use it; this allows plugins to still define their own any assertion if they so choose, thus automatically overriding the proxy protection.

[lucasfcosta]

I totally agree with you @meeber! That was a great explanation!
I'm sorry for the delay but I have been busy in the last week with some college related issues.
I also agree with @vieiralucas and I think that your strategy to deprecate it is great.

As you've said it yourself, any under the hood is just an or assertion and it creates the possibility of writing "loose" tests, which is tests that allow incorrect implementations to pass due to them being "loose".

+1 for deprecation.

[meeber]

Pinging @shvaikalesh and @keithamus :D

[meeber]

I still think we need to do something here: possibly dump any, but definitely make the behavior described in the first post consistent across relevant assertions.

Arguments in favor of keeping any:

• At this point, it'd be a breaking change to get rid of it.
• Interestingly, any makes negated assertions slightly less-bad... not.any is better than not.all because not.all connects a chain of assertions together via OR, whereas not.any connects them by AND.
• There is potential value of OR-based assertions in end-to-end testing in which nothing is being mocked, particularly random number generators.

Argument in favor of dumping any:

• In most cases using any is a bad practice that is effectively performing multiple assertions in a single line and connecting them via OR. In general, it's much better to assert specifically on a single, expected outcome. In cases in which that isn't possible (like the end-to-end RNG example described above), it may be more appropriate for any functionality to be provided via a plugin, instead of in Chai core.

[meeber]

In the process of rewriting some documentation for an unrelated PR, I ran into a bug that's related to this topic.

Because of this line (I presume), the following tests both fail:

expect({a: 1, b: 2, c: 3}).to.have.keys('a', 'b');  // Correctly fails
expect({a: 1, b: 2, c: 3}).to.not.have.keys('a', 'b'); // Should pass but it fails too

The second test is incorrectly failing; it should pass right away on the basis that the target and given values don't have the same number of keys, which is a requirement when the contains flag isn't set.

[keithamus]

I think we have a good implicit plan for chai 5 where the architecture will change to drop flags, and instead you'll only be able to use greenlit chains of words for assertions:

• all will be a plugin that explicitly applies whatever the assertion you use to every item of an iterable (so e.g. [1,2,3].should.be.a('number') will do typeof [1,2,3] === 'number' while [1,2,3].should.all.be.a('number') will do for(const item of [1,2,3]) { if (typeof item !== 'number') return false }
• any can be deprecated entirely; or if we need it for specific assertions it will only be allowed to used with those specific assertions. If you try to use any in other places, it will fail.
• I want to deprecate members as it doesn't align to any well defined terminology in the language, we will have includes which matches Array|String.prototype.includes.
• I want to deprecate contains for the same reasons as above.

We can keep discussing this, if you have any input - but right now the above points are reflected in the roadmap. I'll close this though, as I want us to move to closing issues when we have something actionable that can sit in the roadmap - which I think we now do.