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

[mlee]

Current behavior:

chai.assert.strictEqual(undefined, undefined) // -> passes

Desired behavior:

chai.assert.strictEqual(undefined, undefined) // -> passes with warning

If asserting on two undefined values, it is better to just use chai.assert.isUndefined. The problem with the current behavior is if the expected and actual arguments to strictEqual are undefined by mistake, the assertion will still pass.

A (contrived) example:

function testedFunc() {
  'asdf'; // desired behavior is actually to return 'asdf'
}

...

var expectedObj = {
  thing: 'asdf'
};

assert.strictEqual(testedFunc(), expectedObj.thin); // second argument should have been expectedObj.thing, but this assertion still passes

[meeber]

@mlee Thanks for the issue. In #726, I wrote:

We have a problem in Chai. The problem is that it's too easy to write tests that appear valid but are actually broken in such a way that they pass no matter what. Strict TDD/BDD can help mitigate this problem, but it's not enough, especially when reviewing PRs from developers of varying skill levels.

That quote was mostly in reference to misspelled assertions (e.g., expect(true).to.be.ture;), but the problem you've identified fits into the same category. Therefore, this is definitely something the team should look into.

I'm flagging this as "more-discussion-needed" for now.

Some things to consider:

• In the name of consistency, should every equality-related assertion on every interface be covered with this kind of protection against undefined?
• Due to the role of automated testing in CI, is a warning message enough, or does Chai need to cause such a test to fail with a helpful error message guiding the user to use undefined instead?

[shvaikalesh]

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.

[keithamus]

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

I really like this idea, and I think maybe this would be the right way to go.

I think while we probably could banish use of undefined from our equality tests, it would violate the principle of least surprise, as @shvaikalesh demonstrated above.

In reality, I think if you access a property of an object, you should be making some assertions about that, for example:

assert.property(objA, key);
assert.isDefined(objA[key]);
assert.isEqual(objA[key], objB[key]);

Or, of course, using the more expressive syntaxes

expect(objA).to.have.property(key).that.is.not.undefined.and.equals(objB[key])

For my own tests, I often find myself asserting on stuff that some argue is a waste of time, for example a recent codebase Im working on has stuff like:

describe('foo', () => {

  it('is a function', () => expect(foo).to.be.a('function'))

  it('takes 2 arguments, returning a boolean', () => expect(foo(1, 2)).to.be.a('boolean'))

  it('throws if given 1 argument', () => expect(() => foo(1)).to.throw(TypeError, 'missing 2nd param')

});

This become very useful, because having a simple test like it('takes 2 arguments, returning a boolean', () => expect(foo(1, 2)).to.be.a('boolean')) is never complex enough that if output changes I could break the test.

[meeber]

@shvaikalesh Why wouldn't Chai be able to accomplish this alone (from a technical perspective, irregardless of whether or not it should)? I think your first example (assert.strictEqual(objA[key], objB[key])) is the exact kind of situation that @mlee is suggesting we protect against.

Wouldn't the implementation be something along the lines of:

assert.strictEqual = function (act, exp, msg) {
  if (act === undefined && exp === undefined) {
    throw Error("Invalid Chai assertion: `assert.strictEqual` cannot be used to compare `undefined` values. Instead, use `assert.isUndefined`. See the docs for more info.");
  }
  new Assertion(act, msg).to.equal(exp);
};

The above protects against writing a test that looks like it's valid, but is actually invalid:

assert.strictEqual(objA[misspelledKey], objB[misspelledKey]); // Only passes because both are undefined

Or maybe instead of "misspelledKey", the code was just refactored and the key names changed, but the developer forgot to update the tests accordingly. Unfortunately, the tests will continue passing despite the refactor to the code because undefined equals undefined. This is one of the worst outcomes possible: the developer believes part of their code is covered by tests, but it's actually not.

@keithamus A linting plugin wouldn't be able to detect the kind of problems that @mlee is attempting to address here, as exemplified above. However, your testing style would catch those kinds of problems. I personally subscribe to the same kind of style for the same reason. But I'm not sure we can rely on other developers to use this style.

Edit: Just to reiterate for the sake of clarity: The purpose of this protection is to prevent writing a test that appears to be valid but is actually invalid and is only passing because undefined === undefined. There's increased risk of a previously valid test becoming invalid in this way later on after a code refactor. The consequence of implementing such protection is that anyone purposefully comparing a value to undefined using an equality comparison would receive the same error message to use isUndefined instead. Thus, adding this protection would be a breaking change if it was an error instead of a warning.

[keithamus]

A linting plugin wouldn't be able to detect the kind of problems that @mlee is attempting to address here, as exemplified above. However, your testing style would catch those kinds of problems

The linter is able to detect when you are accessing properties on an object, to pass into the expectation method (chai is not, as it can only be done with static code analysis). So it can warn you when passing in potentially undefined objects to the actual. To illustrate

expect(foo).to.equal(bar.baz); // eslint-error: no-expected-property Do not pass object properties into `.equal`, as they may be undefined

expect(foo).to.equal(undefined); // this passes lint, as it is explicitly undefined

[keithamus]

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.

[meeber]

@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]

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.

[keithamus]

Also to add;

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

I don't think we do - per se, I think our disagreement lies with what we feel can be done. I agree this is a big problem, and one that needs to be fixed well.

If we could find a solution within chai that could fix this, I'd be happy. But I don't think one exists, the reason being: I think this is more a problem of education. We can educate developers on potential mistakes via analysis of given values, this is useful, but ultimately limited in scope, and outside of the scope of fixing this class of problem (IMO). A tool like eslint can educate developers on potential mistakes, via static code analysis - which is where the class of problem lies.

[meeber]

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

[lucasfcosta]

Hello everyone, awesome considerations and great arguments, from both sides.
I've read all the posts and I agree with @keithamus.

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.

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.

Regarding the example @meeber used, this is what I have to say:

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.
2. If a developer doesn't want something to be undefined he can simply write an assertion for that
3. 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.

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

This pretty much resumes my opinion on this matter:

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

[meeber]

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

[lucasfcosta]

@meeber great arguments! Well, I am still not totally convinced this would be a great idea, but after reading your response to my comment I have a few more considerations to make.

I agree this could benefit our users, but I still don't think it's worth it to create warnings and end up annoying others. I really like the idea of creating a configuration key for that, though, IMO that would be the optimal choice, since we would be able to keep both behaviors, educating developers that want explicit warnings and not annoying those who don't.

Since this is a bit of a personal taste, I'd also like to hear our user's opinion, maybe with a bigger mass of inputs we can take the optimal decision here 😄 What do you guys think about opening a poll?

[keithamus]

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