assert: use same value equal for deepStrictEqual NaN #15036

BridgeAR · 2017-08-25T22:25:03Z

I think it makes sense to further improve assert a bit and to accept NaN as well.

I also fixed the documentation a bit by adding missing changelogs to the assert docs.

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, doc

Trott · 2017-08-25T22:27:48Z

doc/api/assert.md

-  comparison. (Which means they are free of the [caveats][]).
+1. Primitive values besides NaN are compared using the [Strict Equality
+   Comparison][] ( `===` ). Set and Map values, Map keys and NaN are compared
+   using the [SameValueZero][] comparison (Which means they are free of the


Nit: Which -> which

vsemozhetbyt · 2017-08-25T22:43:12Z

Maybe this line with exposed markdown syntax in JS example worth fixing? It can be confusing a bit.

BridgeAR · 2017-08-25T22:46:43Z

@vsemozhetbyt thx, fixed

Trott · 2017-08-25T22:49:34Z

test/parallel/test-assert-deep.js

@@ -466,4 +466,10 @@ assertOnlyDeepEqual(
  assertDeepAndStrictEqual(m3, m4);
 }

+// Handle NaN
+{
+  assert.throws(() => assert.deepEqual(NaN, NaN), assert.AssertionError);


Nit: Leaving out the braces around the arrow function adds an implicit return. Since the returned value is not used, I prefer to add braces. Feel free to ignore, but my two cents on it.

Trott · 2017-08-25T22:50:29Z

test/parallel/test-assert-deep.js

+// Handle NaN
+{
+  assert.throws(() => assert.deepEqual(NaN, NaN), assert.AssertionError);
+  assert.doesNotThrow(() => assert.deepStrictEqual(NaN, NaN));


Since tests are a form of documentation, maybe add an additional test case or two that demonstrates the utility a bit more:

assert.doesNotThrow(() => { assert.deepStrictEqual({ a: NaN }, { a: NaN }); assert.doesNotThrow(() => { assert.deepStrictEqual( [ 1, 2, NaN, 4 ], [ 1, 2, NaN, 4 ]);

Trott

LGTM (with or without my comments addressed) if CI passes

BridgeAR · 2017-08-25T22:55:41Z

I updated the tests according to your suggestions @Trott

vsemozhetbyt · 2017-08-25T22:57:14Z

doc/api/assert.md

@@ -102,6 +102,9 @@ parameter is undefined, a default error message is assigned.
 <!-- YAML
 added: v1.2.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/12142


Is this the proper link?

Good catch 😃

refack

More request comment, than request change.

refack · 2017-08-26T02:58:36Z

test/parallel/test-assert-deep.js

@@ -466,4 +466,11 @@ assertOnlyDeepEqual(
  assertDeepAndStrictEqual(m3, m4);
 }

+// Handle NaN
+assert.throws(() => { assert.deepEqual(NaN, NaN); }, assert.AssertionError);


This is not intuitive. I'd assume that

(deepStrictEqual should be more strict than deepEqual).
What's the rationale for not allowing assert.deepEqual(NaN, NaN)?

That is the bad part about the wording with strict. The strict stands for more exact in this case. deepEqual is actually more like a looksSimilar and it actually fails at seeing the similarity in this case even though it exists. I can add that check to deepEqual as well but I think it would be weird to have it there (even though I would call deepEqual broken anyway - we actually also use a own lint rule to prevent usage of it!).

Actually that is IMHO a great idea, create a better named superset assertion. That would also make this change semver-minor.

sameValues might be a better name (that way neither the word "strict" nor "deep" are necessary)

I think this is absolutely not the right thing to do. And as I tried to explain earlier deepStrictEqual is not strictly a superset of deepEqual because it was just a poor naming from the beginning on. Now we have to live with that naming. Most people do for example not know that NaN is not equal to itself when compared with strict equality. And they also do not know about the other existing equality comparisons in JS. They mostly do not even know how abstract equality is defined - at least that is what my experience tells me.
The comparisons in general are well described here. Adding a new API would not help anyone and would instead create more confusion.

I agree that assert.deepStrictEqual(NaN,NaN) makes sense intuitively.
But ECMAScript abstract equal is "broken" in this sense ((NaN == NaN) === false) and strict equal is broken ((NaN === NaN) === false) is the exact same way.
So IMHO we should be consistent with the standard if we keep using the term equal in our end-point names.

That's why I think breaking away ~~for~~from legacy with a same end-point might be a good idea.

The deepEqual and deepStrictEqual functions have much more then just the "basic" comparison and therefore do not correspond to these properly. I would love if that would be the only distinction because in that case deepEqual would not be so badly broken but that is not the case and it will not change either.
Adding another random name makes the issue not less an issue but just added more problems.

lpinca · 2017-08-26T16:47:08Z

lib/assert.js

-  if (actual === null || expected === null ||
-    typeof actual !== 'object' || typeof expected !== 'object') {
+  if (typeof actual !== 'object') {
+    return typeof actual === 'number' && Number.isNaN(actual) &&


I think typeof actual === 'number' can be removed, Number.isNaN(actual) is sufficient.

That is correct but it is faster for all other types to check for number first and if it is a number it is not a real penalty because that check is super fast.

Does it makes sense to do the same for expected then?

We could but I feel like that is overdoing it as it would only apply for the case where Number.isNaN(actual), especially as I would expect expected to also be NaN in that case more often then not being of type number.

De-escalating to discuss

BridgeAR · 2017-08-27T03:40:08Z

This will actually also fix a issue in a N-API test case that expects NaN to be equal to each other and they tested it by using deepStrictEqual (the error will only show up in #15050).

AssertionError [ERR_ASSERTION]: [Number: NaN] deepStrictEqual [Number: NaN]
    at Object.<anonymous> (/node/test/addons-napi/test_conversions/test.js:119:8)

I personally think this could be counted as a bug fix instead of a semver-major because I highly doubt that anyone relies on NaN !== NaN in a deepStrictEqual call. Especially as most people are not even aware that that is the case.
@jasnell @Trott what do you think about that?

Trott · 2017-08-27T13:44:36Z

I highly doubt that anyone relies on

Changing this behavior could cause tests to succeed erroneously where two calculations are expected to come up with the same number but a bug in the code causes them both to come up with NaN instead. The test may even include a guard using typeof but that will still turn up number.

I'd prefer it be treated as semver-major out of caution but it's a mild preference. If others feel more strongly that it shouldn't be treated as a breaking change, I'm OK with that.

refack

We'll need to deprecate deepEqual eventually

refack · 2017-08-27T15:07:01Z

IMHO it's a bug fix of the ad hoc spec (I see the current behavior of assert as a self defining spec) so I tend towards 'semver-major'.
On the other hand @BridgeAR pointed out to me that other 3rd party asset libraries already treat NaN as equal to itself.

tniessen · 2017-08-28T09:43:37Z

But ECMAScript abstract equal is "broken" in this sense ((NaN == NaN) === false) and strict equal is broken ((NaN === NaN) === false) is the exact same way.

@refack No, it's not. I actually considered implementing assert support for NaN a couple of weeks ago, but came across this part of the famous IEEE-754 specs:

Four mutually exclusive relations are possible: less than, equal, greater than, and unordered. The last arises when at least one operand is NaN. Every NaN shall compare unordered with everything, including itself. Comparisons shall ignore the sign of zero (so +0 = −0). Infinite operands of the same sign shall compare equal.

tniessen · 2017-08-28T09:40:18Z

doc/api/assert.md

-  ( `===` ). Set values and Map keys are compared using the [SameValueZero][]
-  comparison. (Which means they are free of the [caveats][]).
+1. Primitive values besides NaN are compared using the [Strict Equality
+   Comparison][] ( `===` ). Set and Map values, Map keys and NaN are compared


We usually use monospacing for constants such as NaN in the docs.

refack · 2017-08-28T13:13:30Z

@refack No, it's not. I actually considered implementing assert support for NaN a couple of weeks ago, but came across this part of the famous IEEE-754 specs:

Four mutually exclusive relations are possible: less than, equal, greater than, and unordered. The last arises when at least one operand is NaN. Every NaN shall compare unordered with everything, including itself. Comparisons shall ignore the sign of zero (so +0 = −0). Infinite operands of the same sign shall compare equal.

@BridgeAR pointed me to that spec as well. IMHO that spec should not have been implemented verbatim in JS, but that's done (BTW: If I read this correctly then (NaN != NaN) should be false as well)

My point was that if (NaN == NaN) ≡ (NaN === NaN) then we should make deepEqual(NaN, NaN) ≡ deepStrictEqual(NaN, NaN), but @BridgeAR convinced me that deepEqual is not worth our hassle.

BridgeAR · 2017-08-29T02:27:51Z

@Trott

Changing this behavior could cause tests to succeed erroneously where two calculations are expected to come up with the same number but a bug in the code causes them both to come up with NaN instead.

But in that case the test should not have existed yet as it would have otherwise failed and the bug got hopefully fixed? So it is only something for the future, right?

BridgeAR · 2017-08-29T03:26:29Z

I rebased due to conflicts and addressed the monospacing.

BridgeAR · 2017-08-29T03:46:59Z

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/960/

Trott · 2017-08-31T06:34:31Z

But in that case the test should not have existed yet as it would have otherwise failed and the bug got hopefully fixed? So it is only something for the future, right?

I'm thinking of regressions where a bug is introduced while refactoring or something like that. Might be unlikely as you'd need to mess up two calculations that way simultaneously, I suppose.

BridgeAR · 2017-09-01T05:46:15Z

Rebased due to conflicts. I also fixed a documentation bug from a former commit where the PR number was wrong.

@Trott I am not sure I can follow. If I read your comment correct you would rather keep it as semver-major, right?

Trott · 2017-09-02T21:06:45Z

I am not sure I can follow

@BridgeAR I'd prefer it be treated as semver-major out of caution but it's a very mild preference. If no one else agrees, I'm fine with my mild preference being disregarded. :-D

mcollina

LGTM as semver-major

BridgeAR · 2017-09-05T11:20:56Z

Landed in 1789dcf and ea2e636.

PR-URL: #15036 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Comparing NaN will not throw anymore. PR-URL: #15036 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: nodejs/node#15036 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Comparing NaN will not throw anymore. PR-URL: nodejs/node#15036 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Aug 25, 2017

BridgeAR added semver-major PRs that contain breaking changes and should be released in the next major version. and removed assert Issues and PRs related to the assert subsystem. labels Aug 25, 2017

Trott reviewed Aug 25, 2017

View reviewed changes

vsemozhetbyt added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Aug 25, 2017

Trott reviewed Aug 25, 2017

View reviewed changes

Trott approved these changes Aug 25, 2017

View reviewed changes

jasnell approved these changes Aug 25, 2017

View reviewed changes

vsemozhetbyt reviewed Aug 25, 2017

View reviewed changes

refack previously requested changes Aug 26, 2017

View reviewed changes

lpinca reviewed Aug 26, 2017

View reviewed changes

BridgeAR mentioned this pull request Aug 27, 2017

assert: fix boxed primitives in deep*Equal #15050

Closed

4 tasks

refack approved these changes Aug 27, 2017

View reviewed changes

daxlab approved these changes Aug 28, 2017

View reviewed changes

tniessen reviewed Aug 28, 2017

View reviewed changes

BridgeAR force-pushed the assert-nan branch from a2aab62 to f8d78a6 Compare August 29, 2017 03:24

BridgeAR added 2 commits September 1, 2017 02:43

doc: add missing changelogs to assert docs

0bf1af3

assert: use Same-value eql. for deepStrictEqual NaN

07e36df

BridgeAR force-pushed the assert-nan branch from f8d78a6 to 07e36df Compare September 1, 2017 05:44

mcollina approved these changes Sep 4, 2017

View reviewed changes

BridgeAR closed this Sep 5, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

bradennapier mentioned this pull request Nov 3, 2017

[REQUEST]: Document Breaking Changes in 9.0 #16713

Closed

BridgeAR deleted the assert-nan branch April 1, 2019 23:37

adamlaska mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities adamlaska/node#67

Open

snyk-bot mentioned this pull request Apr 27, 2023

[Snyk] Fix for 20 vulnerabilities aliscco/alisco-node#253

Open

This was referenced Apr 28, 2023

[Snyk] Fix for 16 vulnerabilities aliscco/alisco-node#304

Open

[Snyk] Fix for 18 vulnerabilities aliscco/alisco-node#310

Open

snyk-bot mentioned this pull request Apr 28, 2023

[Snyk] Fix for 20 vulnerabilities aliscco/alisco-node#314

Open

This was referenced Apr 28, 2023

[Snyk] Fix for 9 vulnerabilities aliscco/alisco-node#330

Open

[Snyk] Fix for 19 vulnerabilities aliscco/alisco-node#334

Open

snyk-bot mentioned this pull request Apr 28, 2023

[Snyk] Fix for 114 vulnerabilities aliscco/alisco-node#338

Open

aliscco mentioned this pull request Apr 28, 2023

[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#342

Open

assert: use same value equal for deepStrictEqual NaN #15036

assert: use same value equal for deepStrictEqual NaN #15036

Conversation

BridgeAR commented Aug 25, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 25, 2017

BridgeAR commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 27, 2017 • edited Loading

Trott commented Aug 27, 2017

refack left a comment • edited Loading

Choose a reason for hiding this comment

refack commented Aug 27, 2017

tniessen commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Aug 28, 2017

BridgeAR commented Aug 29, 2017

BridgeAR commented Aug 29, 2017

BridgeAR commented Aug 29, 2017

Trott commented Aug 31, 2017

BridgeAR commented Sep 1, 2017

Trott commented Sep 2, 2017

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 5, 2017

Trott left a comment •

edited

Loading

refack Aug 26, 2017 •

edited

Loading

refack Aug 26, 2017 •

edited

Loading

BridgeAR commented Aug 27, 2017 •

edited

Loading

refack left a comment •

edited

Loading