Last Comment Bug 769090 - waitForDownload() has to return early if download failure occurs
: waitForDownload() has to return early if download failure occurs
Status: RESOLVED FIXED
[mozmill-test-failure]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Henrik Skupin (:whimboo)
:
:
Mentors:
http://mozmill-ondemand.blargon7.com/...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-27 16:29 PDT by Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Modified: 2012-06-29 06:37 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
screenshot (78.94 KB, image/jpeg)
2012-06-27 23:35 PDT, Henrik Skupin (:whimboo)
no flags Details
screenshot (en-US) (42.24 KB, image/jpeg)
2012-06-27 23:45 PDT, Henrik Skupin (:whimboo)
no flags Details
Patch v1 (default, aurora) (1.14 KB, patch)
2012-06-28 02:50 PDT, Henrik Skupin (:whimboo)
no flags Details | Diff | Splinter Review
Patch v1.1 (default, aurora) (1.26 KB, patch)
2012-06-28 03:13 PDT, Henrik Skupin (:whimboo)
dave.hunt: review+
Details | Diff | Splinter Review
Patch v1.1 (beta, release, esr10) (1.26 KB, patch)
2012-06-28 08:08 PDT, Henrik Skupin (:whimboo)
anthony.s.hughes: review+
Details | Diff | Splinter Review
Follow-up (default, aurora) (1.22 KB, patch)
2012-06-28 10:47 PDT, Henrik Skupin (:whimboo)
anthony.s.hughes: review+
Details | Diff | Splinter Review
Patch v1.2 (beta, release, esr10) (1.25 KB, patch)
2012-06-29 06:35 PDT, Henrik Skupin (:whimboo)
hskupin: review+
Details | Diff | Splinter Review

Description Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-27 16:29:57 PDT
I'm trying to test 12->13.0.1 updates on releasetest and they all appear to be failing.

Partial:
test3.js::testDirectUpdate_AppliedAndNoUpdatesFound
Update has been finished downloading. 

Partial+Fallback:
test4.js::testFallbackUpdate_AppliedAndNoUpdatesFound 	
Update has been finished downloading. 

The AUS URLs appear to be okay, the downloads seem to complete fine, so I'm not sure what could be going on here. The downloads are speedy and apply cleanly when testing manually.

Also, there were no automation issues when testing on betatest.
Comment 1 Henrik Skupin (:whimboo) 2012-06-27 23:14:43 PDT
Well, there is nothing we can really do against slow mirrors or connections to those mirrors. While everything can be ok from your home place the situation somewhere else can be even worse. Something we really need is bug 769151 fixed.

I'm not inclined to raise the timeout even more. We currently are already at 5 minutes for a download. If a mirror connection can't handle that we should in fact time out and proceed the next test. Otherwise all other tests in our cluster will be deferred.

So lets see that we can get bug 769151 fixed asap.
Comment 2 Henrik Skupin (:whimboo) 2012-06-27 23:35:40 PDT
Created attachment 637385 [details]
screenshot

So actually this is not a problem with the download itself. I run an example update tests on Windows XP and get an error *after* downloading the update. See the attached screenshot. So the patch is broken and can't be applied to Firefox. Sorry I can't read Italian so I will try to do the same with an en-US build soon.

But in any case this should be reported against RelEng so the updates or whatever else goes wrong here can be fixed.
Comment 3 Henrik Skupin (:whimboo) 2012-06-27 23:36:56 PDT
In case of our update tests I will see if I can find an easy way to check for this failure, so we have a better failure message.
Comment 4 Henrik Skupin (:whimboo) 2012-06-27 23:45:49 PDT
Created attachment 637388 [details]
screenshot (en-US)

So this is really a problem with updates and not with downloading. We fail with an unknown reason to apply the update.
Comment 5 Henrik Skupin (:whimboo) 2012-06-28 00:26:09 PDT
Actually what we want here is that the waitForDownload() method returns if another page as the downloading one is shown.

With the patch applied locally we get:
http://mozmill-crowd.blargon7.com/#/update/report/23e194f1171aa4baf774928b2604cdae

New wizard page has been selected - got errors, expected finished 

That one should be pretty clear that something went wrong with the update.
Comment 6 Henrik Skupin (:whimboo) 2012-06-28 00:59:14 PDT
I have filed bug 769156 for the underlying platform issue.
Comment 7 Henrik Skupin (:whimboo) 2012-06-28 02:50:56 PDT
Created attachment 637425 [details] [diff] [review]
Patch v1 (default, aurora)

With this patch we bail out early from the waitFor call and check that the error page has not been selected.
Comment 8 Henrik Skupin (:whimboo) 2012-06-28 03:13:23 PDT
Created attachment 637438 [details] [diff] [review]
Patch v1.1 (default, aurora)

Missed a qrefresh in the last patch.
Comment 9 Henrik Skupin (:whimboo) 2012-06-28 08:07:22 PDT
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/933c436abb48
Comment 10 Henrik Skupin (:whimboo) 2012-06-28 08:08:24 PDT
Created attachment 637517 [details] [diff] [review]
Patch v1.1 (beta, release, esr10)

Backported patch for older branches.
Comment 11 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-28 10:06:32 PDT
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Well, there is nothing we can really do against slow mirrors or connections
> to those mirrors. 

Sorry for commenting on a resolved bug, but I just wanted to comment to this point. I'm not convinced we are hitting a timeout. The timeout is 360000 (I assume 5 minutes) and the partial results which are failing are reporting the download is completing in far less then that.
Comment 12 Henrik Skupin (:whimboo) 2012-06-28 10:08:13 PDT
Anthony, please read through my follow-up comments. I have stated that it was a wrong assumption.
Comment 13 Dave Hunt (:davehunt) 2012-06-28 10:14:09 PDT
Comment on attachment 637517 [details] [diff] [review]
Patch v1.1 (beta, release, esr10)

+    }, "Update has been finished downloading.", timeout, undefined, this);

The undefined, this looks odd to me. What is this doing?
Comment 14 Henrik Skupin (:whimboo) 2012-06-28 10:17:56 PDT
Comment on attachment 637517 [details] [diff] [review]
Patch v1.1 (beta, release, esr10)

Not sure what you mean. That's the delay parameter for waitFor which defaults to 100 in case of 'undefined'. That's how waitFor works since it has been introduced. :)
Comment 15 Dave Hunt (:davehunt) 2012-06-28 10:25:26 PDT
Comment on attachment 637517 [details] [diff] [review]
Patch v1.1 (beta, release, esr10)

So why isn't it specified on the default and aurora branches?

    this._controller.waitFor(function () {
      return this.currentPage !== WIZARD_PAGES.downloading ||
             progress.getNode().value === '100';
    }, "Update has been finished downloading.", timeout);
Comment 16 Henrik Skupin (:whimboo) 2012-06-28 10:44:50 PDT
(In reply to Dave Hunt (:davehunt) [Away until July 3rd] from comment #15)
> So why isn't it specified on the default and aurora branches?
> 
>     this._controller.waitFor(function () {
>       return this.currentPage !== WIZARD_PAGES.downloading ||
>              progress.getNode().value === '100';
>     }, "Update has been finished downloading.", timeout);

Good call. It has to because we make use of this inside of the callback. :/ Have to follow-up with another patch on default and aurora.
Comment 17 Henrik Skupin (:whimboo) 2012-06-28 10:47:10 PDT
Created attachment 637588 [details] [diff] [review]
Follow-up (default, aurora)
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-28 11:24:07 PDT
Comment on attachment 637588 [details] [diff] [review]
Follow-up (default, aurora)

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

Looks good to me, please land.
Comment 19 Henrik Skupin (:whimboo) 2012-06-28 11:34:52 PDT
Follow-up landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/e4077f088500
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-28 11:52:05 PDT
Comment on attachment 637517 [details] [diff] [review]
Patch v1.1 (beta, release, esr10)

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

Looks good to me. Please land.
Comment 21 Henrik Skupin (:whimboo) 2012-06-29 06:33:39 PDT
Landed patch and follow-up on aurora:
http://hg.mozilla.org/qa/mozmill-tests/rev/05e6129cc057
http://hg.mozilla.org/qa/mozmill-tests/rev/d8be1734f0fc
Comment 22 Henrik Skupin (:whimboo) 2012-06-29 06:35:19 PDT
Created attachment 637888 [details] [diff] [review]
Patch v1.2 (beta, release, esr10)

Minor fix for message wording which was missing a qrefresh locally.

Note You need to log in before you can comment on or make changes to this bug.