assert: handle enum. symbol keys in deepStrictEqual#15169
assert: handle enum. symbol keys in deepStrictEqual#15169BridgeAR wants to merge 1 commit intonodejs:masterfrom
Conversation
BridgeAR
added
the
semver-major
nodejs-github-bot
added
the
assert
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 anAssertioneErrorbecause the properties on the [RegExp][] object are not enumerable:
doc/api/assert.md
Outdated
BridgeAR
force-pushed
the
assert-symbols
branch
from
a734b87 to
23f6168
Compare
|
Comments addressed and rebased due to conflicts. |
|
@jasnell It seems like something broke CITGM, would you mind taking a look? |
|
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 |
|
@jasnell check the test names of the failures in e.g. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/964/ |
|
RE: CITGM - npm was broken between #15053 and #15131 |
|
@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/ |
|
@BridgeAR ... can you rebase this? |
|
@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
force-pushed
the
assert-symbols
branch
from
23f6168 to
5ae71ae
Compare
BridgeAR
force-pushed
the
assert-symbols
branch
from
5ae71ae to
ddf110f
Compare
|
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 |
|
Landed in 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>
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>
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>
8 participants
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), orvcbuild test(Windows) passesAffected core subsystem(s)
assert