Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

assert.deepEqual is not reflexive #7178

Closed
wants to merge 3 commits into from

Conversation

jugglinmike
Copy link

The behavior of assert.deepEqual is dependent on the ordering of arguments.

var assert = require('assert');
var args = (function() { return arguments; })();

assert.deepEqual([], args); // passes
assert.deepEqual(args, []); // throws

I recognize that this module's stability index is 5, but this non-reflexive behavior seems to be limited to the case of comparing an arguments object with an array and therefore inconsistent enough to warrant a patch.

In addition, the in-line comment immediately preceding the branch suggests that using Objects.keys on an arguments object can cause unexpected results. This operation currently occurs any time only the second argument is an arguments object.

Relevant source: https://github.com/joyent/node/blob/dbae8b569fd1afa04cddac47b30379e4ebf3388a/lib/assert.js#L204-L211

@trevnorris
Copy link

Agreed. This is a bug. The operation should be reflexive.

This won't be high on our priorities list, but any PR's will be entertained. :)

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Mike Pennisi

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

Ensure that the behavior of `assert.deepEqual` does not depend on
argument ordering  when comparing an `arguments` object with a
non-`arguments` object.
@jugglinmike
Copy link
Author

Alright, I've pushed up a commit and signed the CLA. The Jenkins build is currently failing, but I'm told that may be due to expected instability. Let me know if there's anything else I can do!

}
var aIsArgs = isArguments(a),
bIsArgs = isArguments(b);
if (aIsArgs ^ bIsArgs) {
Copy link
Member

Choose a reason for hiding this comment

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

May be do it in a less experimental way? ;) I mean without bit operations.

Copy link
Member

Choose a reason for hiding this comment

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

Also, please omit braces here.

@indutny
Copy link
Member

indutny commented Feb 25, 2014

Minor nits, otherwise LGTM

@jugglinmike
Copy link
Author

@indutny Thanks for the feedback. I've pushed a few new commits; let me know if you'd prefer me to squash them.

I also realized that, using the XOR operator, we don't need a variable named bIsArgs (the result of isArguments(b) is only referenced once in that case). Not sure if reducing local variables is important to you, but I thought I'd mention it just in case.

@indutny
Copy link
Member

indutny commented Feb 25, 2014

Thank you, landed in aae51ec

@indutny indutny closed this Feb 25, 2014
@jugglinmike
Copy link
Author

My pleasure

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants