Skip to content
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: check object prototype in deepEqual #621

Closed
wants to merge 1 commit into from

Conversation

vkurchatkin
Copy link
Contributor

Fixes: #620

R=@bnoordhuis

@vkurchatkin 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
assert.throws(makeBlock(a.deepEqual, new Boolean(true), {}), a.AssertionError);

// prototypes
assert.throws(makeBlock(assert.deepEqual, Object.create({}), {}), a.AssertionError);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Contributor Author

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

@bnoordhuis
Copy link
Member

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
Copy link
Contributor

I'm +1 on this.

@indutny
Copy link
Member

indutny commented Jan 27, 2015

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Jan 27, 2015

LGTM

@seishun
Copy link
Contributor

seishun commented Jan 27, 2015

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
Copy link
Member

rvagg commented Jan 27, 2015

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
Copy link
Member

rvagg commented Jan 27, 2015

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
Copy link

neumino commented Jan 27, 2015

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
Copy link

+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
Copy link
Contributor

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
Copy link
Contributor

-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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@Fishrock123
Copy link
Contributor

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
Copy link

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
Copy link

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.