assert: fix boxed primitives in deep*Equal by BridgeAR · Pull Request #15050 · nodejs/node · GitHub

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

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

nit: can you have better name like rawAcual ?

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.

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

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

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

Rebased due to conflicts.

Ping @nodejs/ctc @nodejs/tsc PTAL

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.

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.

Addressed

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.

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.

Rebased due to conflicts

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

LGTM with the commented test uncommented

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 <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Unbox all primitives and compare them as well instead of only comparing boxed strings. PR-URL: nodejs#15050 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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 <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Unbox all primitives and compare them as well instead of only comparing boxed strings. PR-URL: #15050 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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

assert: fix boxed primitives in deep*Equal by BridgeAR · Pull Request #15050 · nodejs/node · GitHub

Uh oh!

Uh oh!

assert: fix boxed primitives in deep*Equal - #15050

assert: fix boxed primitives in deep*Equal#15050BridgeAR wants to merge 2 commits intonodejs:masternodejs/node:masterfrom BridgeAR:assert-boxedBridgeAR/node:assert-boxedCopy head branch name to clipboard

Conversation

BridgeAR commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 29, 2017

Uh oh!

jasnell commented Aug 29, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 30, 2017

Uh oh!

BridgeAR commented Sep 1, 2017

Uh oh!

BridgeAR commented Sep 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Sep 10, 2017

Uh oh!

BridgeAR commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

assert: fix boxed primitives in deep*Equal#15050
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:assert-boxed

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