assert: check object prototype in `deepEqual` #621

vkurchatkin · 2015-01-27T12:38:25Z

Fixes: #620

bnoordhuis · 2015-01-27T13:10:06Z

test/parallel/test-assert.js

+assert.throws(makeBlock(a.deepEqual, new Boolean(true), {}), a.AssertionError);
+
+// prototypes
+assert.throws(makeBlock(assert.deepEqual, Object.create({}), {}), a.AssertionError);


Long line, please wrap at 80 columns. If you use vim, adding the following to your .vimrc gives you a visual indicator:

set textwidth=80 set colorcolumn=+1

Sorry about that. I actually DO have a guideline, but in this case I saw some long lines just above and ignored it. Anyway, wrapped this and other offending lines.

bnoordhuis · 2015-01-27T13:11:30Z

LGTM apart from the long lines. I'd like one or two other contributors to sign off on this as well.

Can you update the commit log with a few lines explaining what changed and why?

vkurchatkin · 2015-01-27T13:25:18Z

@bnoordhuis updated. I labeled this PR as semver-major because it is kind of a breaking change.

bnoordhuis · 2015-01-27T13:30:52Z

Don't hate me but textwidth=72 in commit logs. :-) I have a autocmd BufEnter COMMIT_EDITMSG set textwidth=72 for that in my .vimrc.

Summoning more @iojs/collaborators.

Objects with different prototypes should not be considered deep equal, e.g. `assert.deepEqual({}, [])` should throw. Previously `deepEqual` was implemented as per CommonJS Unit Testing/1.0 spec. It states that objects should have 'an identical "prototype" property' to be considered deep equal, which is incorrect. Instead object prototypes should be compared, i.e. `Object.getPrototypeOf` or `__proto__`. Fixes: nodejs#620

evanlucas · 2015-01-27T13:33:21Z

I'm +1 on this.

indutny · 2015-01-27T13:41:26Z

LGTM

cjihrig · 2015-01-27T14:10:45Z

LGTM

seishun · 2015-01-27T19:39:48Z

I'm not so sure, people might expect this to be CommonJS compliant, even though the spec is horribly broken (and deepEqual is just one example).

rvagg · 2015-01-27T22:23:15Z

I'm not convinced either, I can imagine this breaking a whole lot of userland code, let's try and solicit broader feedback, particularly from people implementing other assertion libraries and test runners.

rvagg · 2015-01-27T22:26:32Z

Some poeple I can think of that may be able to muster an opinion on this change (and may even be impacted by it): @substack, @cjohansen, @isaacs, @thlorenz

semver-major is also going to complicate getting this merged because we haven't even figured out how to handle semver-minor branching and releases yet.

neumino · 2015-01-27T22:48:06Z

Please don't change the behavior of deepEqual; consider adding a new method instead.

This makes impossible to compare an object sent over the network in the JSON format and an instance of a model without creating another instance of the model.

bevacqua · 2015-01-27T23:12:14Z

+1 to what @neumino said. Prototype equality means that you can't compare a model object with a plain JS object for quick assertions

thlorenz · 2015-01-28T00:52:31Z

I've mainly been using deepEqual to compare arrays and object literals so not much would break for what I'm aware of, but there are going to be edge cases like the one @neumino is describing.

The question is what does deepEqual mean regarding non-primitive types?

a) all properties -- that are not builtin (like __proto__) -- are equal recursively
b) all properties and the builtin object properties like __proto__ are equal recursively

Neither is clear from the docs:

Tests for deep equality.

I understand the current check for prototype doesn't seem to be of much use, but that would be a remnant of history we'd need to live with. Removing this check entirely could also be discussed although I'm not sure what effect on existing userland code that woulld have.

However, whether the current behavior is due to some misunderstanding or not, changing it now as proposed in this PR is problematic.
For starters, changes to the method also need to be reflected in the deep-equal and deep-is possibly more? modules.
More painful would be to go and fix all tests breaking after upgrading the io.js version.

The CommonJS Unit testing Wiki is not a specification, but merely a collection of suggestions.

This wiki is a starting point for collecting up ideas, any draft API suggestions

Additionally io.js deviated from these in other areas as well.

Taking all this into account I'd suggest to stick with the current behavior for deepEqual and discuss an alternative method, i.e. protoEqual to just check the __proto__ or alternatively deepProtoEqual (maybe better name) to combine deepEqual and protoEqual.

aheckmann · 2015-01-28T01:01:52Z

-1 from me as well since it will break existing apps and isn't necessarily "more correct" behavior than current behavior. Either a new method or an option seems safer.

thlorenz · 2015-01-28T01:09:41Z

Do we actually have a way to test at least how many modules/tests on npm would break with this change?
That could give us more data to judge how much pain people would have to endure if we preferred correctness over backwards-compat.

jonathanong · 2015-01-28T02:19:41Z

LGTM, but this is semver-major like the label states.

@thlorenz i'm interested in making some acceptance test build system described in nodejs/roadmap#11, just need to plan it out in iojs/build.

vkurchatkin · 2015-01-28T05:19:26Z

as a non-breaking alternative: introduce deepStrictEqual which uses === and also does this

Fishrock123 · 2015-01-28T05:25:20Z

as a non-breaking alternative: introduce deepStrictEqual which uses === and also does this

The problem is that deep and strict are mutually exclusive conceptually.

+1 to deepStrictlyQuacksLike()

meandmycode · 2015-01-28T07:02:06Z

I'm -1 on this personally the times I use deep equality are on things that aren't the same but are at least shaped the same. I.e. structural equivalence or 'duck typed'.

cjohansen · 2015-01-28T08:08:35Z

deepEqual({}, []) should definitely throw. I don't have a strong opinion on whether this means deepEquals should enforce a check on the prototype. I'm leaning towards +1 on this change.

To fill the void, you could add a function like referee's assert.match (although with less flexibility/confusing amounts of input types) - fn(a, b) that passes when a contains all the properties in b with the same (recursive check) values, but allows a to have additional properties (and different prototype).

vkurchatkin · 2015-01-28T15:25:03Z

thanks for the feedback! the PR seems to be too controversial to be merged. We need to go in a different direction to solve this issue.

vkurchatkin added 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. labels Jan 27, 2015

bnoordhuis reviewed Jan 27, 2015
View reviewed changes

vkurchatkin force-pushed the assert-deep branch from e0b88b5 to a77effa Compare January 27, 2015 13:22

vkurchatkin force-pushed the assert-deep branch from a77effa to 5fd9451 Compare January 27, 2015 13:36

jonathanong mentioned this pull request Jan 28, 2015

docs: lower the maximum stability level to "stable" #633

Closed

vkurchatkin closed this Jan 28, 2015

vkurchatkin mentioned this pull request Jan 28, 2015

assert: don't compare object prototype property #636

Closed

Fishrock123 mentioned this pull request Feb 4, 2015

assert: introduce deepStrictEqual #639

Closed

jzaefferer mentioned this pull request Mar 11, 2015

Prop equal against circular structures qunitjs/qunit#776

Closed

mbroadst mentioned this pull request Sep 8, 2015

test(Model): reworked model.js test neumino/thinky#329

Closed

This was referenced Sep 11, 2015

Object with index as keys should not equal array mjackson/expect#17

Closed

Update to 4.0.0 deepEqual inspect-js/node-deep-equal#25

Closed

snyk-bot mentioned this pull request Apr 25, 2023

[Snyk] Fix for 3 vulnerabilities aliscco/alisco-node#180

Open

leonardoadame mentioned this pull request Jul 7, 2023

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#57

Open

snyk-io bot mentioned this pull request Oct 23, 2024

[Snyk] Fix for 5 vulnerabilities Graysonbarton/node#93

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: check object prototype in `deepEqual` #621

assert: check object prototype in `deepEqual` #621

vkurchatkin commented Jan 27, 2015

bnoordhuis Jan 27, 2015

vkurchatkin Jan 27, 2015

bnoordhuis commented Jan 27, 2015

vkurchatkin commented Jan 27, 2015

bnoordhuis commented Jan 27, 2015

evanlucas commented Jan 27, 2015

indutny commented Jan 27, 2015

cjihrig commented Jan 27, 2015

seishun commented Jan 27, 2015

rvagg commented Jan 27, 2015

rvagg commented Jan 27, 2015

neumino commented Jan 27, 2015

bevacqua commented Jan 27, 2015

thlorenz commented Jan 28, 2015

aheckmann commented Jan 28, 2015

thlorenz commented Jan 28, 2015

jonathanong commented Jan 28, 2015

vkurchatkin commented Jan 28, 2015

Fishrock123 commented Jan 28, 2015

meandmycode commented Jan 28, 2015

cjohansen commented Jan 28, 2015

vkurchatkin commented Jan 28, 2015

assert: check object prototype in deepEqual #621

assert: check object prototype in deepEqual #621

Conversation

vkurchatkin commented Jan 27, 2015

bnoordhuis Jan 27, 2015

Choose a reason for hiding this comment

vkurchatkin Jan 27, 2015

Choose a reason for hiding this comment

bnoordhuis commented Jan 27, 2015

vkurchatkin commented Jan 27, 2015

bnoordhuis commented Jan 27, 2015

evanlucas commented Jan 27, 2015

indutny commented Jan 27, 2015

cjihrig commented Jan 27, 2015

seishun commented Jan 27, 2015

rvagg commented Jan 27, 2015

rvagg commented Jan 27, 2015

neumino commented Jan 27, 2015

bevacqua commented Jan 27, 2015

thlorenz commented Jan 28, 2015

aheckmann commented Jan 28, 2015

thlorenz commented Jan 28, 2015

jonathanong commented Jan 28, 2015

vkurchatkin commented Jan 28, 2015

Fishrock123 commented Jan 28, 2015

meandmycode commented Jan 28, 2015

cjohansen commented Jan 28, 2015

vkurchatkin commented Jan 28, 2015

assert: check object prototype in `deepEqual` #621

assert: check object prototype in `deepEqual` #621