Feature request: Use `Object.is()` instead of `===` in `t.is()`

[alexrussell]

I was using AVA to test some math stuff the other day and noticed that I needed to use t.deepEqual() to test for NaN. If the === test was swapped out for Object.is() in assert.js it would be able to correctly handle NaN being 'equal' to NaN without having to resort to deepEqual.

The loss we get with switching to Object.is as far as MDN says is that +0 and -0 are not longer treated as equal. That's actually probably okay, as they are 'more different' than two NaNs are, and certainly the NaN problem is far more common than "why doesn't it think my two zeroes are the same?" it could be argued that, during a test, you may well wish to know if you are getting two different zeroes.

Checking the package.json it says that the 'engine' must be node >= 4, and Node.js 4 does indeed support Object.is() according to the ES compatibility table.

---

All that said, the same MDN article linked above, further down, says that maybe using Object.is() could introduce more confusion (though it'd be interesting to know if power assert can correctly give meaningful output in a diff of +0 and -0, as that would probably be what this hinges on) so may best be avoided. If it is decided that this is the case, I thinking adding the specific isNaN checks to is in assert.js would still be really helpful as, arguably, NaN is (in the sense of t.is) NaN, it's just not "strictly 100% guaranteed to be actually the exact same type of NaN value as the NaN you passed into the t.is function", which is not what people want when they say t.is.

[novemberborn]

Makes sense. I agree that +0 shouldn't equal -0, and I doubt that most people would run into that. Making it easier to compare NaN seems sensible.

@avajs/core?

[sotojuan]

I agree with the change 👍

[alexrussell]

This is good news. On the less-good-news side, I have started a PR for this (as I figured it was kinda simple and worth having a go at) and that's when problems started.

1. The tests aren't super simple to work out what's going on due to the use of the failsWith macro, etc. But I kinda worked it out where I could.
2. The tests themselves don't contain much "everything testing", at least in the is/not section where I looked. They simply test that t.is('foo', 'foo') and assumes it's all good for the passing case. However, it'd make more sense to include 'all possible' passing cases, such as true, true, null, null, 0, 0, -0, -0, etc. in order to create a good document for the design of is, as well as ensure what is actually expected by the maintainers continues to be true forever. So I added a few of these in, but got a little stuck on things like [], [] and [1, 2], [2, 1] as the formatting is a bit complex, and not really testing what we want - first off, power assert is a bit useless here as the diff between [] and [] is '' (i.e. an empty string), and while I can write a test for this, it's only testing current behaviour rather than some realistic future expectation (that is says that the two arrays are not the same instance and that's why they're not equal despite looking the same, for example).
3. I changed is to use Object.is but then I had to change not to use Object.is. No major problem there. But then I see that is and not both use an operator key which is currently '==='. Should I change it to Object.is, Object.is() or just remove it entirely like for deepEqual?
4. Presumably the docs should be updated to say that is uses Object.is and not ===. Here's where another issue arises - the macro examples (see lines 635/636 of readme.md) use is, and they assert (in English, not code) that 2 + 2 === 4 because we use is, but now that's not strictly true (it's now Object.is(2 + 2, 4) which is slightly less catchy in the example). If we any examples that use is like this, it could make these parts somewhat more confusing. That said, that section is the only real issue here.

So, with that in mind, I figured I'd come back here in case the maintainers decide that actually that's too much of a change for the sake of supporting NaN and actually somewhat complicates things when is is used as an example.

If we're good to go still, I'm happy to submit the PR and point out the various bits where I made a call that the maintainers may wish to revert.

[alexrussell]

FWIW my changes (no PR yet) are here: https://github.com/avajs/ava/compare/master...alexrussell:is-uses-object-is?expand=1

[novemberborn]

So, with that in mind, I figured I'd come back here in case the maintainers decide that actually that's too much of a change for the sake of supporting NaN and actually somewhat complicates things when is is used as an example.

Let's see what @sindresorhus says.

In the mean time…

The tests themselves don't contain much "everything testing", at least in the is/not section where I looked. They simply test that t.is('foo', 'foo') and assumes it's all good for the passing case. However, it'd make more sense to include 'all possible' passing cases, such as true, true, null, null, 0, 0, -0, -0, etc. in order to create a good document for the design of is, as well as ensure what is actually expected by the maintainers continues to be true forever.

It's hard to do this exhaustively. I'd be happy to see the edge-cases that Object.is() covers that === does not.

So I added a few of these in, but got a little stuck on things like [], [] and [1, 2], [2, 1] as the formatting is a bit complex, and not really testing what we want - first off, power assert is a bit useless here as the diff between [] and [] is '' (i.e. an empty string), and while I can write a test for this, it's only testing current behaviour rather than some realistic future expectation (that is says that the two arrays are not the same instance and that's why they're not equal despite looking the same, for example).

We also don't really have tests for power-assert, see #1324. Don't worry about it here.

I changed is to use Object.is but then I had to change not to use Object.is. No major problem there.

👍

But then I see that is and not both use an operator key which is currently '==='. Should I change it to Object.is, Object.is() or just remove it entirely like for deepEqual?

Remove it for now I think. operator is used mostly for the TAP reporter and it tries to follow a convention, but a lot of our assertions don't have an operator equivalent and I'm not even sure what the convention is.

Presumably the docs should be updated to say that is uses Object.is and not ===.

Yea, let's be explicit on how t.is() and t.not() do their comparisons.

Here's where another issue arises - the macro examples (see lines 635/636 of readme.md) use is, and they assert (in English, not code) that 2 + 2 === 4 because we use is, but now that's not strictly true (it's now Object.is(2 + 2, 4) which is slightly less catchy in the example). If we any examples that use is like this, it could make these parts somewhat more confusing. That said, that section is the only real issue here.

I think it's OK. Perhaps change it to use = in the test title, which is appropriate for a math-expression.

[alexrussell]

@novemberborn Good responses :) I'll have a think if there are a few more test assertions I can add with my limited knowledge of the testing helpers (and I wasn't complaining here, just pointing out that I have added a bunch of assertions and explaining the reasoning) and make the changes to the macro examples.

[alexrussell]

Okay I have updated my branch to use = in the examples and added a few more test assertions for t.is() I figured it wasn't a big deal to do the same for t.not() as it is literally a negation of t.is(). Arguably the tests should be there to ensure that t.not() doesn't stray from being equivalent to ! t.is() so I can do that if required.