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

QUnit.equiv ignores object methods #914

Closed
gibson042 opened this Issue Dec 31, 2015 · 6 comments

Comments

3 participants
@gibson042
Member

gibson042 commented Dec 31, 2015

https://jsfiddle.net/xcrh9n3d/

Since we verify inherited properties anyway, we should replace the weird constructor-based "function" callback with a simple return false (relying solely on the strict equality check in innerEquiv).

@gibson042

This comment has been minimized.

Show comment
Hide comment
@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Jan 4, 2016

Member

It looks like a bug on deepEqual, which would be ok on a propEqual use only.

Even if we use LoDash's isEqual it looks like deepEqual is not doing it's job to call .equiv on the object properties.

function Foo( method ) {
    this.method = method;
}

QUnit.module( "Method comparison failure" );
QUnit.test( "proof", function( assert ) {
    var a = new Foo( function(){ return true; } ),
    b = new Foo( function(){ return false; } );
    assert.deepEqual( a, b, "objects" );
  assert.equal( a.method, b.method, "object methods (loose)" );
  assert.strictEqual( a.method, b.method, "object methods (strict)" );
} );

on your jsfiddle example (copy/pasted above) I would expect all the assertions to fail, but the first is passing.

We can see it's not using QUnit.equiv at all if we add the following:

assert.ok( QUnit.equiv( a.method, b.method ), "QUnit.equiv" );

Or it is converting functions to strings and comparing as the example below:

function( ){
  [code]
}

It might be intentional, but it's not something I am aware of. @jzaefferer do you know anything about this behavior?

Member

leobalter commented Jan 4, 2016

It looks like a bug on deepEqual, which would be ok on a propEqual use only.

Even if we use LoDash's isEqual it looks like deepEqual is not doing it's job to call .equiv on the object properties.

function Foo( method ) {
    this.method = method;
}

QUnit.module( "Method comparison failure" );
QUnit.test( "proof", function( assert ) {
    var a = new Foo( function(){ return true; } ),
    b = new Foo( function(){ return false; } );
    assert.deepEqual( a, b, "objects" );
  assert.equal( a.method, b.method, "object methods (loose)" );
  assert.strictEqual( a.method, b.method, "object methods (strict)" );
} );

on your jsfiddle example (copy/pasted above) I would expect all the assertions to fail, but the first is passing.

We can see it's not using QUnit.equiv at all if we add the following:

assert.ok( QUnit.equiv( a.method, b.method ), "QUnit.equiv" );

Or it is converting functions to strings and comparing as the example below:

function( ){
  [code]
}

It might be intentional, but it's not something I am aware of. @jzaefferer do you know anything about this behavior?

@leobalter leobalter added the bug label Jan 4, 2016

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 4, 2016

Member

QUnit.equiv is explicitly treating all methods as equal on non-plain objects: https://github.com/jquery/qunit/blob/5977d91111575c105a8f0dde1364fb37643fcdd4/src/equiv.js#L85

Member

gibson042 commented Jan 4, 2016

QUnit.equiv is explicitly treating all methods as equal on non-plain objects: https://github.com/jquery/qunit/blob/5977d91111575c105a8f0dde1364fb37643fcdd4/src/equiv.js#L85

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Jan 4, 2016

Member

The origin is here: b8b33e7#diff-2cec82a4b4b78d3fcff317475ff08e42L491 and here: 4ffe241#diff-2cec82a4b4b78d3fcff317475ff08e42R112

This is a piece of code existing for 7 years and it might be interesting trying to get rid of it.

Member

leobalter commented Jan 4, 2016

The origin is here: b8b33e7#diff-2cec82a4b4b78d3fcff317475ff08e42L491 and here: 4ffe241#diff-2cec82a4b4b78d3fcff317475ff08e42R112

This is a piece of code existing for 7 years and it might be interesting trying to get rid of it.

@kumarmj

This comment has been minimized.

Show comment
Hide comment
@kumarmj

kumarmj Sep 30, 2016

Contributor

I would love to work upon this bug issue, I studied this issue. I concluded that -

Our intention was to show positive result for -

function B() {
    this.fn = function() {};
}

var b1 = new B();
var b2 = new B();

source code: https://github.com/jquery/qunit/blob/master/test/main/deepEqual.js#L1298-L1302

and we missed this -

function Foo( method ) {
    this.method = method;
}
var a = new Foo( function(){ return true; } );
var b = new Foo( function(){ return false; } );

As pointed above by @gibson042 - https://jsfiddle.net/xcrh9n3d/

I think, what @leobalter said above can solve the problem

converting functions to strings and comparing

Some addition on this line https://github.com/jquery/qunit/blob/master/src/equiv.js#L93

return caller !== Object && typeof caller !== "undefined" &&
            a.toString() === b.toString();

and problem solved. Please correct me, if I went wrong about things

Contributor

kumarmj commented Sep 30, 2016

I would love to work upon this bug issue, I studied this issue. I concluded that -

Our intention was to show positive result for -

function B() {
    this.fn = function() {};
}

var b1 = new B();
var b2 = new B();

source code: https://github.com/jquery/qunit/blob/master/test/main/deepEqual.js#L1298-L1302

and we missed this -

function Foo( method ) {
    this.method = method;
}
var a = new Foo( function(){ return true; } );
var b = new Foo( function(){ return false; } );

As pointed above by @gibson042 - https://jsfiddle.net/xcrh9n3d/

I think, what @leobalter said above can solve the problem

converting functions to strings and comparing

Some addition on this line https://github.com/jquery/qunit/blob/master/src/equiv.js#L93

return caller !== Object && typeof caller !== "undefined" &&
            a.toString() === b.toString();

and problem solved. Please correct me, if I went wrong about things

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 3, 2016

Member

That seems like a reasonable compromise to me, @kumarmj. I guess a pull request is in order!

Member

gibson042 commented Oct 3, 2016

That seems like a reasonable compromise to me, @kumarmj. I guess a pull request is in order!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment