-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Conversation
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. :) |
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:
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.
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) { |
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.
May be do it in a less experimental way? ;) I mean without bit operations.
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.
Also, please omit braces here.
Minor nits, otherwise LGTM |
@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 |
Thank you, landed in aae51ec |
My pleasure |
The behavior of
assert.deepEqual
is dependent on the ordering of arguments.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 anarguments
object can cause unexpected results. This operation currently occurs any time only the second argument is anarguments
object.Relevant source: https://github.com/joyent/node/blob/dbae8b569fd1afa04cddac47b30379e4ebf3388a/lib/assert.js#L204-L211