Permalink
Browse files

Tests: Provide equal() arguments in correct order (actual, expected)

  • Loading branch information...
gibson042 committed Sep 8, 2015
1 parent 0e98243 commit d3d8d9751f3d14a545b26cf820dc1f51896a7b50
Showing with 13 additions and 12 deletions.
  1. +13 −12 test/unit/data.js
View
@@ -824,25 +824,25 @@ QUnit.test( "acceptData", function( assert ) {
var flash, pdf, form;
assert.equal( 42, jQuery( document ).data( "test", 42 ).data( "test" ), "document" );
assert.equal( 42, jQuery( document.documentElement ).data( "test", 42 ).data( "test" ), "documentElement" );
assert.equal( 42, jQuery( {} ).data( "test", 42 ).data( "test" ), "object" );
assert.equal( 42, jQuery( document.createElement( "embed" ) ).data( "test", 42 ).data( "test" ), "embed" );
assert.equal( jQuery( document ).data( "test", 42 ).data( "test" ), 42, "document" );
assert.equal( jQuery( document.documentElement ).data( "test", 42 ).data( "test" ), 42, "documentElement" );
assert.equal( jQuery( {} ).data( "test", 42 ).data( "test" ), 42, "object" );
assert.equal( jQuery( document.createElement( "embed" ) ).data( "test", 42 ).data( "test" ), 42, "embed" );
flash = document.createElement( "object" );
flash.setAttribute( "classid", "clsid:D27CDB6E-AE6D-11cf-96B8-444553540000" );
assert.equal( 42, jQuery( flash ).data( "test", 42 ).data( "test" ), "flash" );
assert.equal( jQuery( flash ).data( "test", 42 ).data( "test" ), 42, "flash" );
pdf = document.createElement( "object" );
pdf.setAttribute( "classid", "clsid:CA8A9780-280D-11CF-A24D-444553540000" );
assert.equal( 42, jQuery( pdf ).data( "test", 42 ).data( "test" ), "pdf" );
assert.equal( jQuery( pdf ).data( "test", 42 ).data( "test" ), 42, "pdf" );
assert.equal( undefined, jQuery( document.createComment( "" ) ).data( "test", 42 ).data( "test" ), "comment" );
assert.equal( undefined, jQuery( document.createTextNode( "" ) ).data( "test", 42 ).data( "test" ), "text" );
assert.equal( undefined, jQuery( document.createDocumentFragment() ).data( "test", 42 ).data( "test" ), "documentFragment" );
assert.strictEqual( jQuery( document.createComment( "" ) ).data( "test", 42 ).data( "test" ), undefined, "comment" );
assert.strictEqual( jQuery( document.createTextNode( "" ) ).data( "test", 42 ).data( "test" ), undefined, "text" );
assert.strictEqual( jQuery( document.createDocumentFragment() ).data( "test", 42 ).data( "test" ), undefined, "documentFragment" );
form = jQuery( "#form" ).append( "<input id='nodeType'/><input id='nodeName'/>" )[ 0 ];
assert.equal( 42, jQuery( form ) .data( "test", 42 ).data( "test" ), "form with aliased DOM properties" );
assert.equal( jQuery( form ) .data( "test", 42 ).data( "test" ), 42, "form with aliased DOM properties" );
} );
QUnit.test( "Check proper data removal of non-element descendants nodes (#8335)", function( assert ) {
@@ -906,8 +906,9 @@ QUnit.test( ".data(prop) does not create expando", function( assert ) {
var key,
div = jQuery( "<div/>" );
div.data("foo");
assert.equal( false, jQuery.hasData( div[0] ) );
div.data( "foo" );
assert.equal( jQuery.hasData( div[ 0 ] ), false, "No data exists after access" );
// Make sure no expando has been added
for ( key in div[ 0 ] ) {
if ( /^jQuery/.test( key ) ) {

10 comments on commit d3d8d97

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 8, 2015

Member

I felt something's not 100% right with this commit...

Member

mgol replied Sep 8, 2015

I felt something's not 100% right with this commit...

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Sep 8, 2015

Member

No biggie.

It is better though, to give a contributor an opportunity to correct himself, although here, we changing other unrelated parts of the commit.

Member

markelog replied Sep 8, 2015

No biggie.

It is better though, to give a contributor an opportunity to correct himself, although here, we changing other unrelated parts of the commit.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Sep 9, 2015

Member

In this case, the original contribution landed without any of us catching the mistake. Combine that with the difference being so slight (invisible while the tests pass, in fact), and it seemed prudent to just update accordingly.

Member

gibson042 replied Sep 9, 2015

In this case, the original contribution landed without any of us catching the mistake. Combine that with the difference being so slight (invisible while the tests pass, in fact), and it seemed prudent to just update accordingly.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Sep 9, 2015

Contributor

Opps. Thanks for catching that!

Contributor

jbedard replied Sep 9, 2015

Opps. Thanks for catching that!

@FagnerMartinsBrack

This comment has been minimized.

Show comment
Hide comment
@FagnerMartinsBrack

FagnerMartinsBrack Sep 9, 2015

I would argue that QUnit should have some kind of syntax like:

strictEqual({
  expected: 42,
  actual: jQuery( form ) .data( "test", 42 ).data( "test" )
}, "form with aliased DOM properties");

This arguments confusion seems pretty common, since a reader cannot infer the arguments order from the calling code upon review. Also, for those who also work with JUnit (Java) in a day to day basis will be confused by the fact that the arguments are inverted.

@jzaefferer is it worth creating an issue in QUnit, or this has been discussed already? I couldn't find it.

FagnerMartinsBrack replied Sep 9, 2015

I would argue that QUnit should have some kind of syntax like:

strictEqual({
  expected: 42,
  actual: jQuery( form ) .data( "test", 42 ).data( "test" )
}, "form with aliased DOM properties");

This arguments confusion seems pretty common, since a reader cannot infer the arguments order from the calling code upon review. Also, for those who also work with JUnit (Java) in a day to day basis will be confused by the fact that the arguments are inverted.

@jzaefferer is it worth creating an issue in QUnit, or this has been discussed already? I couldn't find it.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 9, 2015

Member

I don't think its worth another signature, unless you want to try and propagate that to other libraries as well, like node's assert. A check in jshint or eslint might make sense, if its possible to reliably detect this issue.

Member

jzaefferer replied Sep 9, 2015

I don't think its worth another signature, unless you want to try and propagate that to other libraries as well, like node's assert. A check in jshint or eslint might make sense, if its possible to reliably detect this issue.

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Sep 9, 2015

Contributor

@jzaefferer I guess the only thing which can be reliably checked for via a lint tool is that the "actual"-value isnt passed as a constant. the "expected" value is most of the time a constant, but sometimes also expression are used.

Contributor

staabm replied Sep 9, 2015

@jzaefferer I guess the only thing which can be reliably checked for via a lint tool is that the "actual"-value isnt passed as a constant. the "expected" value is most of the time a constant, but sometimes also expression are used.

@FagnerMartinsBrack

This comment has been minimized.

Show comment
Hide comment
@FagnerMartinsBrack

FagnerMartinsBrack Sep 9, 2015

... the "expected" value is most of the time a constant, but sometimes also expression are used.

Sometimes I end up doing things like this to document the API for the reader:

var actual = fn();
var expected = 42;
assert.strictEqual( actual, expected );

Here is an external example of the "expected" argument as an expression.

FagnerMartinsBrack replied Sep 9, 2015

... the "expected" value is most of the time a constant, but sometimes also expression are used.

Sometimes I end up doing things like this to document the API for the reader:

var actual = fn();
var expected = 42;
assert.strictEqual( actual, expected );

Here is an external example of the "expected" argument as an expression.

@staabm

This comment has been minimized.

Show comment
Hide comment
@staabm

staabm Sep 9, 2015

Contributor

Here is an external example of the "expected" argument as an expression.

right, therefore my argument was, the only thing we can check is, that the actual argument cannot be a constant.

Contributor

staabm replied Sep 9, 2015

Here is an external example of the "expected" argument as an expression.

right, therefore my argument was, the only thing we can check is, that the actual argument cannot be a constant.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 21, 2015

Member

the only thing we can check is, that the actual argument cannot be a constant.

That sounds useful to me. It would've caught the assertions this commit addresses. Anyone interested in implementing that in a tool?

Member

jzaefferer replied Sep 21, 2015

the only thing we can check is, that the actual argument cannot be a constant.

That sounds useful to me. It would've caught the assertions this commit addresses. Anyone interested in implementing that in a tool?

Please sign in to comment.