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

Assert: Support compare primitive objects in deepEqual #905

Closed
wants to merge 5 commits into
from

Conversation

4 participants
@mixed
Contributor

mixed commented Dec 15, 2015

Ref #895

@mixed mixed changed the title from Assert: Support compare primitive objects. to Assert: Support compare primitive objects in deepEqual Dec 15, 2015

Show outdated Hide outdated test/main/deepEqual.js
QUnit.test( "Compare primitive objects", function( assert ) {
var SafeNumber = Number,
SafeString = String,
SafeBoolean = Boolean;

This comment has been minimized.

@leobalter

leobalter Dec 15, 2015

Member

why saving these references? Does not look necessary.

@leobalter

leobalter Dec 15, 2015

Member

why saving these references? Does not look necessary.

This comment has been minimized.

@mixed

mixed Dec 15, 2015

Contributor

@leobalter
For lint validation. like it link

:)

@mixed

mixed Dec 15, 2015

Contributor

@leobalter
For lint validation. like it link

:)

This comment has been minimized.

@leobalter

leobalter Dec 15, 2015

Member

Oops, makes sense.

We should fix the code style here, that's relatively new on the code style, but we should declare all the variables with a new var statement.

var SafeNumber = Number;
var SafeString = String;
var SafeBoolean = Boolean;
@leobalter

leobalter Dec 15, 2015

Member

Oops, makes sense.

We should fix the code style here, that's relatively new on the code style, but we should declare all the variables with a new var statement.

var SafeNumber = Number;
var SafeString = String;
var SafeBoolean = Boolean;
Show outdated Hide outdated test/main/deepEqual.js
@@ -1581,6 +1581,35 @@ QUnit.test( "Test that must be done at the end because they extend some primitiv
}
);
QUnit.test( "Compare primitive objects", function( assert ) {

This comment has been minimized.

@leobalter

leobalter Dec 15, 2015

Member

We could split these tests in a module and create a test for each type.

@leobalter

leobalter Dec 15, 2015

Member

We could split these tests in a module and create a test for each type.

This comment has been minimized.

@leobalter

leobalter Dec 15, 2015

Member

This should also be called "Compare primitive values", they are not primitive objects.

@leobalter

leobalter Dec 15, 2015

Member

This should also be called "Compare primitive values", they are not primitive objects.

This comment has been minimized.

@mixed

mixed Dec 15, 2015

Contributor

@leobalter
OK. I going to update.

@mixed

mixed Dec 15, 2015

Contributor

@leobalter
OK. I going to update.

Show outdated Hide outdated test/main/deepEqual.js
assert.equal( QUnit.equiv(new SafeNumber(1), new SafeNumber(1) ), true,
"Number: When create same parameter should be equal."
);

This comment has been minimized.

@leobalter

leobalter Dec 15, 2015

Member

We may also simplify these tests using the ok and notOk assertions and/or using a single line for each parameter:

assert.equal(
    QUnit.equiv( new SafeNumber(1), new SafeNumber(1) ),
    true,
    "Number: When create same parameter should be equal."
);

// or simply:
assert.ok(
    QUnit.equiv( new SafeNumber( 1 ), new SafeNumber( 1 ) ),
    "Number: When create same parameter should be equal."
);
@leobalter

leobalter Dec 15, 2015

Member

We may also simplify these tests using the ok and notOk assertions and/or using a single line for each parameter:

assert.equal(
    QUnit.equiv( new SafeNumber(1), new SafeNumber(1) ),
    true,
    "Number: When create same parameter should be equal."
);

// or simply:
assert.ok(
    QUnit.equiv( new SafeNumber( 1 ), new SafeNumber( 1 ) ),
    "Number: When create same parameter should be equal."
);

This comment has been minimized.

@leobalter

leobalter Dec 15, 2015

Member

Also: the code style requires a space before and after the arguments list, so new SafeNumber(1) => new SafeNumber( 1 )

@leobalter

leobalter Dec 15, 2015

Member

Also: the code style requires a space before and after the arguments list, so new SafeNumber(1) => new SafeNumber( 1 )

This comment has been minimized.

@mixed

mixed Dec 15, 2015

Contributor

@leobalter
Thank you for your advice. I going to update all review.

@mixed

mixed Dec 15, 2015

Contributor

@leobalter
Thank you for your advice. I going to update all review.

Show outdated Hide outdated test/main/deepEqual.js
"Number: When create same parameter should be equal."
);
assert.equal( QUnit.equiv(new SafeNumber(1), new SafeNumber(2) ), false,
"Number: When create different parameter should be not equal."

This comment has been minimized.

@leobalter

leobalter Dec 15, 2015

Member

Maybe replace the assertion message with:

"Number objects with different values are not equivalent" or "QUnit.equiv returns false for Number objects with different values"

@leobalter

leobalter Dec 15, 2015

Member

Maybe replace the assertion message with:

"Number objects with different values are not equivalent" or "QUnit.equiv returns false for Number objects with different values"

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 15, 2015

Member

I know I commented on several details, but this is a great work adding a great value to QUnit.equiv.

I'm looking forward to merge this PR. Thanks, @mixed!

Member

leobalter commented Dec 15, 2015

I know I commented on several details, but this is a great work adding a great value to QUnit.equiv.

I'm looking forward to merge this PR. Thanks, @mixed!

@mixed

This comment has been minimized.

Show comment
Hide comment
@mixed

mixed Dec 15, 2015

Contributor

@leobalter
Thank you. Good review for me.

Contributor

mixed commented Dec 15, 2015

@leobalter
Thank you. Good review for me.

Show outdated Hide outdated test/main/deepEqual.js
@@ -1581,6 +1581,39 @@ QUnit.test( "Test that must be done at the end because they extend some primitiv
}
);
QUnit.test( "Compare Number values", function( assert ) {

This comment has been minimized.

@leobalter

leobalter Dec 16, 2015

Member

@mixed: would you create a single module on the top of these tests called "Compare primitive values"?

Example:

QUnit.module( "Compare primitive values" );
@leobalter

leobalter Dec 16, 2015

Member

@mixed: would you create a single module on the top of these tests called "Compare primitive values"?

Example:

QUnit.module( "Compare primitive values" );
Show outdated Hide outdated src/equiv.js
@@ -10,10 +10,11 @@ QUnit.equiv = (function() {
var parentsB = [];
function useStrictEquality( b, a ) {

This comment has been minimized.

@leobalter

leobalter Dec 16, 2015

Member

this blank line should be kept, all comments (even the jshint ones) should be preceded by a blank line.

@leobalter

leobalter Dec 16, 2015

Member

this blank line should be kept, all comments (even the jshint ones) should be preceded by a blank line.

Show outdated Hide outdated src/equiv.js
if ( b instanceof a.constructor || a instanceof b.constructor ) {
if ( b instanceof a.constructor && a instanceof b.constructor ) {
// Compare same constructor.

This comment has been minimized.

@leobalter

leobalter Dec 16, 2015

Member

missing a preceding blank line here

@leobalter

leobalter Dec 16, 2015

Member

missing a preceding blank line here

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 16, 2015

Member

We're close!

Member

leobalter commented Dec 16, 2015

We're close!

Show outdated Hide outdated src/equiv.js
// Compare same constructor.
return a.toString() == b.toString();

This comment has been minimized.

@gibson042

gibson042 Dec 20, 2015

Member

I think this could be generalized and simplified a bit further, to something like

if ( typeof a === "object" ) {
    a = a.valueOf();
}
if ( typeof b === "object" ) {
    b = b.valueOf();
}
return a === b;
@gibson042

gibson042 Dec 20, 2015

Member

I think this could be generalized and simplified a bit further, to something like

if ( typeof a === "object" ) {
    a = a.valueOf();
}
if ( typeof b === "object" ) {
    b = b.valueOf();
}
return a === b;

This comment has been minimized.

@mixed

mixed Dec 20, 2015

Contributor

@gibson042
Thank you. I think of it again. I think this don't have to check type. Could you please explain to me more detail about check type? And How about think below logic?

function useStrictEquality( b, a ) {
    if ( b instanceof a.constructor || a instanceof b.constructor ) {
        return a.valueOf() === b.valueOf();
    } else {
        return a === b;
    }
}
@mixed

mixed Dec 20, 2015

Contributor

@gibson042
Thank you. I think of it again. I think this don't have to check type. Could you please explain to me more detail about check type? And How about think below logic?

function useStrictEquality( b, a ) {
    if ( b instanceof a.constructor || a instanceof b.constructor ) {
        return a.valueOf() === b.valueOf();
    } else {
        return a === b;
    }
}

This comment has been minimized.

@gibson042

gibson042 Dec 21, 2015

Member

Well, there are two issues. First, and most importantly, we should probably have an automatic return true in a separate guaranteed function for null, undefined, and nan "types". Having cleared those out of the way, there would be a few options for detecting wrapped primitives, and I like typeof object because it strikes me as the most semantic. And separate from all of that, it's just nice to see a single return with uniform comparison logic.

@gibson042

gibson042 Dec 21, 2015

Member

Well, there are two issues. First, and most importantly, we should probably have an automatic return true in a separate guaranteed function for null, undefined, and nan "types". Having cleared those out of the way, there would be a few options for detecting wrapped primitives, and I like typeof object because it strikes me as the most semantic. And separate from all of that, it's just nice to see a single return with uniform comparison logic.

This comment has been minimized.

@mixed

mixed Dec 21, 2015

Contributor

@gibson042

Thank you. :)
I have misunderstood it. but I get it now. I think that check 'constructor' and check 'typeof' before I understand your comment.

Good comments. I updating it soon.

@mixed

mixed Dec 21, 2015

Contributor

@gibson042

Thank you. :)
I have misunderstood it. but I get it now. I think that check 'constructor' and check 'typeof' before I understand your comment.

Good comments. I updating it soon.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 29, 2015

Member

@gibson042 can we merge this?

Member

leobalter commented Dec 29, 2015

@gibson042 can we merge this?

@gibson042 gibson042 closed this in f2c0fc3 Dec 30, 2015

@leobalter leobalter removed the in progress label Dec 30, 2015

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Dec 30, 2015

Member

I increased test coverage a little and restored an explanatory comment, but yes! 😀

Member

gibson042 commented Dec 30, 2015

I increased test coverage a little and restored an explanatory comment, but yes! 😀

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 30, 2015

Member

This is awesome! thanks, @mixed and @gibson042

Member

leobalter commented Dec 30, 2015

This is awesome! thanks, @mixed and @gibson042

flore77 pushed a commit to flore77/qunit that referenced this pull request Aug 10, 2016

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