-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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.
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
assert: fix _deepEqual and improve assert.md #11128
Conversation
bbbf4a6
to
0d1c88d
Compare
Benchmarks results (benchmarks taken from #11092). Note that this implementation favors fast failures, though failures are not benchmarked(when an assertion fails the process should probably be terminated anyway). see resultsimprovement confidence p.value assert/deepequal-buffer.js method="nonstrict" len=100 n=1000 -92.69 % *** 1.398352e-10 assert/deepequal-buffer.js method="strict" len=100 n=1000 -87.27 % *** 8.988908e-07 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="array" 0.52 % 5.671380e-01 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="boolean" 1.33 % 3.162478e-01 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="new-array" 1.39 % * 2.256608e-02 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="null" 1.12 % 1.546353e-01 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="number" -0.52 % 7.352546e-01 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="object" -0.53 % 5.363293e-01 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="string" 0.48 % 5.514581e-01 assert/deepequal-prims-and-objs-big-array.js method="nonstrict" len=100000 n=25 prim="undefined" 0.70 % 2.601707e-01 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="array" 0.59 % 4.341103e-01 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="boolean" 1.73 % ** 9.211000e-03 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="new-array" 1.30 % 1.185455e-01 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="null" 0.68 % 3.269869e-01 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="number" -2.51 % 5.382094e-02 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="object" 2.23 % 8.626440e-02 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="string" 0.20 % 7.698036e-01 assert/deepequal-prims-and-objs-big-array.js method="strict" len=100000 n=25 prim="undefined" 0.86 % 3.789055e-01 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="array" 6.32 % *** 4.516185e-04 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="boolean" 9.25 % *** 8.115494e-05 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="new-array" 9.36 % *** 7.678773e-05 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="null" 9.41 % *** 2.294144e-04 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="number" 5.07 % * 3.376728e-02 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="object" 4.23 % 1.091126e-01 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="string" 6.42 % *** 5.440464e-04 assert/deepequal-prims-and-objs-big-loop.js method="nonstrict" n=1000000 prim="undefined" 8.15 % ** 4.303777e-03 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="array" 6.60 % * 1.017124e-02 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="boolean" 8.82 % *** 6.257818e-05 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="new-array" 11.13 % *** 6.845735e-04 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="null" 8.88 % *** 1.083174e-04 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="number" 4.29 % 2.310841e-01 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="object" 7.70 % *** 2.296652e-04 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="string" 9.81 % *** 1.067824e-06 assert/deepequal-prims-and-objs-big-loop.js method="strict" n=1000000 prim="undefined" 13.05 % *** 1.293929e-05 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Float32Array" 0.45 % 6.665986e-01 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Float64Array" 1.62 % 1.182916e-01 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Int16Array" -98.85 % *** 6.838162e-11 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Int32Array" -98.02 % *** 1.541802e-11 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Int8Array" -99.33 % *** 1.169792e-12 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Uint16Array" -98.88 % *** 1.836848e-11 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Uint32Array" -97.99 % *** 7.248263e-13 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Uint8Array" -99.32 % *** 3.145163e-11 assert/deepequal-typedarrays.js len=1000000 method="nonstrict" n=1 type="Uint8ClampedArray" -99.30 % *** 5.984442e-12 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Float32Array" 0.78 % 4.221460e-01 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Float64Array" -0.08 % 9.232714e-01 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Int16Array" -6.42 % 7.496064e-02 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Int32Array" -0.20 % 9.728131e-01 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Int8Array" -5.38 % 5.518969e-01 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Uint16Array" -6.58 % 2.480984e-01 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Uint32Array" -4.62 % 1.772045e-01 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Uint8Array" -10.72 % *** 1.283864e-04 assert/deepequal-typedarrays.js len=1000000 method="strict" n=1 type="Uint8ClampedArray" -0.45 % 8.987984e-01 |
0d1c88d
to
500da43
Compare
Hmm, canary died but I can't be sure if the failures are related(most are in network-related modules, and node-sass has an error about platform support). @MylesBorins Am I using it properly? |
doc/api/assert.md
Outdated
|
||
Only [enumerable "own" properties](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties) | ||
are considered. The `deepEqual()` implementation does not test the | ||
[`[[Prototype]]`](https://tc39.github.io/ecma262/#sec-ordinary-object-internal-methods-and-internal-slots) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the link ref to the end of the file and use an inline ref like:
[`[[Prototype]]`][]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this but seems GitHub has a little bit trouble with that(I am guessing it's the [[
and ]]
somehow confuses its parser?), not sure if the tooling can deal with it?
doc/api/assert.md
Outdated
Only [enumerable "own" properties](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties) | ||
are considered. The `deepEqual()` implementation does not test the | ||
[`[[Prototype]]`](https://tc39.github.io/ecma262/#sec-ordinary-object-internal-methods-and-internal-slots) | ||
of objects, attached symbols, or non-enumerable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is rather short, can you update this so that the line length is closer to 80
doc/api/assert.md
Outdated
|
||
For the following cases, consider using ES2015 [`Object.is()`][], | ||
which uses the | ||
[SameValueZero](https://tc39.github.io/ecma262/#sec-samevaluezero) comparison. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to a reference at the bottom of the file also. :-)
test/parallel/test-assert-deep.js
Outdated
result += str.replace(/[\\^$.*+?()[\]{}|=!<>:-]/g, '\\$&'); | ||
result += literals[i + 1]; | ||
} | ||
return new RegExp('^AssertionError: ' + result + '$'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return new RegExp(`^AssertionError: ${result}$`);
500da43
to
d6bddd6
Compare
@jasnell Thanks, PTAL. I am not quite sure about what to do the |
Escape the
For instance: [[Prototype]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as on #10282 - it'd be great to cache everything at module level rather than trusting user-provided objects, or user code, not to have broken something.
lib/assert.js
Outdated
} | ||
|
||
function isArguments(tag) { | ||
return tag == '[object Arguments]'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
===
lib/assert.js
Outdated
// for a list of tags pre-defined in the spec. | ||
// There are some unspecified tags in the wild too (e.g. typed array tags). | ||
// Since tags can be altered, they only serve fast failures | ||
const actualTag = Object.prototype.toString.call(actual); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as I made in #10282 - Object.prototype.toString
should be cached at module level.
(I think the discussion about @ljharb 's comments can be moved here since those commented code is intended to be merged by this PR) I am +1-ish on caching In the meantime I'll change |
1a83174
to
bbc95aa
Compare
Rebased and, ah...right, I was being silly, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good but doc needs a few tweaks.
doc/api/assert.md
Outdated
Primitive values are compared with the [Abstract Equality Comparison][] | ||
( `==` ). | ||
|
||
Only [enumerable "own" properties](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the link to the bottom and use an inline reference like [enumerable "own" properties][]
to avoid having the over-long line
doc/api/assert.md
Outdated
[`[[Prototype]]`][prototype-spec] of objects, attached symbols, or | ||
non-enumerable properties (for those checks, consider using | ||
[`assert.deepStrictEqual()`][] instead). This can lead to some | ||
potentially surprising results. For example, the following example does no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/does no/does not (end of the line)
doc/api/assert.md
Outdated
|
||
For the following cases, consider using ES2015 [`Object.is()`][], | ||
which uses the | ||
[SameValueZero][] comparison. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this up to the previous line :-)
doc/api/assert.md
Outdated
``` | ||
|
||
For more information, see | ||
[MDN's guide on equality comparisons and sameness](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even tho this is at the end of the file, please move the link to the index at the end and use an inline reference for this as well, just for consistency :-)
bbc95aa
to
c541024
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor doc nit, but otherwise LGTM if CI is green and a CITGM run doesn't reveal anything alarming.
Punctuation fixed, thanks! @Trott |
Recent CITGM jobs are all red, launched https://ci.nodejs.org/job/citgm-smoker/582/ against master and https://ci.nodejs.org/job/citgm-smoker/583/ against this PR for comparison. |
doc/api/assert.md
Outdated
because the properties on the [`Error`][] object are non-enumerable: | ||
Only [enumerable "own" properties][] are considered. The `deepEqual()` | ||
implementation does not test the [`[[Prototype]]`][prototype-spec] of | ||
objects, attached symbols, or non-enumerable properties. (For those checks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just spotted this... can you actually remove the parens in this sentence. It can be reworded slightly as:
Only [enumerable "own" properties][] are considered. The `deepEqual()`
implementation does not test the [`[[Prototype]]`][prototype-spec] of objects,
attached symbols, or non-enumerable properties — for such checks, consider
using [assert.deepStrictEqual()][] instead.
Also, using deepEqual()
in one sentence and assert.deepStrictEqual()
in another is a bit inconsistent. Consider either using the assert.
in both or omitting it from both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I was thinking parens look a bit awkward too, the hyphen does the trick!
I'll use assert.deepEqual()
because it has a link already. Thanks!
doc/api/assert.md
Outdated
[Abstract Equality Comparison]: https://tc39.github.io/ecma262/#sec-abstract-equality-comparison | ||
[Strict Equality Comparison]: https://tc39.github.io/ecma262/#sec-strict-equality-comparison | ||
[`Object.is()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is | ||
[SameValueZero]: https://tc39.github.io/ecma262/#sec-SameValueZero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: #sec-samevaluezero
doc/api/assert.md
Outdated
For more information, see | ||
[MDN's guide on equality comparisons and sameness][mdn-equality-guide]. | ||
|
||
[Locked]: documentation.html#documentation_stability_index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this link is unused
@targos Thanks for the review, updated. New CI: https://ci.nodejs.org/job/node-test-pull-request/6492/ The CITGM for the last code change has the same amount of failure as master ( |
Hmm..don't know what else does this PR need to land? CITGM is |
@joyeecheung I don’t think there’s anything speaking against landing it. :) |
@addaleax Alright, gonna land this tomorrow :) Thanks |
FYI, needs a rebase thanks to a PR I just landed. |
lib/assert.js
Outdated
|
||
function objEquiv(a, b, strict, actualVisitedObjects) { | ||
if (a === null || a === undefined || b === null || b === undefined) | ||
// GH-7178. Ensure reflexivity of deepEqual with `arguments` objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a good time to change that GH reference to a full URL. Generally a good idea, but especially here since it's not in this repository, but in the archive one instead:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'll update those links, thanks!
test/parallel/test-assert-deep.js
Outdated
new Int32Array([1]), // Int32Array | ||
new Uint32Array([1]), // Uint32Array | ||
Buffer.from([1]), | ||
// Arguments {'0': '1'} is not here, see GH-7178 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, full URL would be helpful: nodejs/node-v0.x-archive#7178
26e4805
to
5fd9803
Compare
Refactors _deepEqual and fixes a few code paths that lead to behaviors contradicting what the doc says. Before this commit certain types of objects (Buffers, Dates, etc.) are not checked properly, and can get away with different prototypes AND different enumerable owned properties because _deepEqual would jump to premature conclusion for them. Since we no longer follow CommonJS unit testing spec, the checks for primitives and object prototypes are moved forward for faster failure. Improve regexp and float* array checks: * Don't compare lastIndex of regexps, because they are not enumerable, so according to the docs they should not be compared * Compare flags of regexps instead of separate properties * Use built-in tags to test for float* arrays instead of using instanceof Use full link to the archived GitHub repository. Use util.objectToString for future improvements to that function that makes sure the call won't be tampered with. Refs: nodejs#10282 (comment) Refs: nodejs#10258 (comment)
Use the term "Abstract Equality Comparison" and "Strict Equality Comparison" from ECMAScript specification to refer to the operations done by `==` and `===`, instead of "equal comparison operator" and "strict equality operator". Clarify that deep strict comparisons checks `[[Prototype]]` property, instead of the vague "object prototypes". Suggest using `Object.is()` to avoid the caveats of +0, -0 and NaN. Add a MDN link explaining what enumerable "own" properties are.
Rebased and squashed with full github links added. New CI: https://ci.nodejs.org/job/node-test-pull-request/6583/ |
@nodejs/citgm do these failures look reasonable? |
This is weird, the CITGM against this PR is |
Not sure why CITGM got less errors with this PR, but according previous results I am pretty sure it's not related to the code change here. I am going to land this tomorrow if no more concerns arise... |
CITGM and CI look fine. I've seen flakiness like that before. |
Refactors _deepEqual and fixes a few code paths that lead to behaviors contradicting what the doc says. Before this commit certain types of objects (Buffers, Dates, etc.) are not checked properly, and can get away with different prototypes AND different enumerable owned properties because _deepEqual would jump to premature conclusion for them. Since we no longer follow CommonJS unit testing spec, the checks for primitives and object prototypes are moved forward for faster failure. Improve regexp and float* array checks: * Don't compare lastIndex of regexps, because they are not enumerable, so according to the docs they should not be compared * Compare flags of regexps instead of separate properties * Use built-in tags to test for float* arrays instead of using instanceof Use full link to the archived GitHub repository. Use util.objectToString for future improvements to that function that makes sure the call won't be tampered with. PR-URL: #11128 Refs: #10282 (comment) Refs: #10258 (comment) Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Use the term "Abstract Equality Comparison" and "Strict Equality Comparison" from ECMAScript specification to refer to the operations done by `==` and `===`, instead of "equal comparison operator" and "strict equality operator". Clarify that deep strict comparisons checks `[[Prototype]]` property, instead of the vague "object prototypes". Suggest using `Object.is()` to avoid the caveats of +0, -0 and NaN. Add a MDN link explaining what enumerable "own" properties are. PR-URL: #11128 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
This is the more semver-patch part of #10282.
Fix _deepEqual
Refactors _deepEqual and fixes a few code paths that lead to
behaviors contradicting what the doc says. Before this commit
certain types of objects (Buffers, Dates, etc.) are not checked
properly, and can get away with different prototypes AND different
enumerable owned properties because _deepEqual would jump to
premature conclusion for them.
Since we no longer follow CommonJS unit testing spec,
the checks for primitives and object prototypes are moved
forward for faster failure.
Improve regexp and float* array checks:
enumerable, so according to the docs they should not be compared
instanceof
Refs: #10282 (comment)
Refs: #10258 (comment)
Improve documentation
Use the term "Abstract Equality Comparison" and "Strict Equality
Comparison" from ECMAScript specification to refer to the operations
done by
==
and===
, instead of the informal"equal comparison operator" and "strict equality operator".
Clarify that deep strict comparisons checks
[[Prototype]]
property, instead of the vague "object prototypes".
Suggest using
Object.is()
to avoid the caveats of +0, -0 and NaN.Add a MDN link explaining what enumerable "own" properties are.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test, assert, doc