Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
No ticket: improve global variable/ajax request tracking
  • Loading branch information
@gibson042
gibson042 committed Dec 3, 2012
1 parent 27c9360 commit 5b9bf13
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 54 deletions.
1 change: 0 additions & 1 deletion test/.jshintrc
View file
Expand Up @@ -45,7 +45,6 @@
"createXMLFragment": true,
"moduleTeardown": true,
"testFoo": true,
"foobar": true,
"url": true,
"t": true,
"q": true,
Expand Down
2 changes: 1 addition & 1 deletion test/data/test.js
View file
@@ -1,3 +1,3 @@
var foobar = "bar";
var testBar = "bar";
jQuery('#ap').html('bar');
ok( true, "test.js executed");
55 changes: 31 additions & 24 deletions test/data/testinit.js
View file
Expand Up @@ -158,39 +158,46 @@ function ajaxTest( title, expect, options ) {
if ( options.setup ) {
options.setup();
}
var aborted = false,
abort = function( reason ) {
if ( !aborted ) {
aborted = true;

var completed = false,
complete = function() {
completed = true;
delete ajaxTest.abort;
},
abort = ajaxTest.abort = function( reason ) {
if ( !completed ) {
complete();
ok( false, "unexpected " + reason );
jQuery.each( requests, function( _, request ) {
request.abort();
if ( request && request.abort ) {
request.abort();
}
});
}
},
requests = jQuery.map( requestOptions, function( options ) {
var request = ( options.create || jQuery.ajax )( options );
requests = jQuery.map( requestOptions, function( options, index ) {
var request = ( options.create || jQuery.ajax )( options ),
callIfDefined = function( deferType, optionType ) {
var handler = options[ deferType ] || !!options[ optionType ];
return handler ?
function() {
if ( !completed && jQuery.isFunction( handler ) ) {
handler.apply( this, arguments );
}
} :
function() {
abort( optionType );
};
};

if ( options.afterSend ) {
options.afterSend( request );
}
return request;

return request.then( callIfDefined( "done", "success" ), callIfDefined( "fail", "error" ) );

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Dec 3, 2012

Member

With this change, requests will never be aborted unless one of them immediately responds. If they don't immediately respond, by the time this code is hit, all requests have been "thened" and have no abort method. So if something goes wrong in an ajax test, we'll have dangling ajax requests potentially issueing asserts in the following tests (which is what the initial code was solving by separating the ajax request from the "thened" promise).

Unless I'm missing something, I don't really see the improvement here.

This comment has been minimized.

Sign in to view

Copy link
@gibson042

gibson042 Dec 3, 2012

Author Member

Shoot, you're right. That was supposed to be .done + .fail.
});
jQuery.when.apply( jQuery, jQuery.map( requests, function( request, index ) {
function callIfDefined( deferType, optionType ) {
var handler = requestOptions[ index ][ deferType ] || !!requestOptions[ index ][ optionType ];
return handler ? function() {
if ( !aborted && jQuery.isFunction( handler ) ) {
handler.apply( this, arguments );
}
} : function() {
abort( optionType );
}
}
return request.then( callIfDefined( "done", "success" ), callIfDefined( "fail", "error" ) );
}) ).always(
options.teardown,
start
);

jQuery.when.apply( jQuery, requests ).always( complete, options.teardown, start);
});
};

Expand Down
16 changes: 8 additions & 8 deletions test/data/testrunner.js
View file
Expand Up @@ -3,10 +3,10 @@
*/
jQuery.noConflict();

// For checking globals pollution
window[ jQuery.expando ] = undefined;
// ...in Gecko
this.getInterface = this.getInterface;
// For checking globals pollution despite auto-created globals in various environments
jQuery.each( [ jQuery.expando, "getInterface", "Packages", "java", "netscape" ], function( i, name ) {
window[ name ] = window[ name ];
});

// Expose Sizzle for Sizzle's selector tests
// We remove Sizzle's globalization in jQuery
Expand Down Expand Up @@ -138,7 +138,7 @@ function testSubproject( label, url, risTests ) {
// Explanation at http://perfectionkills.com/understanding-delete/#ie_bugs
var Globals = (function() {
var globals = {};
return QUnit.config.noglobals ? {
return {

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Dec 3, 2012

Member

My only concern is that testSwarm will pass its time global-evaling variable deletes, hence why there was a no-op imp. Couldn't the no-op just have set the variable to undefined when registering and be done with it or am I over-estimating the strain this would take on old IEs. I just want to limit testSwarm timeouts as much as possible but I have no perf data or anything, mind you ;)

This comment has been minimized.

Sign in to view

Copy link
@gibson042

gibson042 Dec 3, 2012

Author Member

Actually, I'm thinking we need a set to undefined AND a delete. It doesn't take that much time (and this coming from someone very concerned about how long our tests take), but even if it did, the ajax suite in particular is so sensitive to globals management that I consider it necessary.

EDIT: But if that proves untenable, set to undefined would definitely be the fallback.
register: function( name ) {
globals[ name ] = true;
jQuery.globalEval( "var " + name );
Expand All @@ -153,9 +153,6 @@ var Globals = (function() {
"; } catch( x ) {}" );
}
}
} : {
register: jQuery.noop,
cleanup: jQuery.noop
};
})();

Expand Down Expand Up @@ -312,6 +309,9 @@ var Globals = (function() {
}
if ( jQuery.active !== undefined && jQuery.active !== oldActive ) {
equal( jQuery.active, 0, "No AJAX requests are still active" );
if ( ajaxTest.abort ) {
ajaxTest.abort("active request");
}

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Dec 3, 2012

Member

Is this worth the complexity of exposing a global abort method? What's the use-case?

This comment has been minimized.

Sign in to view

Copy link
@gibson042

gibson042 Dec 3, 2012

Author Member

I've been seeing HEAD request tests timeout in BrowserStack, and wanted to guarantee that they wrapped up cleanly.

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Dec 3, 2012

Member

Global QUnit timeout, got you.
oldActive = jQuery.active;
}
};
Expand Down
33 changes: 26 additions & 7 deletions test/unit/ajax.js
View file
Expand Up @@ -533,15 +533,15 @@ module( "ajax", {
ajaxTest( "jQuery.ajax() - dataType html", 5, {
setup: function() {
Globals.register("testFoo");
Globals.register("foobar");
Globals.register("testBar");
},
dataType: "html",
url: url("data/test.html"),
success: function( data ) {
ok( data.match( /^html text/ ), "Check content for datatype html" );
jQuery("#ap").html( data );
strictEqual( window["testFoo"], "foo", "Check if script was evaluated for datatype html" );
strictEqual( window["foobar"], "bar", "Check if script src was evaluated for datatype html" );
strictEqual( window["testBar"], "bar", "Check if script src was evaluated for datatype html" );
}
});

Expand Down Expand Up @@ -588,6 +588,7 @@ module( "ajax", {
jQuery( document ).off("ajaxError.passthru");
start();
});
Globals.register("testBar");

ok( jQuery.get( url(target), success ), "get" );
ok( jQuery.post( url(target), success ), "post" );
Expand Down Expand Up @@ -836,28 +837,37 @@ module( "ajax", {
});

ajaxTest( "jQuery.ajax() - script, Remote", 2, {
setup: function() {
Globals.register("testBar");
},
url: window.location.href.replace( /[^\/]*$/, "" ) + "data/test.js",
dataType: "script",
success: function( data ) {
ok( window[ "foobar" ], "Script results returned (GET, no callback)" );
strictEqual( window["testBar"], "bar", "Script results returned (GET, no callback)" );
}
});

ajaxTest( "jQuery.ajax() - script, Remote with POST", 3, {
setup: function() {
Globals.register("testBar");
},
url: window.location.href.replace( /[^\/]*$/, "" ) + "data/test.js",
type: "POST",
dataType: "script",
success: function( data, status ) {
ok( window[ "foobar" ], "Script results returned (POST, no callback)" );
strictEqual( window["testBar"], "bar", "Script results returned (POST, no callback)" );
strictEqual( status, "success", "Script results returned (POST, no callback)" );
}
});

ajaxTest( "jQuery.ajax() - script, Remote with scheme-less URL", 2, {
setup: function() {
Globals.register("testBar");
},
url: window.location.href.replace( /[^\/]*$/, "" ).replace( /^.*?\/\//, "//" ) + "data/test.js",
dataType: "script",
success: function( data ) {
ok( window[ "foobar" ], "Script results returned (GET, no callback)" );
strictEqual( window["testBar"], "bar", "Script results returned (GET, no callback)" );
}
});

Expand Down Expand Up @@ -1655,17 +1665,20 @@ module( "ajax", {
//----------- jQuery.getScript()

asyncTest( "jQuery.getScript( String, Function ) - with callback", 2, function() {
Globals.register("testBar");
jQuery.getScript( url("data/test.js"), function( data, _, jqXHR ) {
strictEqual( foobar, "bar", "Check if script was evaluated" );
strictEqual( window["testBar"], "bar", "Check if script was evaluated" );
start();
});
});

asyncTest( "jQuery.getScript( String, Function ) - no callback", 1, function() {
Globals.register("testBar");
jQuery.getScript( url("data/test.js") ).done( start );
});

asyncTest( "#8082 - jQuery.getScript( String, Function ) - source as responseText", 2, function() {
Globals.register("testBar");
jQuery.getScript( url("data/test.js"), function( data, _, jqXHR ) {
strictEqual( data, jqXHR.responseText, "Same-domain script requests returns the source of the script" );
start();
Expand Down Expand Up @@ -1729,10 +1742,14 @@ module( "ajax", {

asyncTest( "jQuery.fn.load( String, Function ) - check scripts", 7, function() {
var verifyEvaluation = function() {
strictEqual( window["foobar"], "bar", "Check if script src was evaluated after load" );
strictEqual( window["testBar"], "bar", "Check if script src was evaluated after load" );
strictEqual( jQuery("#ap").html(), "bar", "Check if script evaluation has modified DOM");
start();
};

Globals.register("testFoo");
Globals.register("testBar");

jQuery("#first").load( url("data/test.html"), function() {
ok( jQuery("#first").html().match( /^html text/ ), "Check content after loading html" );
strictEqual( jQuery("#foo").html(), "foo", "Check if script evaluation has modified DOM" );
Expand All @@ -1742,6 +1759,8 @@ module( "ajax", {
});

asyncTest( "jQuery.fn.load( String, Function ) - check file with only a script tag", 3, function() {
Globals.register("testFoo");

jQuery("#first").load( url("data/test2.html"), function() {
strictEqual( jQuery("#foo").html(), "foo", "Check if script evaluation has modified DOM");
strictEqual( window["testFoo"], "foo", "Check if script was evaluated after load" );
Expand Down
3 changes: 1 addition & 2 deletions test/unit/core.js
View file
Expand Up @@ -195,6 +195,7 @@ test( "selector state", function() {

test( "globalEval", function() {
expect( 3 );
Globals.register("globalEvalTest");

This comment has been minimized.

Sign in to view

Copy link
@jaubourg

jaubourg Dec 3, 2012

Member

OMG it's spreading! Oo ;)

jQuery.globalEval("globalEvalTest = 1;");
equal( window.globalEvalTest, 1, "Test variable assignments are global" );
Expand All @@ -204,8 +205,6 @@ test( "globalEval", function() {

jQuery.globalEval("this.globalEvalTest = 3;");
equal( window.globalEvalTest, 3, "Test context (this) is the window object" );

jQuery.globalEval("delete globalEvalTest;");
});

test("noConflict", function() {
Expand Down
12 changes: 1 addition & 11 deletions test/unit/manipulation.js
View file
Expand Up @@ -704,7 +704,7 @@ test("append(xml)", function() {
});

test("appendTo(String|Element|Array<Element>|jQuery)", function() {
expect( 16 + ( jQuery.getScript ? 1 : 0 ) );
expect( 16 );

var defaultText = "Try them out:";

Expand Down Expand Up @@ -775,16 +775,6 @@ test("appendTo(String|Element|Array<Element>|jQuery)", function() {
div.remove().appendTo("#qunit-fixture");

equal( jQuery("#qunit-fixture div").length, num, "Make sure all the removed divs were inserted." );

QUnit.reset();

if ( jQuery.getScript ) {
stop();
jQuery.getScript("data/test.js", function() {
jQuery("script[src*='data\\/test\\.js']").remove();
start();
});
}
});

var testPrepend = function(val) {
Expand Down

0 comments on commit 5b9bf13

Please sign in to comment.