chai: chai.assert.strictEqual should warn when comparing two undefined values

I’ve agonized over this one quite a bit. I think the cost/benefit is very close, and I could go either way. My current stance is that I support making Chai throw an error when undefined is fed into an equality assertion, but with the following caveats:

• The change is made to all equality assertions, not just assert.strictEqual.
• An error is thrown (instead of just printing a warning). Otherwise, the problem will risk passing by undetected through automated testing.
• The error message is descriptive enough so that the user doesn’t have to consult the docs in order to understand what happened and how to proceed (by using isUndefined instead).
• This protection is enabled by default but a configuration option exists to disable the behavior. That way, the impact of this change on existing projects is drastically reduced, but new projects don’t have to mess with configuration options to get up and running.
• The entire Chai team supports the idea.

That last caveat is key because while I do like this idea, I don’t like it enough to go to war over it. Therefore, I consider this post my closing arguments, and I’ll go along with whatever the rest of the team decides.

Here is a summary of my thoughts regarding the various arguments made against this idea:

1. Users shouldn’t have to consult the docs to use Chai. The error message can be made descriptive enough so that consulting the docs isn’t necessary.
2. Configuration options make it harder to use Chai. Having configuration options that allow power users to disable certain default behavior (e.g., proxy protection or this change) is okay and has a much lower impact than ESLint-style configuration options which require end users to study documentation and write configuration before they can even get started.
3. Any value could be substituted for undefined and it’d be the same problem. The role of undefined in JavaScript opens the door for unique and significantly more common mistakes compared to any other value, particularly when it comes to misspelled or changed property names. In other words, The odds of making this kind of mistake with undefined is so vastly higher than making one in regard to any other value, that it’s not fair to package them together and require that a solution solve the problem for any value, not just undefined.
4. The real problem is feeding object properties into equality assertions. Feeding object properties into equality assertions is a common vehicle for this problem, but it’s not an inherently wrong thing to do. An example of when it’s necessary to feed object properties into an equality assertion is when testing Symbol-based enums, such as expect(unit.profession).to.equal(PROFESSIONS.WIZARD);. You cannot substitute PROFESSIONS.WIZARD with an explicit value if it’s a Symbol.
5. The real problem is creating circular tests. Circular tests is a common vehicle for this problem, but it’s sometimes necessary to create such a test, especially when testing object properties that are Symbols, or are extremely long (like a bitmap).
6. The real solution is linting to prevent property access inside of equality assertions. There are valid uses of property access as noted in the previous two points. Plus it’d be difficult to write a linting rule that handled the fact that the actual property access can occur prior to the assertion, or even in a separate function.
7. The real solution is for the end user to write more defensive tests. Even a defensive programmer can forget to type-check a property before comparing it for equality; it’s not an outrageous mistake. This feature can act as a safety net without violating the idea that better education and defensive testing habits should also be pursued.
8. Not allowing undefined in equality assertions violates the principle of least surprise. The error message can be made descriptive enough to compensate for this surprise.

Just in case this goes any further, I want to shout my objections for configuration options.

I’m going to share some (perhaps tenuously related) sentiment with @meeber here. Testing is scary, and easy to get wrong, and we absolutely should, nay, must help developers where we (reasonably) can. Configuration options eschew this ideal, by making it harder for developers to get running with an optimal setup. I would prefer chai to be zero-configuration (the extreme of which, I believe that chai should only have the expect interface, but that’s another discussion with a probable dead end).

–

Okay back to the conversation at hand.

This is why I am against this idea in general: ideally, I think developers should never have to refer to the docs for chai. Chai should just do the “Most Obvious Thing™”. Proxies made this much closer to reality, because if I do something wrong (like expect(whatever).to.be.undefine()), rather than reading through docs to figure what went wrong, the proxy code just tells me:

Error: Invalid Chai property: undefine. Did you mean "undefined"?

This is awesome because it means I spend less time trawling through docs, and chai is smart enough to set me right. We should definitely do this kind of stuff all the time. I really hope one day we get to be so friendly that our errors look something like…

expect({ foo: 'bar' }).to.equal({ foo: 'baz' });
// Error: expected `{ foo: 'bar' }` to equal `{ foo: 'baz' }`
//        they have different `foo` values the actual value of `foo`
//        is 'bar' but the expected value of `foo` is 'baz'.

expect(someDate).to.equal(otherDate);
// Error: expected `someDate` to equal `otherDate`
//        they have the same values but have different references.
//        Consider `expect(someDate).to.deep.equal(otherDate)` instead.

Rejecting undefined almost does this, it certainly shares a similar principle, but my argument here is that it is a red herring. undefined could be any value, the real issue is that it is coming from an object - and that is what we should reject. We cannot do that though (without static code analysis). In a perfect world, I wish chai could do this:

expect(myObj.color).to.equal(otherObj.color);
// Error: expected `myObj.color` to equal `otherObj.color`
//        they are the same value (`undefined`) but passing an object
//        with a property as an expected value is incorrect.
//        Consider instead writing `expect(myObj.color).to.equal(undefined)`

Note that this would error the same if color was false:

expect(myObj.color).to.equal(otherObj.color);
// Error: expected `myObj.color` to equal `otherObj.color`
//        they are the same value (`false`) but passing an object
//        with a property as an expected value is incorrect.
//        Consider instead writing `expect(myObj.color).to.equal(false)`

We can’t do the above without static code analysis. @meeber is right though, we also can’t rely on static analysis to catch every incantation; like this:

const color = undefined
expect(myObj.color).to.equal(color);
// Is this valid? Or not? Why is this invalid/valid when other styles are invalid?

My issue here is that adding the code to reject undefined values goes a tiny way to solving the real issue, and adds additional headaches for users when it suddenly no longer does “The Most Obvious Thing™”. If I actually want to test an undefined value, I can no longer use .to.equal(undefined), which is “The Most Obvious Thing™”. Instead I get Chai telling me that I should use isUndefined(), which is okay, but then to understand why I’m back to trawling through docs or long READMEs about why this is the case.

So. Ultimately my thoughts are:

• This is a UX problem. It is a hard one to solve without in depth philosophical discussion on our choices
• This is impossible to truly fix in chai’s code, hard to fix in something like eslint.
• IMO, to quote WarGames, “the only winning move is not to play”.

Thanks for the responses/discussion everyone!

I definitely understand the philosophy of chai doing the “Most Obvious Thing” - without consulting documentation, it would indeed be my expectation that strictEqual does a === check, and only a === check. That kind of predictability/intuitiveness is a selling point of using chai for me. I also buy that this kind of problem is not something chai or any assertion library can solve completely, and it is definitely the case that developers should write tests defensively and much of the responsibility is with the developer to avoid the problems described so far.

However, the argument that appeals to me the most is the cost-benefit one brought up by @meeber. I can provide a bit of anecdotal evidence from the codebase that I work on:

• 8567 instances of assert.strictEqual
• 46 instances of comparison between 2 undefined values

Of those 46 instances:

• 14 problematic (comparing two values which access a nonexistent object property/otherwise unintentionally undefined value)
• 32 comparisons against explicit undefined (i.e. assert.strictEqual(someVar, undefined))

I fixed the assertions in the first group on a case-by-case basis (luckily none of these resulted in discovery of an actual production bug!). The second group is a codemod-able problem.

My takeaway was that in our codebase there were no times when you actually want to compare that two things are simultaneously strictly equal and also undefined. Even if you could contrive a case where that might make sense, it doesn’t seem particularly burdensome to just use assert.isUndefined twice.

While there was some fixed cost of addressing existing offenses and a potential increased learning cost when using strictEqual, the benefit was high because it caught bad assertions that were giving us false confidence about the integrity of our code.

@lucasfcosta Thanks for sharing your thoughts!

I think it’ a developer’s responsibility to write testes defensively and that they should be maintained instead of just being trusted after every change, after all, test is code and it should be maintained as such, just as it happens with application code.

I agree, but that doesn’t mean that this idea should be automatically rejected on principle. Instead, every idea should be evaluated independently based on a cost/benefit analysis. There are times when Chai will be able to help developers avoid common pitfalls at a reasonable cost, and there are times when the cost will be too great. I haven’t made up my mind yet on this issue. However, I suspect that even defensive programmers are vulnerable to this type of problem; it’s really not that outrageous or uncommon of a mistake to forget to type-check a value prior to an equality assertion.

If I were a developer I’d really expect undefined to be strict equal undefined and this seems pretty obvious, I think that if we added an warning for that it would just annoy people, since this is the behavior that assertion should have had since the beginning.

Either I’m underestimating the negative impact this change would have on developers, or you and @keithamus are overestimating the negative impact. Perhaps both. This is another area that I haven’t made up my mind. I’m still evaluating. My opinion at the moment is that if I was a new developer using Chai and I wrote an equal assertion comparing a value against undefined, and then Chai told me “nope, use isUndefined instead; the docs explain why”, I wouldn’t be annoyed at all. But if I was upgrading Chai in an existing project and the new version suddenly flagged 200 assertions and told me to change them to isUndefined instead, then I wouldn’t be so thrilled. This cost to existing assertions is currently my biggest hangup in regard to giving this idea my support. Hiding this feature behind a config flag might mitigate the problem, but reduce its value in the process, especially if it defaulted to disabled.

1. I wouldn’t test an specific key of an object against another of a cloned object since this would be redundant (as @keithamus said), instead I’d test the clone function alone, after all, it is a unit test and the unit in case is a function. A simple clone and then asserting a deep equality could solve that more simply and it would also be less error prone.

The details of the example could be reasonably changed to counter your argument. For example, maybe the clone function returns an object where all the properties are the same except a new id is assigned, so each property has to be tested individually, instead of one deep equality. Maybe one of the fields being tested is prohibitively long or contains a bunch of special characters so the only practical way to test it is to compare the property in the source object to the property in the cloned object. We could go back-and-forth on the details of the example but I think the key point is that there are reasonable situations in which a developer will need to directly compare two properties using an equality assertion.

1. If a developer doesn’t want something to be undefined he can simply write an assertion for that

Yes, there’s no debate about whether or not the test can and should be written in a better way to avoid this problem. But it’s my opinion that the kind of mistake being discussed here is easy to make, non-obvious, and destructive. I believe even an experienced, defensive programmer can become vulnerable to this kind of problem. And even though developer education is definitely part of the solution, it’s not mutually exclusive with Chai implementing features that help the developer to avoid common mistakes if it’s reasonable to do so from a cost/benefit perspective. Which to me is the main question here.

1. The cost of implementing warnings when arguments to the strictEqual are undefined would be to annoy users that rely on that behavior and add code to fix something that is not our responsibility (IMO) at all. This is what strictEqual does and it is pretty self-explanatory: if something is === another thing it will pass.

Agreed. As I mentioned above, there are scenarios where this will annoy users, especially in regard to existing assertions, and it’s the main reason I haven’t fully embraced this idea yet. The cost is high in some scenarios. I suspect it’s pretty low in most scenarios, though. I’m still not sure yet what that means for the overall cost/benefit ratio. All I know is that I value the benefit so greatly that I think it’s an idea worthy of objective cost/benefit analysis rather than automatic dismal based on principle.

I think a linting options file would be far better. It keeps the concerns separated and then we would avoid adding this kind of code to our core, which (as I already said) is not our responsibility (IMO).

I agree that the potential for linting to prevent the problem is an important thing to consider when evaluating the cost/benefit of the change. The availability of reliable, lower-cost alternatives is a big factor in decision making. However, I’m unconvinced of the viability of linting to adequately address this problem; can it handle when the object’s property isn’t appearing directly inside the assertion and is instead extracted to a separate variable on a previous line or function? I disagree that this kind of change doesn’t belong in Chai; I feel like it’s appropriate to protect against developer mistakes as long as the cost isn’t too steep (which it could be in this case).

This pretty much resumes my opinion on this matter:

we cannot stop developers writing bad code, we cannot stop developers writing bad tests.

While true, I don’t like the role that this sentiment plays in decision making. It places a lot more value in general principle than specific cost/benefit.

I think the feature being proposed here is similar in nature to the proxy protection we added in 4.x; the main difference is that this feature likely has lower benefit and higher cost than proxies. I think it’s important we base our decision on those benefits and costs, as opposed to general principles (which could’ve been used to reject the proxy idea). As I said before, I’m pretty much sold on the benefit that this feature would provide, but I’m having problems regarding the cost as well as the viability of proposed linting alternatives, and thus haven’t made up my mind yet.

I appreciate all of your points, and you’re not really wrong with anything you say. I suppose my point is more: we cannot stop developers writing bad code, we cannot stop developers writing bad tests. We can mitigate it, but it comes at the cost of making testing more awkward in general, IMO.

The problem the OP has is just one instance of a general problem; relying on circular tests without stringent additional assertions.

To take your test example:

it("cloned cat has the same color as original cat after cloning", function () {
  var cat = new Cat("blue");
  var clonedCat = cloneCat(cat);

  expect(clonedCat.color).to.equal(cat.color);
});

What if my cloneCat function looks like this:

function cloneCat(cat) {
  cat.color = false;
  return cat;
}

The tests pass, despite some obviously broken code, and Chai would have no reasonable way to prevent you doing the above. Who is at fault does not matter, this class of problems is, as you say, easy to trip up on. It is not uncommon to have accidentally passing tests when refactoring - and for this kind of situation to occur (although obviously not nearly as contrived).

The crux of the issue here is: people need to write better tests. We can make our assertions try to be more clever, but ultimately they’ll just assert on values, and it will never fix the root problem; that people need to write better tests.

FWIW, anyone reading this and thinking “how can I fix this in my code? How can I make sure my tests are robust”, I offer these rules of thumb:

1. Avoid testing in a circular way - in other words, avoid testing stuff against itself - the above code has expect(clonedCat.color).to.equal(cat.color); - instead write the literal value (expect(clonedCat.color).to.equal('blue');)
2. Always write assertions as defensively as possible, test everything you can think; for the above test I’d write expect(clonedCat).to.be.an('object').and.not.equal(cat).and.have.property('color', 'blue')
3. Pretend that the person writing the code that passes your tests is the laziest developer you’ve ever met - to do their job they just have to write code that passes your tests. Your job as a tester is to make sure this lazy developer cannot cheat.

@mlee There’s some great discussion here, but I don’t see enough support to continue pursuing this change. I’m gonna go ahead and close the issue. Thank you again for contributing!

@meeber rocks! Awesome explanation, as always! You are a professional problem solver with great analytical skills 😄

Well, considering each one of your points, these are my thoughts:

1. Users shouldn’t have to consult the docs to use Chai

I totally agree that the message could be descriptive enough for our users to fix it right away, but understanding the why of this decision is kind of problematic. It could be easy to fix that, but I don’t think many people will just do that blindly and then they’ll look for the docs and they may even end up reading this issue where we discuss the pros and cons of this decision. I think an error message helps the user to fix that right away but I think they will annoy our users and even raise more questions in their minds.

1. Configuration options make it harder to use Chai

I totally agree with you on this and perhaps the great majority of our userbase will never even see that this option exists. But the cost/benefit relation for implementing this is really high since it involves very little and safe changes to the code.

1. Any value could be substituted for undefined and it’d be the same problem

I’m not sure I fully understand this. You mean that the odds of someone making a mistake when writing the name of an object are very high and thus this would justify adding warnings? If so, this makes sense, but I still think it’s not enough for us to add warnings and this is kind of related with the 8th item of this list.

1. The real problem is feeding object properties into equality assertions.

Agreed 100%.

1. The real problem is creating circular tests.

Agreed, but I don’t think these are the majority of the cases our users will be dealing with, so I’d like to favor most of them by avoiding warnings that may be annoying.

1. The real solution is linting to prevent property access inside of equality assertions.

I would be difficult indeed to write such a linting rule, but I think this is less “invasive” and more accurate.

1. The real solution is for the end user to write more defensive tests.

This makes sense, there’s nothing to add here.

1. Not allowing undefined in equality assertions violates the principle of least surprise.

I think the error message itself can be considered a surprise. If anyone migrates from 3.5.0 to 4.x.x (or whatever version this might be included into) and their testing code starts giving lots of warnings this might be a surprise itself. However it might not be a bad idea to have a descriptive message only.

After all, I don’t disagree with any of your assumptions, I just see then in another way.

Also, after reading all these points, adding a warning really seems like a good idea and I’d like to have it, I’m just not sure the cost/benefit relation of adding this would compensate for the change, so I’d go with the less risky but still somewhat effective option.

Thank you for sharing your thoughts, you rock! 😄

@mlee thanks for the data! That’s a great contribution to our discussion! 😄

I’m sorry, but I didn’t fully understand your last paragraph, so, you mean you think the benefit of having to explicitly change those problematic assertions to make sure none of the inputs were actually undefined was high or did you mean that the benefit of adding warnings yourself was high?

Given that data, (IMO) we have more solid arguments to avoid warnings, since more than 75% of the assertions were explicitly checking variables against undefined values.

It would be great to hear what other users have to say though and what the rest of the team thinks about this matter.

PS.: I’m so proud to be part of such a great open source project and be able to discuss this subject with such dedicated and intelligent people. Thank you all ❤️

@keithamus We agree on general principles, but I think we disagree on the severity of this problem as well as the urgency and cost/benefit of addressing the problem directly in Chai, as opposed to relying on opt-in linting or better developer education. Let me attempt to explain my viewpoint with an example and analysis (which ties into some of the points you made):

it("cloned cat has the same color as original cat after cloning", function () {
  var cat = new Cat("blue");
  var clonedCat = cloneCat(cat);

  expect(clonedCat.color).to.equal(cat.color);
});

If two months from now, another developer swoops in and refactors the Cat class to change color to furColor because they also added an eyeColor property, and they updated 23 tests to reflect this change but forgot to update the one written above, then this test will always pass from now on, even if they accidentally introduce a bug later on that causes cloned cats not to inherit the source cat’s fur color.

So I ask myself:

1. How bad of a problem is this when it occurs?
2. Is this the original developer’s fault for writing a bad test initially?
3. If so, how outrageous or uncommon of a mistake did the original developer make?
4. Is Chai capable of preventing this problem?
5. If so, what is the cost of preventing it?
6. Are there other ways to prevent this problem?
7. Given the severity/commonality of the problem, and the cost associated with Chai preventing the problem, and the availability of other solutions to the problem, should Chai prevent it?

And I answer:

1. It’s extremely bad. Having a passing test that appears to be valid but is actually invalid and is just passing for some unrelated reason is pretty much the worst scenario for an assertion library.
2. Yes, the original developer should’ve asserted against an explicit value, such as expect(clonedCat.color).to.equal("blue");, or at least should’ve been more defensive about checking type.
3. It doesn’t seem like that outrageous or uncommon of a mistake; it could be made by a wide range of developers.
4. Yes, Chai can prevent this problem by throwing an error if undefined is used in an equality assertion.
5. The cost is a small development effort and, more importantly, a breaking change that makes it so that developers are no longer able to test undefined in equality assertions. Instead, they must use the isUndefined assertion for such tests.
6. Possibly. A linting rule or combination of linting rules might be able to prevent all possible variations of this problem from occurring (although it’s more complicated than disallowing property access inside of an assertion, since the actual property access could occur in a previous line or separate function). Additionally, increased developer education and adoption of defensive testing strategies would help reduce this problem.
7. I haven’t made up my mind yet but I’m leaning toward “yes.” I think it’s a really severe problem when it occurs. And I think the mistake that leads to it happening isn’t particularly outrageous or uncommon. And I don’t think the cost to Chai to fix it is prohibitively high. And I think the problem might be too severe to rely on opt-in linting or better developer education and practices.

Regarding the slippery slope: I think each problem should be evaluated independently. My feeling is that undefined is uniquely positioned in JavaScript to cause this kind of problem, and that any attempt at an analogous example using null or falsey values will score much differently regarding the third question of “how outrageous or uncommon of a mistake was made?” I haven’t been able to think of a situation yet involving null or falsey that warrants similar protection.

Also, I’m not sure if the “do we just ban undefined values?” question is applicable because the problem being discussed here affects tests that aren’t supposed to be working with undefined in the first place. But I’ll answer anyway: no, I don’t think Chai should deprecate assertions such as isUndefined or throw an error whenever undefined is used anywhere.

@keithamus Thank you for taking the time to have this discussion! I totally agree that there’s a broader issue here that can only be solved through education and writing better tests. The three points of advice that you provided are all spot-on.

I also agree that there’s no way Chai could defend against errors in logic like the one you used in your example; the only solution in such cases is to write better tests. I also agree there are a lot of similarities between my original example, which could be defended against by Chai, and your example, which could not. And I understand the argument that Chai shouldn’t defend against the problem in my original example if it can’t also defend against the broader problem, such as the one demonstrated in your example, especially because there’s a cost associated with fixing the problem in my original example, and because there are other potential solutions such as linting that don’t have that cost.

Nevertheless, I’m still on the fence on this particular issue. I feel like it’s at least possible that the benefit of protecting against the accidental undefined problem demonstrated in my original example is worth the cost of requiring developers to use isUndefined instead of equality assertions when testing for undefined, even though it’s a subclass of a broader problem that can’t be completely fixed by Chai. There’s a somewhat subjective cost/benefit analysis involved with this that I haven’t yet fully resolved.

The point is, I’m still not yet convinced either way. I’d like to think about it some more, and I’d also love to hear some more opinions: @vieiralucas @lucasfcosta @shvaikalesh.

To expand, the real problem @mlee is having, is that they are giving potentially undefined values to one of our methods. We could fix this, by disallowing undefined values in our methods - but that’s as much of a footgun (IMO) and a slippery slope for us. Do we just ban undefined values? What about null? What about falsey values? What if someone wants to test a method is returning explicitly undefined?

The real fix for @mlee is to fix the broken/insufficient tests. Tests need to be written defensively, arguably more than code. I’m not laying blame with @mlee - but also not laying blame with our methods. This is a soft problem - one of coding style, not one chai can fix, and not one developers can be taught by chai. I think a mediated solution is the right one; for example opting into static code analysis with something like eslint.

I don’t see any way we can accomplish this in Chai alone.

Please, consider a simple example like:

assert.strictEqual(objA[key], objB[key])

If both values are undefined, I should not get the warning. For cases like:

assert.strictEqual(objA[key], undefined)

I would like to get a warning that isUndefined should be used instead.

This issue reminded me of NaN === NaN problem: you would want them equal when either lhs or rhs is literal NaN value, but would not if both are expressions.

Maybe we should have custom ESLint plugin (like AVA has) to help writing better assertions.