assert: fix boxed primitives in deep*Equal #15050

BridgeAR · 2017-08-27T03:36:58Z

Unbox all primitives and compare them as well instead of
only comparing boxed strings.

~~This is blocked by #15036 because there is a test case from N-API that currently only passes because deepStrictEqual is not testing them properly and this hides the underlying NaN issue.~~

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

Update: I commented out the failing test for now.

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

kunalspathak · 2017-08-28T21:30:26Z

lib/assert.js

@@ -203,6 +203,11 @@ function strictDeepEqual(actual, expected) {
        Object.keys(expected).length === expected.length) {
      return true;
    }
+  } else if (typeof actual.valueOf === 'function') {
+    const rawA = actual.valueOf();


rawA [](start = 10, length = 4)

nit: can you have better name like rawAcual ?

BridgeAR · 2017-08-29T05:52:33Z

I rebased due to conflicts, addressed the nit and added a fast path for boxed primitives. I also commented out the failing test, so now it is not blocked anymore.

jasnell · 2017-08-29T17:08:38Z

Curious about the semver-iness of this change. Thoughts @nodejs/tsc?

jasnell

The code change LGTM but I'd like a CITGM run and others from @nodejs/tsc to sign off

BridgeAR · 2017-08-30T12:21:46Z

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

BridgeAR · 2017-09-01T05:47:55Z

Rebased due to conflicts.

BridgeAR · 2017-09-02T06:32:51Z

Ping @nodejs/ctc @nodejs/tsc PTAL

mcollina · 2017-09-02T06:44:26Z

test/addons-napi/test_conversions/test.js

@@ -116,7 +116,8 @@ assert.deepStrictEqual(new Boolean(false), test.toObject(false));
 assert.deepStrictEqual(new Boolean(true), test.toObject(true));
 assert.deepStrictEqual(new String(''), test.toObject(''));
 assert.deepStrictEqual(new Number(0), test.toObject(0));
-assert.deepStrictEqual(new Number(Number.NaN), test.toObject(Number.NaN));
+// TODO: Add test back in as soon as NaN is properly compared
+// assert.deepStrictEqual(new Number(Number.NaN), test.toObject(Number.NaN));


Can we remove the todo and add the test back?

Not before #15036 landed (still trying to figure out if that one should be semver-major or a patch though). Otherwise the test would fail.

what's the depedency of this change to #15036?

Currently assert.deepStrictEqual(NaN, NaN) will fail. This test only worked because NaN was never compared as it was not unboxed before. #15036 will change that to not fail anymore in case NaN is compared to itself.

Have you pushed you changes to this PR? Looks like these lines are still commented?

Thanks a lot for reminding me! It is now back in place.

joyeecheung · 2017-09-03T06:44:12Z

lib/assert.js

    // Buffer.compare returns true, so actual.length === expected.length
    // if they both only contain numeric keys, we don't need to exam further
    if (Object.keys(actual).length === actual.length &&
        Object.keys(expected).length === expected.length) {
      return true;
    }
+  } else if (typeof actual.valueOf === 'function') {


IIUC this also affects objects with custom valueOf, so the documentation needs to be updated as well..

You are right! I am going to add that to the documentation.

Trott · 2017-09-05T05:20:29Z

doc/api/assert.md

+assert.deepStrictEqual({}, new Number(1));
+// Fails as the wrapped number is unwrapped and compared as well.
+assert.deepStrictEqual(new String('foo'), Object('foo'));
+// Ok, both objects and strings are unwrapped identical.


Nit: Perhaps change the comment to something like:

OK because the object and the string are identical when unwrapped.

I'm confused by this documentation. It's supposed to document new behavior you're adding, right? But the assertions here are already fail/succeed as described in Node.js 8.4.0 so the behavior isn't because of any changes added here. Am I misunderstanding?

Changed. I indeed chose a bad example for the number and updated that. Currently only wrapped strings are compared but in my former example with the wrapped number, the toString() was not identical and it failed early.

Trott · 2017-09-05T05:20:54Z

doc/api/assert.md

@@ -161,6 +162,11 @@ assert.deepEqual(date, fakeDate);
 assert.deepStrictEqual(date, fakeDate);
 // AssertionError: 2017-03-11T14:25:31.849Z deepStrictEqual Date {}
 // Different type tags
+
+assert.deepStrictEqual({}, new Number(1));
+// Fails as the wrapped number is unwrapped and compared as well.


Micro-nit: Fails as the wrapped number is... -> Fails because the wrapped number is...

Thanks for the suggestion!

Unbox all primitives and compare them as well instead of only comparing boxed strings.

BridgeAR · 2017-09-05T11:27:49Z

Rebased due to conflicts

Trott

Seems good to me. I've refrained from giving at a green check because I worry I may not be grasping all the ramifications.

joyeecheung · 2017-09-10T04:01:54Z

LGTM with the commented test uncommented

BridgeAR · 2017-09-12T20:32:01Z

Landed in 22ae8c0

Unbox all primitives and compare them as well instead of only comparing boxed strings. PR-URL: #15050 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Unbox all primitives and compare them as well instead of only comparing boxed strings. PR-URL: nodejs#15050 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

jasnell · 2017-09-20T15:44:31Z

this would need to be backported for v8.x

Unbox all primitives and compare them as well instead of only comparing boxed strings. PR-URL: nodejs#15050 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Unbox all primitives and compare them as well instead of only comparing boxed strings. PR-URL: #15050 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Aug 27, 2017

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

BridgeAR mentioned this pull request Aug 27, 2017

assert: use same value equal for deepStrictEqual NaN #15036

Closed

4 tasks

kunalspathak reviewed Aug 28, 2017

View reviewed changes

BridgeAR force-pushed the assert-boxed branch from 73f7077 to b2887a7 Compare August 29, 2017 05:51

BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Aug 29, 2017

jasnell approved these changes Aug 29, 2017

View reviewed changes

BridgeAR force-pushed the assert-boxed branch from b2887a7 to fd96e26 Compare September 1, 2017 05:47

mcollina reviewed Sep 2, 2017

View reviewed changes

nodejs deleted a comment Sep 2, 2017

joyeecheung reviewed Sep 3, 2017

View reviewed changes

BridgeAR requested a review from Trott September 4, 2017 12:41

Trott reviewed Sep 5, 2017

View reviewed changes

BridgeAR force-pushed the assert-boxed branch from 320cfae to af780c9 Compare September 5, 2017 11:25

assert: fix boxed primitives in deep*Equal

66432d8

Unbox all primitives and compare them as well instead of only comparing boxed strings.

BridgeAR force-pushed the assert-boxed branch from af780c9 to 66432d8 Compare September 5, 2017 11:26

BridgeAR requested review from Trott and joyeecheung September 9, 2017 02:40

Trott reviewed Sep 9, 2017

View reviewed changes

joyeecheung approved these changes Sep 10, 2017

View reviewed changes

[squash] add testcase back in

7dfd780

BridgeAR closed this Sep 12, 2017

jasnell added the backport-requested-v8.x label Sep 20, 2017

BridgeAR mentioned this pull request Sep 20, 2017

[v8.x backport] assert: fix boxed primitives in deepStrictEqual #15514

Closed

4 tasks

jasnell added backported-to-v8.x and removed backport-requested-v8.x labels Sep 21, 2017

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: fix boxed primitives in deep*Equal #15050

assert: fix boxed primitives in deep*Equal #15050

BridgeAR commented Aug 27, 2017 •

edited

Loading

kunalspathak Aug 28, 2017

BridgeAR commented Aug 29, 2017

jasnell commented Aug 29, 2017

jasnell left a comment

BridgeAR commented Aug 30, 2017

BridgeAR commented Sep 1, 2017

BridgeAR commented Sep 2, 2017

mcollina Sep 2, 2017

BridgeAR Sep 2, 2017 •

edited

Loading

mcollina Sep 4, 2017

BridgeAR Sep 4, 2017

joyeecheung Sep 10, 2017

BridgeAR Sep 11, 2017

joyeecheung Sep 3, 2017

BridgeAR Sep 3, 2017

BridgeAR Sep 4, 2017

Trott Sep 5, 2017

Trott Sep 5, 2017

BridgeAR Sep 5, 2017

Trott Sep 5, 2017

BridgeAR Sep 5, 2017

BridgeAR commented Sep 5, 2017 •

edited

Loading

Trott left a comment

joyeecheung commented Sep 10, 2017

BridgeAR commented Sep 12, 2017 •

edited

Loading

jasnell commented Sep 20, 2017

assert: fix boxed primitives in deep*Equal #15050

assert: fix boxed primitives in deep*Equal #15050

Conversation

BridgeAR commented Aug 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

BridgeAR commented Aug 29, 2017

jasnell commented Aug 29, 2017

jasnell left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 30, 2017

BridgeAR commented Sep 1, 2017

BridgeAR commented Sep 2, 2017

Choose a reason for hiding this comment

BridgeAR Sep 2, 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

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

Choose a reason for hiding this comment

BridgeAR commented Sep 5, 2017 • edited Loading

Trott left a comment

Choose a reason for hiding this comment

joyeecheung commented Sep 10, 2017

BridgeAR commented Sep 12, 2017 • edited Loading

jasnell commented Sep 20, 2017

BridgeAR commented Aug 27, 2017 •

edited

Loading

BridgeAR Sep 2, 2017 •

edited

Loading

BridgeAR commented Sep 5, 2017 •

edited

Loading

BridgeAR commented Sep 12, 2017 •

edited

Loading