assert: enforce type check in deepStrictEqual by joyeecheung · Pull Request #10282 · nodejs/node · GitHub

For a write-up on this PR, see #10282 (comment)

The less-breaking part of this PR has been moved to #11128

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

assert, test

Description of change

Add checks for the built-in tags and refactor _deepEqual,
most user-land modules do this for a strict, deep comparison.
nodejs/node-v0.x-archive#7178 already added
this check for argument objects, even in non-strict tests.

Fixes #10258.
Docs updates are postponed due to #7815.

See https://tc39.github.io/ecma262/#sec-object.prototype.tostring
for a partial list of the tags.

I’ve labelled this semver-major since it looks like a pretty clear behavioural change to me, even if it seems to make a lot of sense from the first look at it.

Fixes #10258.

Could you add a Fixes: https://github.com/nodejs/node/issues/10258 line at the end of the commit message? :)

So, this is not related to the changes you are making here, but I would have a couple of suggestions:

I guess you could probably make arguments both for including lastIndex here and for leaving it out, and I’m tempted to say “drop it”. I probably wouldn’t expect this behaviour.
Maybe instead of the global, multiline and ignoreCase checks a single check for .flags would be a good idea? There are new flags like u that would currently be ignored.

(If you think these suggestions make sense, it’s probably best to keep that for a separate commit/separate PR.)

Good suggestion, I think I will just submit another commit in this PR.

The indentation seems off here?

Also, could you update the instanceof checks here to use getTag, too? (probably also best kept for a separate commit or PR)

Oops, why my make lint doesn't scream at me about this :/? I will update these indentations, thanks. And the tag checks can go into another commit I think.

nit: Can I suggest that you update this to use the full URL here?

Is the eslint-disable tag to that deepEqual can be used here?

no-restricted-properties is the one I get from make lint, I can't find a specific eslint-disable tag for deepEqual, do you know what that is?

@joyeecheung Nope, and to be clear, I wasn’t requesting you to change anything here, just making sure that I understood why the line is there in the first place

Oh yes, this disable tag is for all the deepEquals below :).

I think it's fine the way you did it, but if you want to get even more granular with the linting options, you can disable the check on just the lines that need it by either using // eslint-disable-line no-restricted-properties or // eslint-disable-next-line no-restricted-properties (depending on if you can fit the comment on the same line or if it needs to go on the line right before the assert.deepEqual()).

Thank you for the tips!

The indentation is off here, and there’s a assert.throws pitfall: You can’t specify a string to check the error message against; only a constructor, RegExp or function that checks the argument work here. (You can probably just wrap the expression in new RegExp or something)

(edit: link to docs: https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message)

Sorry for not reading the docs more thoroughly. I'll wrap them up in new RegExp, thanks!

Sorry for not reading the docs more thoroughly.

@joyeecheung If it helps… that mistake is something found a lot in our own tests, at least until very recently ;)

typos: returns and checks (here and below)

@addaleax Thanks for the review, I've updated this PR, PTAL.

Thanks! This looks pretty good so far, I’ll have time for a full review of this later.

CI: https://ci.nodejs.org/job/node-test-commit/6668/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/481/

Thanks for doing this! Mostly seems good to me. Not sure if we'd have to "unlock" the API, but (as of right now, at least) I'd be in favor of doing that. Some small comments.

These numbers (7.2) correspond to the sections of a specification that assert followed in its initial implementation. http://wiki.commonjs.org/wiki/Unit_Testing/1.0

If we're going to change the algorithm, I'd recommend removing the numbers, since we're not following the spec anymore.

Didn't know this is a thing :O I will remove those numbers. Thanks.

Instead of removing this test, would it make sense to change it to assert.doesNotThrow?

Good suggestion. I will update these. Thanks.

Same here: assert.doesNotThrow()?

@Trott Thank you for the review, I have updated this PR, PTAL.

@nodejs/ctc Would we need to "unlock" the assert module to land this? I think the answer is: "technically yes" but there might be some wiggle room on something like this?

Added the blocked label until the "Locked API" issue is sorted out.

LGTM if CI is ✅ and CITGM is ✨

LGTM. Maybe read me adding the “semver-major” label more as a precaution than anything else; I am fine considering this a bugfix.

Extend the assert-throws-arguments custom ESLint rule to also check for the use of template literals as a second argument to assert.throws. PR-URL: nodejs#10301 Ref: nodejs#10282 (comment) Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Italo A. Casas <me@italoacasas.com>

Extend the assert-throws-arguments custom ESLint rule to also check for the use of template literals as a second argument to assert.throws. PR-URL: #10301 Ref: #10282 (comment) Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Italo A. Casas <me@italoacasas.com>

I've left a few comments, but in general, it'd be awesome if new code in node core was robust against user modification of the language builtins.

why is this == and a few lines above, ===? I assume this should be ===?

Oops, it's mistake, thanks for catching that.

For the sake of robustness, please cache Object.prototype.toString at module level, and don't rely on nobody having monkeyed with Object.prototype at runtime :-)

It's safer to cache Date.prototype.getTime at module level, and .call it here - rather than relying both on .getTime not being shadowed on these two objects, and on Date.prototype being unbroken.

Similarly, Object.keys should be cached at module level, not runtime-looked-up on Object

This is blocked by #11128

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)

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 <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Added the blocked label until the "Locked API" issue is sorted out.

assert was unlocked in #11304, removing blocked label.

This is blocked by #11128

That one landed too.

@ChALkeR Yeah this is not blocked anymore. Thanks!

Did a rebase locally, haven't decided about #10282 (comment), I'll push it later.

Rebased with documentation and a few more test cases (for faked global and process)

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: nodejs#10258

Still LGTM :)

I can’t find the definition of [Object.prototype.toString()] … am I missing something?

Uh, forgot to put the reference link below :P

typo: own properties?

@addaleax Thanks for the review, fixed the typo and the missing link.

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

The last CITGM run is also a bit old, so:

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

Went ahead and landed this in efec14a :)

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: #10258 PR-URL: #10282 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: nodejs#10258 PR-URL: nodejs#10282 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. lts-watch-v6.x labels Dec 15, 2016

joyeecheung force-pushed the fix-assert branch from ecdd024 to d065c7c Compare December 15, 2016 09:00

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed lts-watch-v6.x labels Dec 15, 2016

addaleax reviewed Dec 15, 2016

View reviewed changes

joyeecheung force-pushed the fix-assert branch from d065c7c to 89cdd8f Compare December 15, 2016 16:29

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

joyeecheung force-pushed the fix-assert branch from 89cdd8f to df77ce1 Compare December 15, 2016 16:32

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

joyeecheung force-pushed the fix-assert branch from df77ce1 to 5cdfc0e Compare December 15, 2016 16:34

nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016

addaleax removed the lts-watch-v6.x label Dec 15, 2016

addaleax requested a review from Trott December 15, 2016 22:01

Trott requested changes Dec 16, 2016

View reviewed changes

targos mentioned this pull request Dec 16, 2016

tools: forbid template literals in assert.throws #10301

Merged

4 tasks

nodejs-github-bot added the lts-watch-v6.x label Dec 16, 2016

Trott approved these changes Dec 16, 2016

View reviewed changes

Trott added the blocked PRs that are blocked by other issues or PRs. label Dec 16, 2016

Trott approved these changes Dec 16, 2016

View reviewed changes

addaleax approved these changes Dec 17, 2016

View reviewed changes

ljharb requested changes Feb 6, 2017

View reviewed changes

ChALkeR mentioned this pull request Feb 8, 2017

Changing Definition of "Locked" for Stability Index in API Docs #11200

Closed

joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Feb 17, 2017

joyeecheung mentioned this pull request Feb 18, 2017

util: eliminate unecessary internal/util export #11455

Closed

2 tasks

ChALkeR removed the blocked PRs that are blocked by other issues or PRs. label Mar 6, 2017

joyeecheung force-pushed the fix-assert branch from d27e611 to 855ac04 Compare March 11, 2017 13:49

joyeecheung mentioned this pull request Mar 11, 2017

assert.deepStrictEqual() fails to compare ES6 style object properties #11803

Closed

joyeecheung force-pushed the fix-assert branch 2 times, most recently from 2dabb67 to 3efc514 Compare March 11, 2017 14:51

assert: enforce type check in deepStrictEqual

3efc514

Add checks for the built-in type tags to catch objects with faked prototypes. See https://tc39.github.io/ecma262/#sec-object.prototype.tostring for a partial list of built-in tags. Fixes: nodejs#10258

addaleax approved these changes Mar 11, 2017

View reviewed changes

add missing link, fix typo

b8ed11b

addaleax closed this Mar 12, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

kt3k mentioned this pull request Jun 20, 2025

fix(ext/node): improve comparison of faked objects in deepStrictEqual denoland/deno#29819

Merged

                 ( `===` ).
 . [`[[Prototype]]`][prototype-spec] of objects are compared using
                 the [Strict Equality Comparison][] too.
+. [Type tags][Object.prototype.toString()] of objects should be the same.

               // AssertionError: { a: 1 } deepStrictEqual { a: '1' }
               // because 1 !== '1' using strict equality
+              // The following objects don't have owned properties

assert: enforce type check in deepStrictEqual by joyeecheung · Pull Request #10282 · nodejs/node · GitHub

Uh oh!

Uh oh!

assert: enforce type check in deepStrictEqual - #10282

assert: enforce type check in deepStrictEqual#10282joyeecheung wants to merge 2 commits intonodejs:masternodejs/node:masterfrom joyeecheung:fix-assertjoyeecheung/node:fix-assertCopy head branch name to clipboard

Conversation

joyeecheung commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 15, 2016 • 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!

joyeecheung Dec 16, 2016 • 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!

addaleax Dec 15, 2016 • 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!

joyeecheung commented Dec 15, 2016

Uh oh!

addaleax commented Dec 15, 2016

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott Dec 16, 2016 • 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!

joyeecheung commented Dec 16, 2016

Uh oh!

Trott commented Dec 16, 2016

Uh oh!

Trott commented Dec 16, 2016

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

assert: enforce type check in deepStrictEqual#10282
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:fix-assert

joyeecheung commented Dec 15, 2016 •
edited
Loading

addaleax Dec 15, 2016 •
edited
Loading

joyeecheung Dec 16, 2016 •
edited
Loading

addaleax Dec 15, 2016 •
edited
Loading

Trott Dec 16, 2016 •
edited
Loading