Fail when passing undefined as the expectation for equals

[mantoni]

Purpose (TL;DR) - mandatory

This is up for discussion. It's a breaking change and I'm not 100% sure about it.

I noticed that assert.equals(foo.property, bar.property) would pass if both properties are not defined, although it's unlikely that this meets my expectation.

This change throws if the expectation is undefined with a message recommending the use of assert.defined or refute.defined instead.

What are your thoughts on this? If we want to try to protect against cases like these, are there other assertions that should be changed as well?

How to verify - mandatory

1. Check out this branch
2. npm install
3. npm t

Checklist for author

• npm run lint passes
• References to standard library functions are cached.

[mgred]

LGTM

[mantoni]

@mgred LGTM is not a discussion 😆 Do you think it's a good idea to introduce this? Are there other cases that come to your mind?

[coveralls]

Pull Request Test Coverage Report for Build 291

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 286:	0.0%
Covered Lines:	654
Relevant Lines:	654

---

💛 - Coveralls

[mroderick]

In my opinion, it is our responsibility as library stewards to guide users away from making mistakes, where we can reasonably detect them. If the author actually want to compare actual with undefined, then the API provides a method to do so.

I think it would be nice if we changed the API a bit, and added isUndefined as an alias of defined. We should deprecate defined, and remove it in the next major version. isUndefined could then be referenced in the error message. Having isUndefined would match the camelCase style of naming the other assertions for special values, like isNull, isTrue, isNaN.

[mantoni]

I agree with the isUndefined situation. When I started using referee I was intuitively trying this and it didn't work.

[mroderick]

Let's put this PR and #89 in the same major version.

[mantoni]

Let's put this PR and #89 in the same major version.

Yep. I already created the 4.0 milestone and added both.

[mgred]

@mgred LGTM is not a discussion 😆 Do you think it's a good idea to introduce this? Are there other cases that come to your mind?

Sorry @mantoni I was a bit fast on this, but still like the idea and think it makes sense in you described scenario 👍

[mantoni]

I just saw that isUndefined and the deprecation of refute.defined was already released as v3.2.0. Will send a PR for refute.defined removal to prepare for v4.0.0 then.