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

deepEqual: returns true comparing empty object with empty class instance #37

Closed
mgred opened this issue Sep 9, 2018 · 0 comments
Closed

Comments

@mgred
Copy link
Contributor

mgred commented Sep 9, 2018

@tbiesemann opened in sinon sinonjs/sinon#1882

it("passes instance and object", function() {
    var obj = {};
    var ins = new (class MyClass {});
    assert.isFalse(samsam.deepEqual(obj, ins));
});
  • library version : 2.0.0
  • Environment : nodejs

What did you expect to happen?

It should return false.

What actually happens

It fails the test and returns true

Solution

As suggested by @tbiesemann, compare the names of the constructors:

if (
    type1 !== type2 ||
    class1 !== class2 ||
    obj1.constructor.name !== obj2.constructor.name ||
    keys1.length !== keys2.length
) {
    return false;
}

https://github.com/sinonjs/samsam/blob/master/lib/deep-equal.js#L105

Note: This modification lets the mentioned test pass but lets tests crash that pass objects that have no prototype.
I will provide a PR for this

@mgred mgred changed the title deepEqual: returns true comparing empty object with empty class incstance deepEqual: returns true comparing empty object with empty class instance Sep 9, 2018
mgred added a commit to mgred/samsam that referenced this issue Sep 9, 2018
This feature adds the comparision of the `name`
property of a given objects constructor if there
is any.

Otherwise, an empty object ({}) will be equal an
empty class instance (new class MyClass {}).

See sinonjs#37
mgred added a commit to mgred/samsam that referenced this issue Sep 12, 2018
* Fix typo
* Add more meaningful test description
* remove unecessary operator

See: sinonjs#37
mroderick pushed a commit that referenced this issue Sep 13, 2018
This feature adds the comparision of the `name`
property of a given objects constructor if there
is any.

Otherwise, an empty object ({}) will be equal an
empty class instance (new class MyClass {}).

See #37
@mgred mgred closed this as completed Sep 13, 2018
mgred added a commit to mgred/sinon that referenced this issue Sep 16, 2018
* add test case provided in sinonjs#1882
* change some eslint config to match test:
** class definitions are not enabled in ecmaVersion 5,
** addint Proxy as global

See: sinonjs#1882, sinonjs/samsam#37
mroderick pushed a commit to sinonjs/sinon that referenced this issue Sep 21, 2018
* test(1882): add test case

* add test case provided in #1882
* change some eslint config to match test:
** class definitions are not enabled in ecmaVersion 5,
** addint Proxy as global

See: #1882, sinonjs/samsam#37

* fix(1882): changes according to review

* Rewert config to support IE11 again
* Adopt changes to test to be ES5
franck-romano pushed a commit to franck-romano/sinon that referenced this issue Oct 1, 2019
* test(1882): add test case

* add test case provided in sinonjs#1882
* change some eslint config to match test:
** class definitions are not enabled in ecmaVersion 5,
** addint Proxy as global

See: sinonjs#1882, sinonjs/samsam#37

* fix(1882): changes according to review

* Rewert config to support IE11 again
* Adopt changes to test to be ES5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant