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: don't compare object prototype property #636

Closed
wants to merge 1 commit into from

Conversation

vkurchatkin
Copy link
Contributor

All own enumerable properties are compared already. Comparing prototype property specifically can cause weird behaviour.

an alternative to #621

Example:

var assert = require('assert');
assert.deepEqual(Object.create({ a: 1 }), Object.create({ b: 2 }));
assert.deepEqual(Object.create({ prototype: 1 }), Object.create({ prototype: 2 }));

The first assertion is ok, the second throws. It is unexpected and has no logical explanation.

Changed test was failing because of the following nameBuilder2.prototype = Object;. Not sure if it is a mistake, or a test for CommonJS bug.

R=@iojs/collaborators

@seishun
Copy link
Contributor

seishun commented Jan 28, 2015

IMO if we're going to deviate from CommonJS spec, then the docs for assert should warn about this and provide a detailed description of their actual behavior.

@vkurchatkin
Copy link
Contributor Author

@vkurchatkin
Copy link
Contributor Author

Can anyone review this? @chrisdickinson or @cjihrig maybe?

@seishun
Copy link
Contributor

seishun commented Feb 1, 2015

This is how they do it in ringo: https://github.com/ringo/ringojs/blob/f06dd6d0c235a0c22fc50ec6ca59209e560da396/modules/assert.js#L92

A comment in the source code doesn't really cut it.

@vkurchatkin vkurchatkin closed this Feb 1, 2015
@vkurchatkin vkurchatkin reopened this Feb 1, 2015
@vkurchatkin
Copy link
Contributor Author

@seishun it's not a about the comment. Some facts:

  • CommonJS spec has a bug;
  • CommonJS spec is dead;
  • node.js/io.js documentation never mentions that assert has anything to do with CommonJS;
  • even ringojs, which claims that it is "CommonJS-based" and implements Unit Testing 1.0 (http://ringojs.org/documentation/commonjs_implementation) doesn't follow this part of the spec*
  • assert already deviates from CommonJS spec.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2015

The code looks fine, but this goes against what I would consider "deep equal."

I think a larger conversation needs to happen around the assert module. People are constantly identifying shortcomings and bugs in the implementation, but everyone is reluctant to change it because of CommonJS.

@vkurchatkin
Copy link
Contributor Author

The code looks fine, but this goes against what I would consider "deep equal."

Can you elaborate? Do you mean this particular change or overall deepEqual implementation?

@seishun
Copy link
Contributor

seishun commented Feb 1, 2015

node.js/io.js documentation never mentions that assert has anything to do with CommonJS;

It doesn't explain what "deep equality" means either, which makes it "obvious" that it uses the algorithm from the buggy and dead spec. Let's not follow ringojs in confusing users even more.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2015

The code looks fine, but this goes against what I would consider "deep equal."

Can you elaborate? Do you mean this particular change or overall deepEqual implementation?

I mean that I would expect a deepEqual implementation to examine the prototype, either using === or a call to objEquiv(). Again, I personally think the current assert methods need to be revisited in general.

@vkurchatkin
Copy link
Contributor Author

@cjihrig this is covered by #639. I decided would be this best course of action. This PR is a bugfix (semver patch), #639 is a new feature (semver minor). Changing deepEqual behaviour would require major bump, which I think is not justified in this case.

@rvagg
Copy link
Member

rvagg commented Feb 1, 2015

I'm not comfortable providing sign-off on this, I'm more of a -0 on this

@vkurchatkin do you see this as a necessary part of #639?

@vkurchatkin
Copy link
Contributor Author

@rvagg no, I don't. But since they share implementation deepStrictEqual will check prototype too. It's not something that we want in a brand new function (I guess), so I should add a condition to skip it in strict mode

@vkurchatkin vkurchatkin added assert Issues and PRs related to the assert subsystem. tc-agenda labels Feb 3, 2015
@vkurchatkin
Copy link
Contributor Author

As discussed at TC meeting, CommonJS spec is not relevant anymore. I will land this as soon as I see a couple of LGTMs.

@@ -130,7 +130,7 @@ assert.doesNotThrow(makeBlock(a.deepEqual, nb1, nb2));

nameBuilder2.prototype = Object;
nb2 = new nameBuilder2('Ryan', 'Dahl');
assert.throws(makeBlock(a.deepEqual, nb1, nb2), a.AssertionError);
assert.doesNotThrow(makeBlock(a.deepEqual, nb1, nb2), 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.

Minus , a.AssertionError and perhaps drop the assert.doesNotThrow call entirely? The exception can just bubble up.

@bnoordhuis
Copy link
Member

Left a comment. Otherwise LGTM.

All own enumerable properties are compared already. Comparing
`prototype` property specifically can cause weird behaviour.
@vkurchatkin
Copy link
Contributor Author

removed a.AssertionError. don't really want to drop assert.doesNotThrow, because the intention is to test deepEqual, not that two random objects are equal. what do you think?

@bnoordhuis
Copy link
Member

I think that assert.doesNotThrow in general is a bit point pointless. It doesn't matter for this PR though. LGTM!

vkurchatkin added a commit that referenced this pull request Feb 6, 2015
All own enumerable properties are compared already. Comparing
`prototype` property specifically can cause weird behaviour.

PR-URL: #636
Reviewed-By: Ben Noordhuis <[email protected]>
@vkurchatkin
Copy link
Contributor Author

Thanks! landed in e7573f9

@vkurchatkin vkurchatkin closed this Feb 6, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

In the last TC meeting, I think there was some discussion about holding off on this until 2.0.0.

@vkurchatkin
Copy link
Contributor Author

I'm pretty sure it wasn't. There was some confusion with #621 though . That WAS a breaking change, this is merely a bugfix

@cjihrig
Copy link
Contributor

cjihrig commented Feb 6, 2015

OK, cool. Sorry for the confusion.

@rvagg rvagg mentioned this pull request Feb 9, 2015
rvagg added a commit that referenced this pull request Feb 11, 2015
Notable changes:

* stream:
  - Simpler stream construction, see
    nodejs/readable-stream#102 for details.
    This extends the streams base objects to make their constructors
    accept default implementation methods, reducing the boilerplate
    required to implement custom streams. An updated version of
    readable-stream will eventually be released to match this change
    in core. (@sonewman)
* dns:
  - `lookup()` now supports an `'all'` boolean option, default to
    `false` but when turned on will cause the method to return an
    array of *all* resolved names for an address, see,
    #744 (@silverwind)
* assert:
  - Remove `prototype` property comparison in `deepEqual()`,
    considered a bugfix, see #636
    (@vkurchatkin)
  - Introduce a `deepStrictEqual()` method to mirror `deepEqual()`
    but performs strict equality checks on primitives, see
    #639 (@vkurchatkin)
* **tracing**:
  - Add LTTng (Linux Trace Toolkit Next Generation) when compiled
    with the  `--with-lttng` option. Trace points match those
    available for DTrace and ETW.
    #702 (@thekemkid)
* npm upgrade to 2.5.1
* **libuv** upgrade to 1.4.0
* Add new collaborators:
  - Aleksey Smolenchuk (@lxe)
  - Shigeki Ohtsu (@shigeki)
@loveencounterflow
Copy link

May i add that i consider assert.deepEqual and friends to be broken, and that this is because the CommonJS assert specs do not make much sense. I understand that spec is no more considered relevant here? If so, that's a good thing.

First of all, the API is horrible. Logically, either two things are equal or they're not, but assert has no less than eight methods: equal, notEqual, deepEqual, notDeepEqual, strictEqual, notStrictEqual, deepStrictEqual, notDeepStrictEqual. If we concede that the not methods are really needed, there's still four competing concepts of 'equality' here: plain equal, deep (and plain) equal, strict equal, and deep strict equal. There should be only a single equality comparison (with two API methods as its negation is needed) with 'deep' equality covering all cases of 'shallow' equality.

As i try to show in jseq, the concept of 'equality' can lead to some hairy questions when it meets the reality of a language like JavaScript with its NaN and +0 vs –0 and so on. What is easier to see is that there's object identity, value equality, and, quite apart from this, as many kinds of 'equivalence' as you have use cases for. I'd argue that only what is currently available as assert.(not)deepStrictEqual approaches a satisfyingly logical, good-enough implementation for 'equality'; the other three concepts are either bogus or mere equivalence (that is, there may be use cases where '456' can stand in for 456, but those two values should never be considered 'equal', and this kind of comparison should not be made part of a standard library—bad enough some people still use == at all).

I suggest to introduce assert.eq (?) and assert.neq (?) (anyway, two snappy names), and remove all the other equality tests. Additionally, the behavior of eq should be based on the outcome of a new util.equals method that does sound deep and shallow equality testing. As for hairy borderline cases (NaN, –0), maybe those should be made configurable.

@vkurchatkin
Copy link
Contributor Author

I agree that non-strict equality is not really useful for assertions (or more like not useful at all). On the other hand, shallow equality is important. There is no way to know if objects should be treated as values or having identity.

It is unlikely that something would be added (or removed) to assert. Recent amendments were made only to fix inadequate behaviour in non-breaking manner. There are many ways to do assertions (extend prototypes, etc) and they can live nicely in npm.

@loveencounterflow
Copy link

I fully understand the part where one wants to avoid breaking existing code and, therefore, keeps 'strange methods with strange behaviors' around and just adds new methods with 'better behavior'. Although if this principle is the only evolutional guideline then NodeJS will become ever more PHP-ish over the years (assert is already the most PHP-ish part of NodeJS/io.js).

I don't get where the difference between shallow and deep equality is important. 4 equals 4 and does not equal '4', [ 4 ], 5 or { foo: 4 }. { foo: { bar: 4 } } equals { foo: { bar: 4 } } and does not equal { foo: { baz: 4 } } or 4 or anything. And object identity, i argue, is something completely different. I think Python got this one right, in a very clear manner (although i know about some flaws in their concepts); if you want identity, you say a is b, a is not b ; if you want equality, you say a == b, a != b in Python.

This is really a semantic problem, and i guess where we can meet is saying that assert.strictEqual should really be assert.is—that is, the API's flaw is not that there is a method too much, but that its name is infelicitous, as it is neither about 'strictness' (?) nor is it about 'equality' but, plainly, object identity (which happens to overlap with equality for primitive values). The API feels so PHP-ish because PHP is renowned for having many broken methods and lots and lots of infelicitous naming decisions.

@loveencounterflow
Copy link

@cjihrig "I mean that I would expect a deepEqual implementation to examine the prototype, either using === or a call to objEquiv()"—goes to show how hairy equality can become. I'd say that if some properties of all the properties x.a, x.b, x.c of some object x are visible because of a prototype X that has, say, a and b, than that value x can not be considered equal to an object y that has y.b as an own property and only y.a through its prototype Y.a. I doubt that many equality testing suites do test for that.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants