assert: use Object.is comparison in .strictEqual

[BridgeAR]

This aligns assert.strictEqual and assert.notStrictEqual with
assert.deepStrictEqual to use the Object.is() comparison instead
of strict equality.

It looked somewhat nicer with === but I think it makes sense to use the equality checks in both functions.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

assert

[addaleax]

I am a bit worried about NaN tbh … I know I’ve used it quite a bit in tests that I have written

[BridgeAR]

Well, I do not have strong feelings about this. I am fine to close this again but it seemed like it makes sense to align both equality comparisons. So I would like to get some more opinions before closing it again.

[mscdex]

Is Object.is() still slow?

Also, you should be able to do a simple NaN equality check with something like:

if (actual !== actual && expected !== expected) {
 // actual === NaN && expected === NaN
}

[addaleax]

LGTM if CITGM is fine with it

I guess if it’s too bad we’ll have more than enough time to revert before Node 10 :)

[Trott]

Is this unrelated? Or is this doc change due to the change in the code?

[BridgeAR]

I removed the Caveats part as there are non left but I felt like this would still be a valuable information from that part, so I moved it to the top.

[BridgeAR]

@mscdex Object.is is still slower with v8 6.2. It should be on par from 6.3 on. I can optimize it as the following but it is a lot more verbose and not that good to read (untested, just written down quickly).

// assert.strictEqual
if (actual === expected) {
  if (actual === 0 && !Object.is(actual, expected))
    fail()
} else (actual === actual || expected === expected) {
  fail();
}

// assert.notStrictEqual
if (actual !== expected) {
  if (actual !== actual && expected !== expected)
    fail()
} else (actual !== 0 || !Object.is(actual, expected)) {
  fail();
}

As this is semver-major though, I would rather stick to the plain Object.is (v.10 should work with v8 >= 6.3).

[joyeecheung]

LGTM if CITGM is fine with it

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/11603/
CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1101/

[TimothyGu]

This deserves a documentation change warning users that "strictEqual" doesn't actually use strict equality. A YAML annotation showing which version this behavior changed is also essential, because I anticipate many many people are going to see broken tests because of this.

[BridgeAR]

@TimothyGu very good point! I updated the documentation accordingly.

[TimothyGu]

Compatibility still concerns me, but the code changes LGTM.

[jasnell]

Landed in 493340f

[MylesBorins]

It would appear that the citgm results were not looked at. This broke the test suite for eslint.

  1) RuleTester should throw an error when the error message is wrong:
     AssertionError: expected [Function] to throw error matching /Bad var\..*==.*Bad error message/ but got '\'Bad var.\' strictEqual \'Bad error message.\''
      at Context.it (tests/lib/testers/rule-tester.js:118:16)

  2) RuleTester should throw an error when the error is a string and it does not match error message:
     AssertionError: expected [Function] to throw error matching /Bad var\..*==.*Bad error message/ but got '\'Bad var.\' strictEqual \'Bad error message.\''
      at Context.it (tests/lib/testers/rule-tester.js:159:16)

Should we consider reverting this or is this something the eslint test suite should fix?

[MylesBorins]

Sent PR to ESLint

eslint/eslint#9688

this was due to the change in the reported error message. This is likely going to nail anyone who was grepping the text of the strictEqual or notStrictEqual output... do we think this breakage is worth it? It is going to result in some gross legacy code like https://github.com/eslint/eslint/pull/9688/files#diff-0b8e8fdd097af4021779e0f5c567281fR129 in order to support both the old and new message.

[BridgeAR]

@MylesBorins in general the changed error message might produce some more breakage but that should be limited to tests.

What we could do is to use === and !== in case the input does not contain NaN or +-0. That would reduce the breaking change in most cases but it would be somewhat inconsistent to the used comparison. I personally feel like the change here is similar to any error message that we change and land as semver-major.

[SimenB]

(More as a datapoint than anything else)

This is gonna break Jest's test suite as well. Not as clean to fix as eslint due to Jest dumping the full output as a snapshot. https://github.com/facebook/jest/blob/ca1793e01eaf81497ad1985547bfa19c297881c5/integration_tests/__tests__/__snapshots__/failures.test.js.snap#L224-L252.

We already need a ugly hack (jestjs/jest#4851) for the change in node 9 (not merged as we have some issues with hanging tests on node 9 (#17040)), so we'll probably just work around it for 10 as well.

+1 for this change in general though, we made the same change in jest for the upcoming 22 🙂

[gibfahn]

do we think this breakage is worth it?

+1 for this change in general though, we made the same change in jest for the upcoming 22 🙂

At least for me, this is the information that's really useful, whether the people affected by the break agree that the new behaviour is worth having to patch their code. Given the lack of negative response, I'd say no need to revert.