This should also cover the test for bug 312661

(In reply to comment #0) > test for bug 312661 See my Bv0 patch there for cases to check...

I may not get to this soon so if anyone wants to run with it please do so.

Often times the download of the xml completes successfully when there are errors.

Comment on attachment 334643 [details] [diff] [review] patch Maybe rename run_test_helper_pt1 to run_test_helper and same for check_test_helper_pt1 since they are used for all parts of the test, but otherwise looks fine.

This includes a bare bones implementation of XMLHttpRequest so the test can return status values we can't simulate with XMLHttpRequest and the test httpd. Where ever possible it uses the actual XMLHttpRequest implementation.

Carrying forward review

Leaving open to get the additional tests in

Comment on attachment 335286 [details] [diff] [review] original patch with nits fixed (checked in) Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/index.cgi/rev/8d77d65919bf48480f249ddcbc068cbb1308e38c

Comment on attachment 335286 [details] [diff] [review] original patch with nits fixed (checked in) >diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in >+ } >+ LOG("Checker", "status: " + status); >+ if (status == 0) Nit: LOG line is misaligned.

Thanks Serge... removed.

(In reply to comment #12) > Thanks Serge... removed. http://hg.mozilla.org/mozilla-central/index.cgi/rev/6b7caf279be039a5cef0180e623b150a7e6580d1

Dave, this is essentially the same patch that also fixes bug 312661 since the tests are from this bug.

Marking this fixed though and will land the additional tests once they are approved

Comment on attachment 335287 [details] [diff] [review] additional tests These tests don't actually run here. >+function overrideXHR(callback) { >+ gXHR = new xhr(); >+ var registrar = Components.manager.QueryInterface(AUS_Ci.nsIComponentRegistrar); >+ registrar.registerFactory(xhr.prototype.classID, xhr.prototype.classDescription, >+ xhr.prototype.contractID, gXHR); >+ gXHR.callback = callback; >+} Here you set callback on a new xhr object. But the createInstance method in xhr returns another new instance, which won't have callback defined. You could maybe set xhr.prototype.callback, or make createInstance always return gXHR. Both will have issues if any code under test tries to use two xhr's simultaneously though I think. >+ >+/** >+ * Bare bones XMLHttpRequest implementation for testing onprogress, onerror, >+ * and onload nsIDomEventListener handleEvent. >+ */ >+function xhr() { >+} >+xhr.prototype = { >+ callback: null, >+ overrideMimeType: function(mimetype) { }, >+ setRequestHeader: function(header, value) { }, >+ status: null, >+ channel: { set notificationCallbacks(val) { } }, >+ _utl: null, I think you meant _url >+ _method: null, >+ open: function (method, url) { this._method = method; this._url = url; }, >+ send: function(body) { >+ gXHR = this; >+ do_timeout(0, "callback()"); // Use a timeout so the XHR completes >+ }, This is kinda ugly, if two xhr's are created and run in quick succession then the second will be the global gXHR when the callback is executed for the first. Also callback is not a defined function anywhere, possibly you wanted "gXHR.callback()". >+function run_test() { >+ do_test_pending(); >+ startAUS(); >+ overrideXHR(callHanleEvent); callHandleEvent ;) >+// Callback function used by the custom XMLHttpRequest implemetation to >+// call the nsIDOMEventListener's handleEvent method for onload. >+function callHanleEvent() { >+ gXHR.status = gExpectedStatus; gExpectedStatus is undeclared

> This is kinda ugly, if two xhr's are created and run in quick succession then > the second will be the global gXHR when the callback is executed for the first. I don't think these or any of the update tests will ever need to be concerned with using two xhr's using a custom xhr implementation... if they do in the future then it can be rewritten to support it though I bet it could use the real implementation. > These tests don't actually run here. Could you attach the log output from the original patch? It was succeeding on my system (though it finishes much faster likely due to the custom xhr) as can be seen by the onError callbacks to the listener as follows. Problems with the original test aside I'm curious why it would fail on your system and not mine. Testing: failed (unknown reason) - http://localhost:4444/data/aus-0051_general-1.xml onError: request.status = 2152398849, update.statusText = Failed (Unknown Reason)

I was just getting: *** test pending *** test pending *** test finished *** running event loop Testing: failed (unknown reason) - http://localhost:4444/data/aus-0051_general-1.xml then it would sit there waiting. I'll retry things tomorrow on different platforms to see if it a Mac oddity.

Comment on attachment 335576 [details] [diff] [review] additional test patch rev2 (checked in) This version works much better.

Comment on attachment 335422 [details] [diff] [review] patch for 1.9.0.x Drivers, I'd like to get this for 1.9.0.3. It is well tested and safe.

Comment on attachment 335576 [details] [diff] [review] additional test patch rev2 (checked in) Checked in the additional tests http://hg.mozilla.org/mozilla-central/index.cgi/rev/c4fa1f905ea7e79ba7c7d89a844ae67cefc8b338

As described in bug 440750, this bug changes an error in checkCert from one wrong error message to another wrong error message. I would request that this is not landed on 1.9.0.x as is, as it would make documenting a frequent error more complicated. Possible solutions could be fixing bug 440750, fixing bug 401292 or special-casing checkCert errors to the 404 message.

Jesper, I'll remove the request for now and make this bug depend on bug 401292 for tracking purposes. If bug 401292 isn't fixed I might make this fallback to the 404 for 1.9.0.x since bug 401292 will most likely require string changes. btw: bug 401292 is simple to fix and I'll most likely have a patch this coming week.