Skip to content
Permalink
Browse files
Tests: Avoid use of QUnit.reset() in tests by splitting them
Fix #14040
Close gh-1457
  • Loading branch information
cjqed authored and dmethvin committed Dec 23, 2013
1 parent 23db994 commit 537e9ce
Show file tree
Hide file tree
Showing 8 changed files with 545 additions and 225 deletions.
@@ -213,3 +213,4 @@ Ilya Kantor <iliakan@gmail.com>
hongymagic <d.hong@me.com>
John Paul <john@johnkpaul.com>
Jakob Stoeck <jakob@pokermania.de>
Christopher Jones <chris@cjqed.com>
@@ -11,9 +11,8 @@ var oldStart = window.start,
oldActive = 0,

expectedDataKeys = {},

reset,
splice = [].splice,
reset = QUnit.reset,
ajaxSettings = jQuery.ajaxSettings;


@@ -157,8 +156,6 @@ window.moduleTeardown = function() {
oldActive = jQuery.active;
}

// Allow QUnit.reset to clean up any attached elements before checking for leaks
QUnit.reset();

for ( i in jQuery.cache ) {
++cacheLength;
@@ -187,8 +184,8 @@ QUnit.done(function() {
supportjQuery("#qunit ~ *").remove();
});

// jQuery-specific QUnit.reset
QUnit.reset = function() {
// jQuery-specific post-test cleanup
reset = function () {

// Ensure jQuery events and data on the fixture are properly removed
jQuery("#qunit-fixture").empty();
@@ -205,11 +202,11 @@ QUnit.reset = function() {

// Cleanup globals
Globals.cleanup();

// Let QUnit reset the fixture
reset.apply( this, arguments );
jQuery("#qunit-fixture")[0].innerHTML = QUnit.config.fixture;
};

QUnit.testDone(reset);

// Register globals for cleanup and the cleanup code itself
// Explanation at http://perfectionkills.com/understanding-delete/#ie_bugs
window.Globals = (function() {
@@ -351,12 +348,11 @@ function testSubproject( label, subProjectURL, risTests, complete ) {

// WARNING: UNDOCUMENTED INTERFACE
QUnit.config.fixture = fixtureHTML;
QUnit.reset();
reset();
if ( supportjQuery("#qunit-fixture").html() !== fixtureHTML ) {
ok( false, "Copied subproject fixture" );
return;
}

fixtureReplaced = true;
}

@@ -635,7 +635,8 @@ test( "removeAttr(Multi String, variable space width)", function() {
});

test( "prop(String, Object)", function() {
expect( 31 );

expect( 17 );

equal( jQuery("#text1").prop("value"), "Test", "Check for value attribute" );
equal( jQuery("#text1").prop( "value", "Test2" ).prop("defaultValue"), "Test", "Check for defaultValue attribute" );
@@ -663,7 +664,11 @@ test( "prop(String, Object)", function() {
equal( jQuery("#table").prop("useMap"), 1, "Check setting and retrieving useMap" );
jQuery("#table").prop( "frameborder", 1 );
equal( jQuery("#table").prop("frameBorder"), 1, "Check setting and retrieving frameBorder" );
QUnit.reset();
});

test( "prop(String, Object) on null/undefined", function() {

expect( 14 );

var select, optgroup, option, attributeNode, commentNode, textNode, obj, $form,
body = document.body,
@@ -795,16 +800,20 @@ test( "removeProp(String)", function() {
});
});

test( "val()", function() {
expect( 21 + ( jQuery.fn.serialize ? 6 : 0 ) );
test( "val() after modification", function() {

var checks, $button;
expect( 1 );

document.getElementById("text1").value = "bla";
equal( jQuery("#text1").val(), "bla", "Check for modified value of input element" );
});


QUnit.reset();
test( "val()", function() {

expect( 20 + ( jQuery.fn.serialize ? 6 : 0 ) );

var checks, $button;
equal( jQuery("#text1").val(), "Test", "Check for value of input element" );
// ticket #1714 this caused a JS error in IE
equal( jQuery("#first").val(), "", "Check a paragraph element to see if it has a value" );
@@ -919,7 +928,6 @@ if ( "value" in document.createElement("meter") &&
var testVal = function( valueObj ) {
expect( 8 );

QUnit.reset();
jQuery("#text1").val( valueObj("test") );
equal( document.getElementById("text1").value, "test", "Check for modified (via val(String)) value of input element" );

@@ -974,7 +982,6 @@ test( "val(Array of Numbers) (Bug #7123)", function() {
test( "val(Function) with incoming value", function() {
expect( 10 );

QUnit.reset();
var oldVal = jQuery("#text1").val();

jQuery("#text1").val(function( i, val ) {
@@ -587,7 +587,6 @@ test("jQuery('html')", function() {

var s, div, j;

QUnit.reset();
jQuery["foo"] = false;
s = jQuery("<script>jQuery.foo='test';</script>")[0];
ok( s, "Creating a script" );
@@ -603,7 +602,6 @@ test("jQuery('html')", function() {
equal( div.childNodes[1].nodeType, 1, "Paragraph." );
equal( div.childNodes[1].firstChild.nodeType, 3, "Paragraph text." );

QUnit.reset();
ok( jQuery("<link rel='stylesheet'/>")[0], "Creating a link" );

ok( !jQuery("<script/>")[0].parentNode, "Create a script" );
@@ -391,9 +391,10 @@ test("css(Object) where values are Functions with incoming values", function() {
});

test("show(); hide()", function() {
expect(22);

var hiddendiv, div, pass, old, test;
expect( 4 );

var hiddendiv, div;

hiddendiv = jQuery("div.hidden");
hiddendiv.hide();
@@ -406,8 +407,13 @@ test("show(); hide()", function() {
div.appendTo("#qunit-fixture").show();
equal( div.css("display"), "block", "Pre-hidden div shown" );

QUnit.reset();
});

test("show();", function() {

expect( 18 );

var hiddendiv, div, pass, old, test;
hiddendiv = jQuery("div.hidden");

equal(jQuery.css( hiddendiv[0], "display"), "none", "hiddendiv is display: none");
@@ -620,7 +626,6 @@ test("toggle()", function() {

test("hide hidden elements (bug #7141)", function() {
expect(3);
QUnit.reset();

var div = jQuery("<div style='display:none'></div>").appendTo("#qunit-fixture");
equal( div.css("display"), "none", "Element is hidden by default" );
@@ -130,7 +130,6 @@ test("show()", 27, function () {

test("show(Number) - other displays", function() {
expect(15);
QUnit.reset();

// #show-tests * is set display: none in CSS
jQuery("#qunit-fixture").append("<div id='show-tests'><div><p><a href='#'></a></p><code></code><pre></pre><span></span></div><table><thead><tr><th></th></tr></thead><tbody><tr><td></td></tr></tbody></table><ul><li></li></ul></div><table id='test-table'></table>");
@@ -1221,7 +1220,6 @@ test("animate with CSS shorthand properties", function(){

test("hide hidden elements, with animation (bug #7141)", function() {
expect(3);
QUnit.reset();

var div = jQuery("<div style='display:none'></div>").appendTo("#qunit-fixture");
equal( div.css("display"), "none", "Element is hidden by default" );

7 comments on commit 537e9ce

@markelog
Copy link
Member

Choose a reason for hiding this comment

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

@dmethvin

Fix
that's an imperative mood, but our guide dictates we should use indicative, which i think is a bug, but we already could see this inconsistency arising (this one also have dot in the end of commit body and header).

/cc @timmywil, @jzaefferer

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

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

Just an oversight from habit on my part. When we were trying to stuff it all on one line it made sense to use imperative since it saves a few characters. If we're all good with what's in the guide I'll just use that.

@markelog
Copy link
Member

Choose a reason for hiding this comment

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

@dmethvin not just yours. The reason why i think we should use imperative mood is because commands like git merge and git revert also generates it that way.

@markelog
Copy link
Member

Choose a reason for hiding this comment

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

hm, but on second thought, git revert uses indicative mood in the commit body and imperative in the head

@mgol
Copy link
Member

@mgol mgol commented on 537e9ce Dec 27, 2013

Choose a reason for hiding this comment

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

@markelog Probably because imperative is shorter and with 72 char limit every char is precious.

@gibson042
Copy link
Member

Choose a reason for hiding this comment

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

I thought we agreed on the imperative in jquery/contribute.jquery.org#61, but a reread shows that it was only ever raised specifically for the commit subject, and implicitly rebutted in the body by our "Fixes"/"Closes" examples. Regardless, it was never actually incorporated.

I'll update my future messages to match our documented format, but for the record I agree with @markelog on preferring imperative mood everywhere.

@timmywil
Copy link
Member

Choose a reason for hiding this comment

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

I prefer indicative for Fixes/Closes. I'm not sure why. I guess "this commit fixes this ticket" flows better than "hey commit, fix the ticket". I don't think it matters that much though.

Please sign in to comment.