Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

assert: handle enum. symbol keys in deepStrictEqual #15169

Closed
wants to merge 1 commit into from
Closed

assert: handle enum. symbol keys in deepStrictEqual #15169

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 4, 2017

Adding support for enumerable symbol keys is consequent as we do compare symbols as primitives in deepEqual but we do not check them as keys.

I think this is important as I think we should try to prevent using enumerable symbols as private keys in our code and instead only use non-enumerable ones.

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

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 4, 2017
@BridgeAR BridgeAR requested a review from Trott September 4, 2017 00:31
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Sep 4, 2017
@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 4, 2017 •
edited
Loading

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

Trott
Trott approved these changes Sep 5, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI and CITGM don't reveal any surprises. Left some nits on the docs but they're tiny suggestions that can be ignored if anyone disputes any of them.

doc/api/assert.md Outdated
throw an `AssertionError` because the properties on the [`RegExp`][] object are
not enumerable:
[`[[Prototype]]`][prototype-spec] of objects or enumerable own [`Symbol`][]
properties — for such checks, consider using [`assert.deepStrictEqual()`][]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-nit: Get rid of the — and make these two simple sentences instead:

Only [enumerable "own" properties][] are considered. The [assert.deepEqual()][] implementation does not test the [[[Prototype]]][prototype-spec] of objects or enumerable own [Symbol][] properties. For such checks, consider using [assert.deepStrictEqual()][] instead.

doc/api/assert.md Outdated
not enumerable:
[`[[Prototype]]`][prototype-spec] of objects or enumerable own [`Symbol`][]
properties — for such checks, consider using [`assert.deepStrictEqual()`][]
instead. This can lead to some potentially surprising results. For example, the
Copy link
Member

@Trott Trott Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: New paragraph starting with This can lead…. Maybe also replace This. And get rid of For example as the next few words are the following example. All told, maybe this?:

assert.deepEqual() can have potentially surprising results. The following example does not throw an AssertioneError because the properties on the [RegExp][] object are not enumerable:

doc/api/assert.md Outdated
@@ -105,6 +104,9 @@ parameter is undefined, a default error message is assigned.
added: v1.2.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/XXXXX
description: Enumerable symbols are now compared as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nano-nit: Remove as well?

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 5, 2017

Comments addressed and rebased due to conflicts.

@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9974/

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 7, 2017

@jasnell It seems like something broke CITGM, would you mind taking a look?

@jasnell
Copy link
Member

jasnell commented Sep 7, 2017

broke in what way? I'm on an extremely slow in flight wifi for the next 9 hours so it may be better to have someone else take a look. @nodejs/citgm

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 8, 2017

@jasnell check the test names of the failures in e.g. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/964/

@refack
Copy link
Contributor

refack commented Sep 8, 2017

RE: CITGM - npm was broken between #15053 and #15131
Rerun: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/966/

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 9, 2017

@refack urgs, you are right. Your CITGM fails as well though because you did not rebase to master^^

Here is a build rebased on master https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/967/

@jasnell
Copy link
Member

jasnell commented Sep 15, 2017

@BridgeAR ... can you rebase this?

@BridgeAR
Copy link
Member Author

@jasnell I will look into this soon. I missed a fast path and I want to think about how to check that in a nice way. That is the reason I did not yet land it^^.

@BridgeAR
Copy link
Member Author

I rebased this and I refactored the code to make sure the fast paths are also checking for the symbols. This required some more changes than before.

So PTAL

@BridgeAR
Copy link
Member Author

CI https://ci.nodejs.org/job/node-test-pull-request/10139/

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 19, 2017 •
edited
Loading

Landed in db2e093

@BridgeAR BridgeAR closed this Sep 19, 2017
BridgeAR added a commit that referenced this pull request Sep 19, 2017
@BridgeAR
assert: handle enumerable symbol keys
db2e093 
PR-URL: #15169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
assert: handle enumerable symbol keys
e69efbe 
PR-URL: nodejs/node#15169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
assert: handle enumerable symbol keys
0cd8abd 
PR-URL: nodejs/node#15169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@bradennapier bradennapier mentioned this pull request Nov 3, 2017
Closed
@BridgeAR BridgeAR deleted the assert-symbols branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@BridgeAR @jasnell @refack @Trott @addaleax @lpinca @tniessen @nodejs-github-bot