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) [away 02/18 - 02/27]
:
:
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) [away 02/18 - 02/27]
no flags Details
screenshot (en-US) (42.24 KB, image/jpeg)
2012-06-27 23:45 PDT, Henrik Skupin (:whimboo) [away 02/18 - 02/27]
no flags Details
Patch v1 (default, aurora) (1.14 KB, patch)
2012-06-28 02:50 PDT, Henrik Skupin (:whimboo) [away 02/18 - 02/27]
no flags Details | Diff | Splinter Review
Patch v1.1 (default, aurora) (1.26 KB, patch)
2012-06-28 03:13 PDT, Henrik Skupin (:whimboo) [away 02/18 - 02/27]
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) [away 02/18 - 02/27]
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) [away 02/18 - 02/27]
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) [away 02/18 - 02/27]
hskupin: review+
Details | Diff | Splinter Review

Description User image 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2012-06-28 00:59:14 PDT
I have filed bug 769156 for the underlying platform issue.
Comment 7 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2012-06-28 08:07:22 PDT
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/933c436abb48
Comment 10 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2012-06-28 10:47:10 PDT
Created attachment 637588 [details] [diff] [review]
Follow-up (default, aurora)
Comment 18 User image 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2012-06-28 11:34:52 PDT
Follow-up landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/e4077f088500
Comment 20 User image 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 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.
Comment 23 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2012-06-29 06:37:45 PDT
Landed on remaining branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/da74e7fe7469 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/e27318e18b1e (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/4120cb149410 (esr10)

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