Evaluate symbolic keys for deep equality assertion #64

BrandonE · 2019-03-01T04:52:39Z

Purpose (TL;DR) - mandatory

Fix issue sinon#1974 by asserting that two objects are not deeply equal if the expectation object contains different symbolic keys than the actual object.

Solution - optional

Previously, deepEqual would just iterate over the Object.keys of the expectation object and compare them to the actual object. This PR changes this so that the object's own property symbols are also considered. Per @mantoni's suggestion, this will assert deep equal if the actual object contains additional symbolic properties that are missing from the expectation.

After making this change, this unit test failed. This is because arguments has a property symbol called iterator (Mozilla Docs):

> function test() { return Object.getOwnPropertySymbols(arguments) } test();
[ Symbol(Symbol.iterator) ]

To get around this, I made it so that if the expectation object is an arguments object, we will ignore the Symbol.iterator key. All of the existing unit tests now pass, though I am a bit worried that there might be additional default symbolic keys that this doesn't account for.

How to verify - mandatory

Check out this branch
npm install
npm test. This PR adds unit tests for objects with symbolic keys.

Checklist for author

npm run lint passes
References to standard library functions are cached.

…properties that are missing from the expectation. -Added unit tests. -Added myself to the authors list.

…ts object. -Fixed linting errors.

coveralls · 2019-03-01T04:55:54Z

Pull Request Test Coverage Report for Build 256

0 of 0 changed or added relevant lines in 0 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 96.97%

Files with Coverage Reduction	New Missed Lines	%
deep-equal.js	3	95.98%

Totals
Change from base Build 248:	-0.1%
Covered Lines:	452
Relevant Lines:	464

💛 - Coveralls

mantoni · 2019-03-01T07:03:12Z

Thank you! If you're reversing the symbol property checking as I suggested, I guess the additional arguments handling becomes obsolete.

BrandonE · 2019-03-01T07:11:05Z

@mantoni I believe the additional arguments handling is necessary because in this unit test, arguments is being passed as the first parameter, meaning it's the expectation. The actual value is missing that symbolic key, so that test would fail.

mantoni · 2019-03-01T07:16:08Z

That is why I'm saying you need to reverse the additional property check. Only properties in the expectation should be considered. Right now, you are checking the symbol properties in the actual.

mantoni · 2019-03-01T06:59:43Z

lib/deep-equal.test.js

+    it("returns true if object contains additional symbolic properties", function() {
+        var obj1 = {};
+        var obj2 = {};
+        obj2[symbol] = 42;


I think this check (and also the implementation) need to be the other way around. The first argument passed to deepEquals is the actual object, the second one the expectation. We want to ignore additional symbol properties on the actual.

mantoni · 2019-03-01T07:17:56Z

Oups. I just now saw that my review wasn't submitted. Sorry. 🤦‍♂️

…d is the expected.

BrandonE · 2019-03-01T07:24:24Z

You're right @mantoni, I mixed up the parameters. Thanks for calling that out! I just reversed the logic. However, if I comment out the additional arguments handling, I now see that two tests are failing:

mantoni · 2019-03-01T07:30:33Z

Oooh, that's an interesting one. I mean, backward compatibility aside, one could argue that we're now expecting an arguments object and those tests are failing because we just provided an array. It's probably a thing that rarely happens that someone provides a arguments object as the expectation 🤔

@mroderick I'd like to hear your opinion on this.

BrandonE · 2019-03-01T07:34:04Z

If backwards compatibility is critical, we could alternatively only assert that two objects are not deeply equal if both of them have the same symbols defined but the values don't match. This solution wouldn't have caught the real bug in one of my applications that inspired me to open this issue, though. 😄

mantoni · 2019-03-01T07:43:57Z

Yeah, the thing we're doing here right now is, we break the symetry of the function in this point. We did already break the symetry a while back with the matcher support, so I'd say we should maybe rename the parameters to actual and expectation. The "edge case" with arguments is theoretically one that could happen with any expectation object that has additional symbol properties that are not in the actual. So the current implementation is strictly speaking a breaking change anyway. I'm wondering if we should / can remove the 2 arguments tests and do a minor release without causing trouble downstream.

mroderick · 2019-03-01T08:49:46Z

I'd say we should maybe rename the parameters to actual and expectation

I think that is much clearer, as that's how we've been using this for a long time anyway.

mroderick · 2019-03-01T09:07:08Z

The non-strict comparison of this library is a double edged sword, that makes solutions to some situations more of a judgement call and not so much something that can just be "pick the stricter option" (often my preferred solution).

I always think of this library as "if I squint and the things look the same, then that's good enough".

If we decide to not support comparing Array instances with arguments (as expectation), then we should make the library fail loudly and with a good explanation.

samsam.deepEqual([1, 2], arguments);
// throw new TypeError("comparing Array to arguments is not supported, because arguments have symbol properties that Array does not. You could convert arguments to an Array with Array.from"

I wonder if that is too strict though, for a library that aims for "same-same" comparisons....

I too wonder if there are additional symbolic keys that we're not handling.

Did I understand the discussion correctly?

mantoni · 2019-03-01T09:10:35Z

Did I understand the discussion correctly?

Yes, I'm sure you did. There is another option though: We can still support arguments as expectations. They would just fail to compare against an array actual with the same values. I lean towards this solution because the user specifically wants to check for an arguments object and the actual isn't.

mroderick · 2019-03-01T09:13:41Z

They would just fail to compare against an array actual with the same values. I lean towards this solution because the user specifically wants to check for an arguments object and the actual isn't.

That tickles my "go for the stricter option" leaning, as a confident author would know that Array and arguments are not the same and cannot be treated the same. A less confident author would have a chance to learn this.

mantoni · 2019-03-01T09:16:29Z

Okay, @BrandonE, I would suggest remove the arguments iterator special case and inverse the assertions in the two failing tests.

This leaves us with the question of all questions: Are we going to release this as a minor or a major?

mroderick · 2019-03-01T09:24:34Z

Here’s a thought: At this point in time, WE know why this will fail. If we let it fail naturally, then users will either have to recognise why it fails, or investigate it. We can save a lot of investigations for users by failing this explicitly with an explanation. Sent from phone, apologies for brevity and auto-correct weirdness.

…

On 1 Mar 2019, at 09.16, Maximilian Antoni ***@***.***> wrote: Okay, @BrandonE, I would suggest remove the arguments iterator special case and inverse the assertions in the two failing tests. This leaves us with the question of all questions: Are we going to release this as a minor or a major? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mantoni · 2019-03-01T09:40:40Z

We can save a lot of investigations for users by failing this explicitly with an explanation.

I generally agree with this idea. However, we would have to special case the "expectation is an arguments object and actual is not" situation and throw an exception. So writing an assertion that verifies that actual is not an arguments object becomes impossible. Is that what you have in mind?

mantoni · 2019-03-01T09:47:04Z

Here is the change I'm suggesting:

diff --git a/lib/deep-equal.js b/lib/deep-equal.js
index e422472..7db27c1 100644
--- a/lib/deep-equal.js
+++ b/lib/deep-equal.js
@@ -114,12 +114,6 @@ function deepEqualCyclic(first, second, match) {
         var obj1IsArguments = isArguments(obj1);
         var obj2IsArguments = isArguments(obj2);
 
-        if (obj2IsArguments) {
-            keysAndSymbols2 = keysAndSymbols2.filter(function(keyOrSymbol) {
-                return keyOrSymbol !== Symbol.iterator;
-            });
-        }
-
         if (obj1IsArguments || obj2IsArguments) {
             if (obj1.length !== obj2.length) {
                 return false;
diff --git a/lib/deep-equal.test.js b/lib/deep-equal.test.js
index 964279c..7dadf73 100644
--- a/lib/deep-equal.test.js
+++ b/lib/deep-equal.test.js
@@ -449,12 +449,12 @@ describe("deepEqual", function() {
         assert.isFalse(checkDeep);
     });
 
-    it("returns true if arguments to array", function() {
+    it("returns false if arguments to array", function() {
         var gather = function() {
             return arguments;
         };
         var checkDeep = samsam.deepEqual([1, 2, {}, []], gather(1, 2, {}, []));
-        assert.isTrue(checkDeep);
+        assert.isFalse(checkDeep);
     });
 
     it("returns true if array to arguments", function() {
@@ -465,13 +465,13 @@ describe("deepEqual", function() {
         assert.isTrue(checkDeep);
     });
 
-    it("returns true if arguments to array like object", function() {
+    it("returns false if arguments to array like object", function() {
         var gather = function() {
             return arguments;
         };
         var arrayLike = { length: 4, "0": 1, "1": 2, "2": {}, "3": [] };
         var checkDeep = samsam.deepEqual(arrayLike, gather(1, 2, {}, []));
-        assert.isTrue(checkDeep);
+        assert.isFalse(checkDeep);
     });
 
     it("returns true for same error", function() {

mroderick · 2019-03-01T10:05:12Z

So writing an assertion that verifies that actual is not an arguments object becomes impossible

I am a bit slow this morning. Why would that be impossible?

mantoni · 2019-03-01T10:44:41Z

From my diff above:

it("returns false if arguments to array", function() {
         var gather = function() {
             return arguments;
         };
         var checkDeep = samsam.deepEqual([1, 2, {}, []], gather(1, 2, {}, []));
-        assert.isTrue(checkDeep);
+        assert.isFalse(checkDeep);
     });

This case would throw an exception.

BrandonE · 2019-03-02T00:36:18Z

I made the changes @mantoni suggested.

I agree with @mroderick's point that having a more specific error will help the users avoid investigating this issue unnecessarily. However, I think it'd be kind of odd for this to be the only exception we throw in this module. Further, I'm worried that, because it's possible that there are other symbolic keys that we aren't handling, people will have to investigate in those instances anyway. All of this makes me think a major version would be more suitable, just in case there is some degree of backwards incompatibility.

Also, in master, we already have a special handling for arguments objects. Do you know why this is necessary?

fearphage · 2019-03-02T03:05:21Z

lib/deep-equal.test.js

+        obj1[symbol] = 42;
+        obj2[symbol] = 43;
+        var checkDeep = samsam.deepEqual(obj1, obj2);
+        assert.isFalse(checkDeep);


MINOR: Can we make the tests a bit more concise?

var obj1 = {[symbol]: 42}; var obj2 = {[symbol]: 43}; assert.isFalse(samsam.deepEqual(obj1, obj2));

Actually, I tried that first, and the linter complained about an unexpected opening bracket. Changing it to this fixed the linting error.

mantoni

I think we should merge it like this. Thank you 👍

BrandonE · 2019-03-02T11:17:42Z

@mantoni Should I make the same change you made to formatio here?

mantoni · 2019-03-02T12:26:11Z

@BrandonE Yes, please 👍

…symbol concatenation. Support IE11.

BrandonE · 2019-03-02T14:34:15Z

@mantoni Done, plus I renamed first to actual and second to expectation. Lastly, there was an issue here where I was incorrectly concatenating obj2's symbols to obj1's keys. Now, both the keys and symbols come from the expectation object.

BrandonE · 2019-03-04T19:14:21Z

@mantoni @mroderick Any other suggestions before we merge?

mroderick

You may want to review the documentation https://github.com/sinonjs/samsam/blob/master/docs/index.md and see if that can be improved, considering the changes in this PR

BrandonE · 2019-03-06T23:06:50Z

How's this, @mroderick ?

mroderick · 2019-03-07T08:39:56Z

How's this, @mroderick ?

Lovely! I'll let @mantoni merge this, as they've spent more time thinking about it than I have 👍

mantoni · 2019-03-07T13:41:39Z

Great! Thanks for sticking with this @BrandonE ❤️

I've manually copied samsam into sinon and the test suite still passes. Doing the same for referee yields 4 failing test cases where arguments are compared with plain objects – as expected.

As discussed, I'm going to release this as a minor and fix the referee test cases according to the new symbol semantics.

mantoni · 2019-03-08T09:30:50Z

Released in v3.3.0.

angelikatyborska · 2019-03-22T13:33:46Z

lib/deep-equal.js

+        var expectationName = getClassName(expectationObj);
+        var expectationSymbols =
+            typeof Object.getOwnPropertySymbols === "function"
+                ? getOwnPropertySymbols(expectationObj)


Is it possible that this line was supposed to read ? Object.getOwnPropertySymbols(expectationObj)?

I just tried updating my project from sinon 7.2.7 to sinon 7.3.0, a change that includes this PR, and I get a lot of errors like this :

'Unhandled promise rejection', TypeError: undefined is not a constructor (evaluating 'getOwnPropertySymbols(expectationObj)') deepEqual@http://localhost:9876/absolute/Users/angelika/myproject/node_modules/sinon/pkg/sinon.js?911775df668393e499a0995e2789094c48727cc3:4395:40 deepEqualCyclic@http://localhost:9876/absolute/Users/angelika/myproject/node_modules/sinon/pkg/sinon.js?911775df668393e499a0995e2789094c48727cc3:4493:7 ...

They all go away when I change the line as I said.

Interesting. Actually, the line obove should also just refer to getOwnPropertySymbols because we're "caching" the function at the top of the file. I would guess that your environment does not actually have symbols support, but it gets shimmed after Sinon is loaded.

That is very probable, yes. Changing the above line to refer to just getOwnPropertySymbols without Object. also gets rid of my errors.

Would you care to send a pull request to fix it?

Here it is: #66

BrandonE added 4 commits January 30, 2019 23:36

Compare the values of object properties with symbolic keys.

2b736a1

Merge branch 'master' of github.com:sinonjs/samsam into symbolic-keys

1bcabb7

-Assert deep equal if the actual object contains additional symbolic …

95b37d4

…properties that are missing from the expectation. -Added unit tests. -Added myself to the authors list.

-Ignore a Symbol.iterator key if the expectation object is an argumen…

7bd5f6e

…ts object. -Fixed linting errors.

BrandonE mentioned this pull request Mar 1, 2019

calledWith ignores symbols used as property keys sinonjs/sinon#1974

Closed

mantoni reviewed Mar 1, 2019

View reviewed changes

Reversed my logic. The first parameter is the actual, while the secon…

67dc08b

…d is the expected.

Remove the special case for arguments objects.

c4188da

fearphage reviewed Mar 2, 2019

View reviewed changes

mantoni approved these changes Mar 2, 2019

View reviewed changes

BrandonE added 2 commits March 2, 2019 08:31

Renamed first to actual and second to expectation. Fixed the key and …

aac1b13

…symbol concatenation. Support IE11.

Staged full commit.

6605fa1

mroderick reviewed Mar 6, 2019

View reviewed changes

BrandonE added 2 commits March 6, 2019 17:04

Updated documentation.

70d5f0b

Added section to documentation explaining symbolic properties.

cef5959

mroderick approved these changes Mar 7, 2019

View reviewed changes

mantoni merged commit 91e1b85 into sinonjs:master Mar 7, 2019

mantoni mentioned this pull request Mar 7, 2019

Fix Symbol related tests for IE 11 #65

Merged

2 tasks

BrandonE deleted the symbolic-keys branch March 8, 2019 01:31

mantoni mentioned this pull request Mar 8, 2019

Update @sinonjs/samsam to v3.3.0 for symbol property support sinonjs/referee#94

Merged

2 tasks

angelikatyborska reviewed Mar 22, 2019

View reviewed changes

angelikatyborska mentioned this pull request Mar 22, 2019

Use cached getOwnPropertySymbols consistently #66

Merged

2 tasks

cincodenada mentioned this pull request Aug 8, 2020

Evaluate symbol keys for object matchers #206

Merged

Evaluate symbolic keys for deep equality assertion #64

Evaluate symbolic keys for deep equality assertion #64

Conversation

BrandonE commented Mar 1, 2019

Purpose (TL;DR) - mandatory

Solution - optional

How to verify - mandatory

Checklist for author

coveralls commented Mar 1, 2019 • edited

Pull Request Test Coverage Report for Build 256

💛 - Coveralls

mantoni commented Mar 1, 2019

BrandonE commented Mar 1, 2019

mantoni commented Mar 1, 2019

mantoni Mar 1, 2019

Choose a reason for hiding this comment

mantoni commented Mar 1, 2019

BrandonE commented Mar 1, 2019

mantoni commented Mar 1, 2019

BrandonE commented Mar 1, 2019

mantoni commented Mar 1, 2019

mroderick commented Mar 1, 2019

mroderick commented Mar 1, 2019

mantoni commented Mar 1, 2019

mroderick commented Mar 1, 2019

mantoni commented Mar 1, 2019

mroderick commented Mar 1, 2019 via email

mantoni commented Mar 1, 2019

mantoni commented Mar 1, 2019

mroderick commented Mar 1, 2019

mantoni commented Mar 1, 2019

BrandonE commented Mar 2, 2019

fearphage Mar 2, 2019

Choose a reason for hiding this comment

BrandonE Mar 2, 2019 • edited

Choose a reason for hiding this comment

mantoni left a comment

Choose a reason for hiding this comment

BrandonE commented Mar 2, 2019

mantoni commented Mar 2, 2019

BrandonE commented Mar 2, 2019

BrandonE commented Mar 4, 2019

mroderick left a comment

Choose a reason for hiding this comment

BrandonE commented Mar 6, 2019

mroderick commented Mar 7, 2019

mantoni commented Mar 7, 2019

mantoni commented Mar 8, 2019

angelikatyborska Mar 22, 2019

Choose a reason for hiding this comment

mantoni Mar 22, 2019

Choose a reason for hiding this comment

angelikatyborska Mar 22, 2019

Choose a reason for hiding this comment

mantoni Mar 22, 2019

Choose a reason for hiding this comment

angelikatyborska Mar 22, 2019

Choose a reason for hiding this comment

coveralls commented Mar 1, 2019 •

edited

BrandonE Mar 2, 2019 •

edited