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

Prop equal against circular structures #776

Closed
Closed
3 changes: 2 additions & 1 deletion Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ grunt.initConfig({
},
all: [
"<%= jshint.all %>",
"!test/deepEqual.js"
"!test/deepEqual.js",
"!test/propEqual.js"
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add more exceptions. We should fix any issues in new files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added test/propEqual.js to the exceptions because the tests in both files are almost the same

Copy link
Member

Choose a reason for hiding this comment

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

Right, but we shouldn't duplicate crappy code.

]
},
search: {
Expand Down
8 changes: 2 additions & 6 deletions src/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,11 @@ QUnit.assert = Assert.prototype = {
},

propEqual: function( actual, expected, message ) {
actual = objectValues( actual );
expected = objectValues( expected );
this.push( QUnit.equiv( actual, expected ), actual, expected, message );
this.push( QUnit.equiv.props( actual, expected ), actual, expected, message );
},

notPropEqual: function( actual, expected, message ) {
actual = objectValues( actual );
expected = objectValues( expected );
this.push( !QUnit.equiv( actual, expected ), actual, expected, message );
this.push( !QUnit.equiv.props( actual, expected ), actual, expected, message );
},

deepEqual: function( actual, expected, message ) {
Expand Down
18 changes: 0 additions & 18 deletions src/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,6 @@ var QUnit,
} else {
return errorString;
}
},
/**
* Makes a clone of an object using only Array or Object as base,
* and copies over the own enumerable properties.
*
* @param {Object} obj
* @return {Object} New object with only the own properties (recursively).
*/
objectValues = function( obj ) {
var key, val,
vals = QUnit.is( "array", obj ) ? [] : {};
for ( key in obj ) {
if ( hasOwn.call( obj, key ) ) {
val = obj[ key ];
vals[ key ] = val === Object( val ) ? objectValues( val ) : val;
}
}
return vals;
};

QUnit = {};
Expand Down
105 changes: 98 additions & 7 deletions src/equiv.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,25 @@
// Author: Philippe Rathé <[email protected]>
QUnit.equiv = (function() {

/**
* Gets the keys of an object which are both enumerable and own
*
* @param {Object} obj
* @return {Array} Array with the enumerable and own keys of the object given
*/
function ownObjectKeys( obj ) {
var i,
keys = [];

for ( i in obj ) {
if ( hasOwn.call( obj, i ) ) {
keys.push( i );
}
}

return keys;
}

// Call the o related callback with the given arguments.
function bindCallbacks( o, callbacks, args ) {
var prop = QUnit.objectType( o );
Expand All @@ -17,6 +36,12 @@ QUnit.equiv = (function() {
// the real equiv function
var innerEquiv,

// the function that performs deep equivalence checks
deepEquiv,

// the function that performs property equivalence checks
propEquiv,

// stack to decide between skip/abort functions
callers = [],

Expand Down Expand Up @@ -116,7 +141,7 @@ QUnit.equiv = (function() {
}
}
}
if ( !loop && !innerEquiv( a[ i ], b[ i ] ) ) {
if ( !loop && !deepEquiv( a[ i ], b[ i ] ) ) {
parents.pop();
parentsB.pop();
return false;
Expand Down Expand Up @@ -171,7 +196,7 @@ QUnit.equiv = (function() {
}
}
aProperties.push( i );
if ( !loop && !innerEquiv( a[ i ], b[ i ] ) ) {
if ( !loop && !deepEquiv( a[ i ], b[ i ] ) ) {
eq = false;
break;
}
Expand All @@ -186,12 +211,11 @@ QUnit.equiv = (function() {
}

// Ensures identical properties name
return eq && innerEquiv( aProperties.sort(), bProperties.sort() );
return eq && deepEquiv( aProperties.sort(), bProperties.sort() );
}
};
}());

innerEquiv = function() { // can take multiple arguments
deepEquiv = function ( ) { // can take multiple arguments
var args = [].slice.apply( arguments );
if ( args.length < 2 ) {
return true; // end transition
Expand All @@ -212,8 +236,75 @@ QUnit.equiv = (function() {

// apply transition with (1..n) arguments
}( args[ 0 ], args[ 1 ] ) ) &&
innerEquiv.apply( this, args.splice( 1, args.length - 1 ) ) );
};
deepEquiv.apply( this, args.splice( 1, args.length - 1 ) ) );
},
propEquiv = function ( ) {
var args = [].slice.apply( arguments );
if ( args.length < 2 ) {
return true;
}

return ( (function ( a, b ) {
if ( !QUnit.is( "object", a ) || !QUnit.is( "object", b ) ) {
// if the test author submits parameters that are not objects, check for deepEquiv
return deepEquiv( a, b );
} else {
/*jshint forin:false */
var i, j, loop, aCircular, bCircular, currentProperty, subEquiv,
// Default to true
eq = true,
aProperties = ownObjectKeys( a ),
bProperties = ownObjectKeys( b );

// stack constructor before traversing properties
callers.push( a.constructor );

// track reference to avoid circular references
parents.push( a );
parentsB.push( b );

for ( i = 0; i < aProperties.length; i++ ){
currentProperty = aProperties[i];
loop = false;
for ( j = 0; j < parents.length; j++ ) {

aCircular = parents[j] === a[currentProperty];
bCircular = parentsB[j] === b[currentProperty];

if ( aCircular || bCircular ){
if ( a[ currentProperty ] === b[ currentProperty ] ||
aCircular && bCircular ){
loop = true;
} else {
eq = false;
break;
}
}

}

// we'll only use prop equivalence to compare objects
subEquiv = QUnit.is( "object", a[ currentProperty ] ) ?
propEquiv : deepEquiv;

if ( !loop && !subEquiv( a[ currentProperty ], b[ currentProperty ] ) ) {
eq = false;
break;
}
}
parents.pop();
parentsB.pop();
callers.pop(); // unstack, we are done

// ensure both objects have the same properties
return eq && deepEquiv(aProperties.sort(), bProperties.sort());
}
}( args[ 0 ], args[ 1 ] ) ) &&
propEquiv.apply(this, args.splice( 1, args.length - 1 ) ) );

},
innerEquiv = deepEquiv; // default equivalence check
Copy link
Member

Choose a reason for hiding this comment

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

Might as well drop innerEquiv and assign deepEquiv.props directly, then return that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that would totally make sense. How could propEquiv be a member of deepEquiv? They are kind of opposite. Something that could possibly be more natural would be to drop deepEquiv and assign innerEquiv and innerEquiv.props directly. The point of this is not having that "linguistic contradiction" with propEquiv being a subproperty of deepEquiv.

Does that make any sense?

Copy link
Member

Choose a reason for hiding this comment

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

Sticking with innerEquiv is fine.

innerEquiv.props = propEquiv;

return innerEquiv;
}());
77 changes: 0 additions & 77 deletions test/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,83 +46,6 @@ QUnit.test( "notStrictEqual", function( assert ) {
assert.notStrictEqual( "foo", { toString: function() { return "foo"; } } );
});

QUnit.test( "propEqual", function( assert ) {
assert.expect( 5 );
var objectCreate = Object.create || function( origin ) {
function O() {}
O.prototype = origin;
var r = new O();
return r;
};

function Foo( x, y, z ) {
this.x = x;
this.y = y;
this.z = z;
}
Foo.prototype.doA = function() {};
Foo.prototype.doB = function() {};
Foo.prototype.bar = "prototype";

function Bar() {
}
Bar.prototype = objectCreate( Foo.prototype );
Bar.prototype.constructor = Bar;

assert.propEqual(
new Foo( 1, "2", [] ),
{
x: 1,
y: "2",
z: []
}
);

assert.notPropEqual(
new Foo( "1", 2, 3 ),
{
x: 1,
y: "2",
z: 3
},
"Primitive values are strictly compared"
);

assert.notPropEqual(
new Foo( 1, "2", [] ),
{
x: 1,
y: "2",
z: {}
},
"Array type is preserved"
);

assert.notPropEqual(
new Foo( 1, "2", {} ),
{
x: 1,
y: "2",
z: []
},
"Empty array is not the same as empty object"
);

assert.propEqual(
new Foo( 1, "2", new Foo( [ 3 ], new Bar(), null ) ),
{
x: 1,
y: "2",
z: {
x: [ 3 ],
y: {},
z: null
}
},
"Complex nesting of different types, inheritance and constructors"
);
});

QUnit.test( "throws", function( assert ) {
assert.expect( 16 );
function CustomError( message ) {
Expand Down
2 changes: 1 addition & 1 deletion test/deepEqual.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
QUnit.module( "equiv" );
QUnit.module( "deepEquiv" );
Copy link
Member

Choose a reason for hiding this comment

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

Let's revert this - there is no exported method called "deepEquiv".


QUnit.test( "Primitive types and constants", function( assert ) {
assert.equal( QUnit.equiv( null, null ), true, "null" );
Expand Down
1 change: 1 addition & 0 deletions test/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<script src="dump.js"></script>
<script src="modules.js"></script>
<script src="deepEqual.js"></script>
<script src="propEqual.js"></script>
<script src="globals.js"></script>
<script src="swarminject.js"></script>
</head>
Expand Down
Loading