The default bug view has changed. See this FAQ.

waitForDownload() has to return early if download failure occurs

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ashughes, Assigned: whimboo)

Tracking

unspecified

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(4 attachments, 3 obsolete attachments)

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.
Blocks: 768576
(Assignee)

Comment 1

5 years ago
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.
Component: Mozmill Tests → Mozmill Automation
QA Contact: mozmill-tests → mozmill-automation
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
Status: NEW → ASSIGNED
Component: Mozmill Automation → Mozmill Tests
QA Contact: mozmill-automation → mozmill-tests
(Assignee)

Updated

5 years ago
Assignee: nobody → hskupin
Summary: All 12->13.0.1 releasetest updates fail | Timeout: Update has been finished downloading → If a downloaded patch can't be applied (error page shown) don't show: "Update has been finished downloading"
(Assignee)

Comment 4

5 years ago
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.
Attachment #637385 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
No longer blocks: 768576
(Assignee)

Comment 5

5 years ago
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.
Summary: If a downloaded patch can't be applied (error page shown) don't show: "Update has been finished downloading" → waitForDownload() has to return early if download failure occurs
(Assignee)

Comment 6

5 years ago
I have filed bug 769156 for the underlying platform issue.
(Assignee)

Comment 7

5 years ago
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.
Attachment #637425 - Flags: review?(dave.hunt)
(Assignee)

Updated

5 years ago
Attachment #637425 - Attachment description: Patch v1 → Patch v1 (default, aurora)
(Assignee)

Comment 8

5 years ago
Created attachment 637438 [details] [diff] [review]
Patch v1.1 (default, aurora)

Missed a qrefresh in the last patch.
Attachment #637425 - Attachment is obsolete: true
Attachment #637425 - Flags: review?(dave.hunt)
Attachment #637438 - Flags: review?(dave.hunt)

Updated

5 years ago
Attachment #637438 - Flags: review?(dave.hunt) → review+
(Assignee)

Comment 9

5 years ago
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/933c436abb48
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 10

5 years ago
Created attachment 637517 [details] [diff] [review]
Patch v1.1 (beta, release, esr10)

Backported patch for older branches.
Attachment #637517 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #637517 - Attachment is patch: true
Attachment #637517 - Flags: review? → review?(dave.hunt)
(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.
(Assignee)

Comment 12

5 years ago
Anthony, please read through my follow-up comments. I have stated that it was a wrong assumption.
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?
Attachment #637517 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 14

5 years ago
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. :)
Attachment #637517 - Flags: review- → review?(dave.hunt)
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);
(Assignee)

Comment 16

5 years ago
(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.
Status: RESOLVED → REOPENED
status-firefox16: fixed → affected
Resolution: FIXED → ---
(Assignee)

Comment 17

5 years ago
Created attachment 637588 [details] [diff] [review]
Follow-up (default, aurora)
Attachment #637588 - Flags: review?(anthony.s.hughes)
Comment on attachment 637588 [details] [diff] [review]
Follow-up (default, aurora)

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

Looks good to me, please land.
Attachment #637588 - Flags: review?(anthony.s.hughes) → review+
(Assignee)

Comment 19

5 years ago
Follow-up landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/e4077f088500
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox16: affected → fixed
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Attachment #637517 - Flags: review?(dave.hunt) → review?(anthony.s.hughes)
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.
Attachment #637517 - Flags: review?(anthony.s.hughes) → review+
(Assignee)

Comment 21

5 years ago
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
status-firefox15: affected → fixed
(Assignee)

Comment 22

5 years ago
Created attachment 637888 [details] [diff] [review]
Patch v1.2 (beta, release, esr10)

Minor fix for message wording which was missing a qrefresh locally.
Attachment #637517 - Attachment is obsolete: true
Attachment #637888 - Flags: review+
(Assignee)

Comment 23

5 years ago
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)
status-firefox-esr10: affected → fixed
status-firefox13: affected → fixed
status-firefox14: affected → fixed
You need to log in before you can comment on or make changes to this bug.