GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — assert: enforce type check in deepStrictEqual by joyeecheung · Pull Request #10282 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Dec 15, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. lts-watch-v6.x labels Dec 15, 2016
@joyeecheung
joyeecheung force-pushed the fix-assert branch from ecdd024 to d065c7c Compare December 15, 2016 09:00
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016
@addaleax 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
addaleax reviewed Dec 15, 2016
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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’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? :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/assert.js Outdated

@addaleax addaleax Dec 15, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 15, 2016

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/assert.js Outdated

@addaleax addaleax Dec 15, 2016

Copy link
Copy Markdown
Member

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.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 15, 2016

Copy link
Copy Markdown
Member Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-assert-strict-typecheck.js Outdated

@addaleax addaleax Dec 15, 2016

Copy link
Copy Markdown
Member

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: Can I suggest that you update this to use the full URL here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-assert-strict-typecheck.js Outdated

@addaleax addaleax Dec 15, 2016

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 15, 2016

Copy link
Copy Markdown
Member Author

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Dec 15, 2016

Copy link
Copy Markdown
Member

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.

@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 16, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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, this disable tag is for all the deepEquals below :).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Trott Trott Dec 16, 2016

Copy link
Copy Markdown
Member

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 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()).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 16, 2016

Copy link
Copy Markdown
Member Author

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.

Thank you for the tips!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-assert-strict-typecheck.js Outdated

@addaleax addaleax Dec 15, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 15, 2016

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@addaleax addaleax Dec 15, 2016

Copy link
Copy Markdown
Member

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.

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 ;)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-assert-strict-typecheck.js Outdated

@addaleax addaleax Dec 15, 2016

Copy link
Copy Markdown
Member

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.

typos: returns and checks (here and below)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung
joyeecheung force-pushed the fix-assert branch from d065c7c to 89cdd8f Compare December 15, 2016 16:29
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016
@joyeecheung
joyeecheung force-pushed the fix-assert branch from 89cdd8f to df77ce1 Compare December 15, 2016 16:32
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016
@joyeecheung

joyeecheung commented Dec 15, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung
joyeecheung force-pushed the fix-assert branch from df77ce1 to 5cdfc0e Compare December 15, 2016 16:34
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Dec 15, 2016
@addaleax addaleax removed the lts-watch-v6.x label Dec 15, 2016
@addaleax

addaleax commented Dec 15, 2016

Copy link
Copy Markdown
Member

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
addaleax requested a review from Trott December 15, 2016 22:01
Trott
Trott requested changes Dec 16, 2016
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/assert.js Outdated

@Trott Trott Dec 16, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 16, 2016

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-assert.js Outdated

@Trott Trott Dec 16, 2016

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Dec 16, 2016

Copy link
Copy Markdown
Member Author

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.

Good suggestion. I will update these. Thanks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-assert.js Outdated

@Trott Trott Dec 16, 2016

Copy link
Copy Markdown
Member

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: assert.doesNotThrow()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@targos targos mentioned this pull request Dec 16, 2016
tools: forbid template literals in assert.throws #10301
Merged
2 of 4 tasks
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Dec 16, 2016
@joyeecheung

joyeecheung commented Dec 16, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Dec 16, 2016
View reviewed changes
@Trott

Trott commented Dec 16, 2016

Copy link
Copy Markdown
Member

@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?
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Dec 16, 2016
@Trott

Trott commented Dec 16, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Dec 16, 2016
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

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.

LGTM if CI is ✅ and CITGM is ✨

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
addaleax
addaleax approved these changes Dec 17, 2016
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
targos added a commit to targos/node that referenced this pull request Dec 19, 2016
@targos
tools: forbid template literals in assert.throws
7cb9813 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 20, 2016
@targos @cjihrig
tools: forbid template literals in assert.throws
be6d362 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
cjihrig pushed a commit that referenced this pull request Dec 20, 2016
@targos @cjihrig
tools: forbid template literals in assert.throws
367eb3e 
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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
ljharb
ljharb requested changes Feb 6, 2017
View reviewed changes

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

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'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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/assert.js Outdated

@ljharb ljharb Feb 6, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Feb 6, 2017

Copy link
Copy Markdown
Member Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/assert.js Outdated

@ljharb ljharb Feb 6, 2017

Copy link
Copy Markdown
Member

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.

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/assert.js Outdated

@ljharb ljharb Feb 6, 2017

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/assert.js Outdated

@ljharb ljharb Feb 6, 2017

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ChALkeR ChALkeR mentioned this pull request Feb 8, 2017
Changing Definition of "Locked" for Stability Index in API Docs #11200
Closed
@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Feb 17, 2017
@joyeecheung

joyeecheung commented Feb 17, 2017

Copy link
Copy Markdown
Member Author

This is blocked by #11128
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung mentioned this pull request Feb 18, 2017
util: eliminate unecessary internal/util export #11455
Closed
2 tasks done
joyeecheung added a commit to joyeecheung/node that referenced this pull request Feb 25, 2017
@joyeecheung
assert: fix premature deep strict comparison
aa023db 
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)
joyeecheung added a commit that referenced this pull request Feb 27, 2017
@joyeecheung
assert: fix premature deep strict comparison
562cf5a 
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]>
@ChALkeR

ChALkeR commented Mar 6, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ChALkeR ChALkeR removed the blocked PRs that are blocked by other issues or PRs. label Mar 6, 2017
@joyeecheung

joyeecheung commented Mar 7, 2017

Copy link
Copy Markdown
Member Author

@ChALkeR Yeah this is not blocked anymore. Thanks!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung
joyeecheung force-pushed the fix-assert branch from d27e611 to 855ac04 Compare March 11, 2017 13:49
@joyeecheung joyeecheung mentioned this pull request Mar 11, 2017
assert.deepStrictEqual() fails to compare ES6 style object properties #11803
Closed
@joyeecheung
joyeecheung force-pushed the fix-assert branch 2 times, most recently from 2dabb67 to 3efc514 Compare March 11, 2017 14:51
@joyeecheung

joyeecheung commented Mar 11, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung
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
addaleax approved these changes Mar 11, 2017
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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.

Still LGTM :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/assert.md
( `===` ).
2. [`[[Prototype]]`][prototype-spec] of objects are compared using
the [Strict Equality Comparison][] too.
3. [Type tags][Object.prototype.toString()] of objects should be the same.

@addaleax addaleax Mar 11, 2017

Copy link
Copy Markdown
Member

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 can’t find the definition of [Object.prototype.toString()] … am I missing something?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Mar 12, 2017

Copy link
Copy Markdown
Member Author

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.

Uh, forgot to put the reference link below :P

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/assert.md Outdated
// AssertionError: { a: 1 } deepStrictEqual { a: '1' }
// because 1 !== '1' using strict equality

// The following objects don't have owned properties

@addaleax addaleax Mar 11, 2017

Copy link
Copy Markdown
Member

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.

typo: own properties?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung

joyeecheung commented Mar 12, 2017

Copy link
Copy Markdown
Member Author

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung
add missing link, fix typo
b8ed11b
@addaleax

addaleax commented Mar 12, 2017

Copy link
Copy Markdown
Member

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Mar 12, 2017

Copy link
Copy Markdown
Member

Went ahead and landed this in efec14a :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Mar 12, 2017
addaleax pushed a commit that referenced this pull request Mar 12, 2017
@joyeecheung @addaleax
assert: enforce type check in deepStrictEqual
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@joyeecheung @jungx098
assert: enforce type check in deepStrictEqual
e98d6b6 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed
@kt3k kt3k mentioned this pull request Jun 20, 2025
fix(ext/node): improve comparison of faked objects in deepStrictEqual denoland/deno#29819
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb requested changes
@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@TimothyGu TimothyGu TimothyGu left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

assert.deepStrictEqual is fooled by strange dates

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@joyeecheung @addaleax @Trott @jasnell @italoacasas @ChALkeR @ljharb @TimothyGu @mscdex @targos @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.