Add a Install/Cancel/Download test to the dom:app test suite

RESOLVED FIXED in mozilla34

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: amac, Assigned: amac)

Tracking

unspecified
mozilla34
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Bug 1036143 detected a hole on the test suite for DOM:Apps. Currently we don't have tests for cancelling/retrying the download. This bug is just to add that test.
Initial version. The test is completed, not asking for review because I want to add a downloadapplied test also.
Attachment #8453643 - Flags: feedback?(fabrice)
Depends on: 903291
Try run (including the patch from bug 903291, and the patch I proposed to that bug also!) on  https://tbpl.mozilla.org/?tree=Try&rev=5c29dc8da9d6
Posted patch V2. With an apply test (obsolete) — Splinter Review
Attachment #8453643 - Attachment is obsolete: true
Attachment #8453643 - Flags: feedback?(fabrice)
Attachment #8453749 - Flags: review?(fabrice)
Sorry for the spam. It's failing on TBPL and I think it might be because of the timer. I've removed the timer on the test_packaged*html, but can't remove it on the .sjs (and in any case it doesn't really matter if it's slower). I have one problem, though, and is that if I set a timer higher than 3 seconds it never fires.
Attachment #8453749 - Attachment is obsolete: true
Attachment #8453749 - Flags: review?(fabrice)
Attachment #8453776 - Flags: review?(fabrice)
Comment on attachment 8453776 [details] [diff] [review]
V3. Without settimeout on the "client" part

And again... /facepalm. I don't know how I did it but I lost the changes of a file on the patch. Removing the review request till this *$·"! thing passes consistently on the TBPL.
Attachment #8453776 - Flags: review?(fabrice)
Comment on attachment 8453880 [details] [diff] [review]
V4. Use a trick so the 'server' timeout doesn't matter as much

Try run is green finally, requesting review.
Attachment #8453880 - Flags: review?(fabrice)
Comment on attachment 8453880 [details] [diff] [review]
V4. Use a trick so the 'server' timeout doesn't matter as much

Review of attachment 8453880 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, but explain me your choice for alreadyDeferred.

::: dom/apps/tests/file_packaged_app.sjs
@@ +26,5 @@
>    var devUrl = ("devUrl" in query) ? query.devUrl : gDevUrl;
> +  // allowCancel just means deliver the file slowly so we have time to cancel it
> +  var allowCancel = "allowCancel" in query;
> +  var getPackage = "getPackage" in query;
> +  var alreadyDeferred = +getState("alreadyDeferred");

why not use a boolean for alreadyDeferred?

@@ +32,5 @@
> +  if (allowCancel && getPackage && !alreadyDeferred) {
> +    // Only do this for the actual package delivery.
> +    response.processAsync();
> +    // And to avoid timer problems, only do this once.
> +    setState("alreadyDeferred", "1");

s/"1"/true ?
Attachment #8453880 - Flags: review?(fabrice) → review+
r=fabrice
Attachment #8453880 - Attachment is obsolete: true
Attachment #8454702 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/28664fb86b3c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Backed out for frequent test_packaged_app_install.html timeouts after this landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/66651fb7bca3

https://tbpl.mozilla.org/php/getParsedLog.php?id=43727836&tree=Mozilla-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla33 → ---
This is basically the same patch that was r+ed before. I don't know why it was failing randomly on try, and I don't really think it's a problem of the test. In any case, I've modified a little the delayed download, to send some data before waiting (in case there's something closing the connection if no data has been seen) and reduced the delay.

The tries I've send are all green... but they were before also :)
Attachment #8454702 - Attachment is obsolete: true
Attachment #8456851 - Flags: review?(fabrice)
(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #14)
> Created attachment 8456851 [details] [diff] [review]
> Refactored the delay and wrote some data before waiting
> 
> This is basically the same patch that was r+ed before. I don't know why it
> was failing randomly on try, and I don't really think it's a problem of the
> test. In any case, I've modified a little the delayed download, to send some
> data before waiting (in case there's something closing the connection if no
> data has been seen) and reduced the delay.
> 
> The tries I've send are all green... but they were before also :)

Please post the try links, and retrigger the test_packaged_app_install.html test 20 times to make sure it's solid.
Attachment #8456851 - Flags: review?(fabrice)
Relaunched the tests a bunch of times on several platforms:

https://tbpl.mozilla.org/?tree=Try&rev=ef444b87bdba
Attachment #8456851 - Attachment is obsolete: true
Attachment #8460273 - Flags: review?(fabrice)
Attachment #8460273 - Attachment is patch: true
Attachment #8460273 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/358263de7795
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.