769090 - waitForDownload() has to return early if download failure occurs

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

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][⌚️UTC+1]]

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][⌚️UTC+1]]

Attachment: 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][⌚️UTC+1]]

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][⌚️UTC+1]]

Attachment: 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][⌚️UTC+1]]

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][⌚️UTC+1]]

I have filed bug 769156 for the underlying platform issue.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: 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][⌚️UTC+1]]

Attachment: Patch v1.1 (default, aurora)

Missed a qrefresh in the last patch.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/933c436abb48

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1.1 (beta, release, esr10)

Backported patch for older branches.

Comment 11

[u279076]

(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][⌚️UTC+1]]

Anthony, please read through my follow-up comments. I have stated that it was a wrong assumption.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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][⌚️UTC+1]]

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] [he/him] ⌚BST]

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][⌚️UTC+1]]

(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][⌚️UTC+1]]

Attachment: Follow-up (default, aurora)

Comment 18

[u279076]

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][⌚️UTC+1]]

Follow-up landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/e4077f088500

Comment 20

[u279076]

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][⌚️UTC+1]]

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][⌚️UTC+1]]

Attachment: Patch v1.2 (beta, release, esr10)

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

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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)