FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

assert: handle enum. symbol keys in deepStrictEqual by BridgeAR · Pull Request #15169 · nodejs/node · GitHub

/ node Public

assert: handle enum. symbol keys in deepStrictEqual - #15169

Closed
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:assert-symbols
ConversationCommits (1)ChecksFiles changed
Closed

assert: handle enum. symbol keys in deepStrictEqual#15169
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:assert-symbols

Conversation

BridgeAR commented Sep 4, 2017

Copy link
Copy Markdown
Member

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 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 4, 2017
BridgeAR requested a review from Trott September 4, 2017 00:31
nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Sep 4, 2017

BridgeAR commented Sep 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

Trott approved these changes Sep 5, 2017
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread doc/api/assert.md Outdated

Trott Sep 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread doc/api/assert.md Outdated

Trott Sep 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

Comment thread doc/api/assert.md Outdated

Trott Sep 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nano-nit: Remove as well?

BridgeAR commented Sep 5, 2017

Copy link
Copy Markdown
Member Author

Comments addressed and rebased due to conflicts.

jasnell commented Sep 7, 2017

Copy link
Copy Markdown
Member

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

BridgeAR commented Sep 7, 2017

Copy link
Copy Markdown
Member Author

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

jasnell commented Sep 7, 2017

Copy link
Copy Markdown
Member

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 commented Sep 8, 2017

Copy link
Copy Markdown
Member Author

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

refack commented Sep 8, 2017

Copy link
Copy Markdown
Contributor

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

BridgeAR commented Sep 9, 2017

Copy link
Copy Markdown
Member Author

@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 commented Sep 15, 2017

Copy link
Copy Markdown
Member

@BridgeAR ... can you rebase this?

BridgeAR commented Sep 16, 2017

Copy link
Copy Markdown
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 commented Sep 16, 2017

Copy link
Copy Markdown
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 commented Sep 19, 2017

Copy link
Copy Markdown
Member Author

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

BridgeAR commented Sep 19, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Landed in db2e093

BridgeAR closed this Sep 19, 2017
BridgeAR added a commit that referenced this pull request Sep 19, 2017
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
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
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 mentioned this pull request Oct 30, 2017
Merged
bradennapier mentioned this pull request Nov 3, 2017
Closed
BridgeAR deleted the assert-symbols branch April 1, 2019 23:37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
tniessen tniessen approved these changes
+1 more reviewer
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL