567258 - Update the SoftwareUpdateAPI for Firefox 4.0

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

The ui changes of the software update dialog causes our Mozmill tests to fail for Minefield builds. The SoftwareUpdateAPI needs an update to support the update process for Firefox 4.0.

I will work on this tomorrow so we can make sure to have recent builds for our daily test-run.

Comment 1

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

Robert, I'm working on an updated version of our shared API now to make it possible to run update tests on trunk. Sadly I have a problem with the activeUpdate which was normally already set after the check had been finished. Now it looks like that the update manager only returns the update after you have clicked on "Next" to download the update. Is there a way to check which type of update we have before I click on next? I need that information to make sure that the correct page of the wizard has been selected.

Comment 2

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

Also updateCount is 0 so I can't even check if there is any update in the queue.

Comment 3

[Robert Strong (they/them - no direct email)]

(In reply to comment #1)
> Robert, I'm working on an updated version of our shared API now to make it
> possible to run update tests on trunk. Sadly I have a problem with the
> activeUpdate which was normally already set after the check had been finished.
> Now it looks like that the update manager only returns the update after you
> have clicked on "Next" to download the update.
Are you sure that was the case before the trunk rewrite? I could have sworn that activeUpdate never got set until after the user clicked next and that we had this same conversation in another bug.

> Is there a way to check which
> type of update we have before I click on next? I need that information to make
> sure that the correct page of the wizard has been selected.
That has changed since we no longer require the billboard page for major updates.

btw: there are already a ton of mochitest chrome tests that test whether the page displayed is correct (all pages except for plugin updates found and though the manual update page is tested it isn't tested for the case where the user doesn't have permissions), sequence of pages going forward and backward, button states, and billboard / license states. The one thing it is unable to test is the real update xml for updates and the real update mar (iirc they are tested by releng and there are xpcshell tests for applying partial / complete mars).

We should probably figure out what actually needs to be tested instead of working on / maintaining redundant tests.

Comment 4

[Robert Strong (they/them - no direct email)]

(In reply to comment #3)
> (In reply to comment #1)
> > Robert, I'm working on an updated version of our shared API now to make it
> > possible to run update tests on trunk. Sadly I have a problem with the
> > activeUpdate which was normally already set after the check had been finished.
> > Now it looks like that the update manager only returns the update after you
> > have clicked on "Next" to download the update.
> Are you sure that was the case before the trunk rewrite? I could have sworn
> that activeUpdate never got set until after the user clicked next and that we
> had this same conversation in another bug.
I verified that activeUpdate is null on 3.6 after the check for updates and is only set after next has been clicked on the update found page as I thoought was the case.

On trunk you can verify that the update found page was the correct one seen by getting the pageid for the update found page, clicking next which will set activeUpdate, and checking the billboardURL attribute of activeUpdate. If billboardURL is present then the update found pageid should be updatesfoundbillboard and if it isn't it should be updatesfoundbasic. The pageid can change though this shouldn't happen very often.

Comment 5

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

Attachment: Patch v1 [checked-in]

As talked with Rob, we will go the easiest way for now. We have addon compatibility checks disabled and the only add-ons which are installed are Mozmill and Jsbridge itself. So we do not have to care about those wizard pages. That means we do:

checking -> updatesfoundbasic/updatesfoundbillboard -> downloading -> finish
errorpatching -> downloading -> finish

Tested on all platforms with updates which do not require a billboard.

Comment 6

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

Landed on default branch as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1870feaac4c7

I will leave it open because I wanna backport some of the changes to older branches.

Anthony please test all update paths on qa-set. The test-run script will use the updated API now. Thanks.

Comment 7

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

(In reply to comment #3)
> > Now it looks like that the update manager only returns the update after you
> > have clicked on "Next" to download the update.
> >
> Are you sure that was the case before the trunk rewrite? I could have sworn
> that activeUpdate never got set until after the user clicked next and that we
> had this same conversation in another bug.

You are right. We used a simple trick to get the update type before we clicked on the update button. There was a description element under the updatesfound page of the wizard. With the new version we have a similar element for the updatesfoundbasic page but not for the updatesfoundbillboard page.

In our Mozmill tests we also check after an update has been applied that no more updates get offered. But if that's the case, i.e. we have a minor update followed by a major update, that little trick wouldn't work anymore. Means the newly offered update would have to be started to download. Only then we can check if we really have a major update and not another minor update in the queue.

So if the billboard update page doesn't have such a label (updatesFountInto) I will have to start the download and cancel it right after the version check.

Rob, would that be the only we for us or are there other options?

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to comment #7)
> (In reply to comment #3)
> > > Now it looks like that the update manager only returns the update after you
> > > have clicked on "Next" to download the update.
> > >
> > Are you sure that was the case before the trunk rewrite? I could have sworn
> > that activeUpdate never got set until after the user clicked next and that we
> > had this same conversation in another bug.
> 
> You are right. We used a simple trick to get the update type before we clicked
> on the update button. There was a description element under the updatesfound
> page of the wizard. With the new version we have a similar element for the
> updatesfoundbasic page but not for the updatesfoundbillboard page.
> 
> In our Mozmill tests we also check after an update has been applied that no
> more updates get offered. But if that's the case, i.e. we have a minor update
> followed by a major update, that little trick wouldn't work anymore. Means the
> newly offered update would have to be started to download. Only then we can
> check if we really have a major update and not another minor update in the
> queue.
I'm having difficulty understanding exactly what you are testing for in the above statement. For example,
"In our Mozmill tests we also check after an update has been applied that no more updates get offered."
so, you are checking there are no more updates available.

"But if that's the case, i.e. we have a minor update followed by a major update..."
when there can be an update available?

What exactly are you checking / testing for? Is it going from minor to the latest minor when there is a major available from the latest minor?

It seems like you should be testing that you are on the expected version instead. Also, keep in mind that major is just an attribute in the update snippet and it can be set to major even for a minor version bump and that it doesn't have to correspond to the major version number change.

Comment 9

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

(In reply to comment #8)
> > newly offered update would have to be started to download. Only then we can
> > check if we really have a major update and not another minor update in the
> > queue.
> I'm having difficulty understanding exactly what you are testing for in the
> above statement. For example,
> "In our Mozmill tests we also check after an update has been applied that no
> more updates get offered."
> so, you are checking there are no more updates available.

Lets use the direct update as example:

1. Check that the user has permissions to update
2. Check that updates are available and detect the type
3. Download the update
4. Restart Firefox
5. Check that no further updates are offered
5.1. If that fails we check if the new update type is not the same as the one from step 2.
 
With step 5 we run into trouble as what we have seen yesterday. There were the following updates on the releasetest channel:

* Firefox 3.6.4 buildX -> Firefox 3.6.4 build7
* Firefox 3.6.4 build7 (releasetest) -> Firefox 3.6.4 build7 (betatest)

The second update is unpredictable for us. In the current state we can't run two or more updates in a row. With the above steps our final check fails.

The question is if we should test for it or not. To run all the updates we would have to enhance Mozmill, so we would be able to run the same test multiple times.

> What exactly are you checking / testing for? Is it going from minor to the
> latest minor when there is a major available from the latest minor?
 
That's correct when we are talking about minor and major updates only. The major one will not be executed. But we run into a failure if another minor is offered afterward, which happens in the above example.

> It seems like you should be testing that you are on the expected version
> instead. Also, keep in mind that major is just an attribute in the update
> snippet and it can be set to major even for a minor version bump and that it
> doesn't have to correspond to the major version number change.

Results we get look like that (with an human check):
https://wiki.mozilla.org/Releases/Firefox_3.6.4/Test_Plan:Software_Update#Windows_7_.28hskupin.29

Those tests have to work with releases and nightly builds, which makes it hard to specify which version you expect to have finally. I don't see an easy way to get the build id for release builds, which would make it much harder for the people to run those tests.

I'm already updating the tests now to make sure we always start the download, if one is available, to get the new update type.

Comment 10

[Robert Strong (they/them - no direct email)]

What is being offered on a channel / app version can't be determined before hand as you found in the releasetest channel and there really isn't anything the client can do about that. An attribute could be set in the UI that denotes the update type but I'd prefer not changing UI (though it would be hidden) for this and it wouldn't help the releasetest case you outlined.

It isn't clear if it would help but buildID is available for the app and the update contains the buildID in the update snippet which should be correct.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1648

Comment 11

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

(In reply to comment #10)
> What is being offered on a channel / app version can't be determined before
> hand as you found in the releasetest channel and there really isn't anything
> the client can do about that. An attribute could be set in the UI that denotes
> the update type but I'd prefer not changing UI (though it would be hidden) for
> this and it wouldn't help the releasetest case you outlined.

Sounds fair enough. I think having an human check the results is the best way we can do right now. That's what we did the last half year and it worked fine.

> It isn't clear if it would help but buildID is available for the app and the
> update contains the buildID in the update snippet which should be correct.
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1648

Ah well, that's probably the right way to check if the update has been applied correctly, instead of checking the version number and build id if they have been increased. Thanks for pointing that out.

Comment 12

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

Attachment: Follow-up (default) [checked-in]

This follow-up fixes the following items:

* Don't use the description on the UI to get the update type but start the download and check activeUpdate.type

* Use the build id offered by the update instead of the pre build id for a comparison with the post build id. That will limit the failure when the build id doesn't change, i.e. we only flip a preference with an update

* Some more refactoring to make it easier to maintain.

Comment 13

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

Landed follow-up on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/f56453d911e0

I will combine all the changes for 1.9.2/1.9.1 later this week.

Comment 14

[u279076]

(In reply to comment #13)
> Landed follow-up on default:
> http://hg.mozilla.org/qa/mozmill-tests/rev/f56453d911e0
> 
> I will combine all the changes for 1.9.2/1.9.1 later this week.

This is definitely needed on 1.9.1/1.9.2...the tests fail for 3.6 updates:
Controller.assertJS("subject.newUpdateType != subject.lastUpdateType") 

I'll prioritize testing of this patch on 3.7 once the current release cycle has completed.  I'll post my findings here.

Comment 15

[u279076]

I've run a sample across all platforms:
1. 3.7a5pre 20100601 complete
2. 3.7a6pre 20100621 partial

The results are in brasstacks, but here is a sample:

* 3.7a5pre => 3.7a6pre, minor, en-US, complete, nightly, 2010-06-22, '''PASS'''
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100601 Minefield/3.7a5pre ID:20100601030700
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100622 Minefield/3.7a6pre ID:20100622030847
** Passed 10 :: Failed 0 :: Skipped 0

* 3.7a5pre => 3.7a5pre, minor, en-US, complete, nightly, 2010-06-22, '''FAIL'''
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100601 Minefield/3.7a5pre ID:20100601030700
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100601 Minefield/3.7a5pre ID:20100601030700
** Passed 10 :: Failed 4 :: Skipped 0

* 3.7a6pre => 3.7a6pre, minor, en-US, partial, nightly, 2010-06-22, '''PASS'''
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100621 Minefield/3.7a6pre ID:20100621103834
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100622 Minefield/3.7a6pre ID:20100622030847
** Passed 14 :: Failed 0 :: Skipped 0

These results are consistent across all platforms.

Comment 16

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

Interesting that I haven't seen it before. The problem here is:

timeout exceeded for waitForEval('subject.currentPage == 'errorpatching'')

Rob, in trunk builds we are showing up the 'errors' wizard page now. Formerly we have used 'errorpatching'. Is that a permanent change on trunk? We missed to talk about this part during our call. If it's the case I will update our tests so they are using 'errors' from now on.

Comment 17

[Robert Strong (they/them - no direct email)]

Without knowing all of the details it is difficult to know what is going on but it might be the fix on trunk if there is only one patch or the patch it was trying to apply was a complete and it fails to apply we go to the errors page and not the errorpattching page which is the correct behavior and a bug on 1.9.2. There are also mochitests for the different scenarios

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/test_0081_error_patchApplyFailure_partial_only.xul
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/test_0082_error_patchApplyFailure_complete_only.xul
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/test_0083_error_patchApplyFailure_partial_complete.xul
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/chrome/test_0084_error_patchApplyFailure_verify_failed.xul

Comment 18

[Robert Strong (they/them - no direct email)]

In case it isn't obvious, if there is only one patch or the patch that failed is a complete the ui will go to the errors page. If the patch that failed was a partial and there is a complete patch then it will go to the errorpatching page.

Comment 19

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

(In reply to comment #18)
> In case it isn't obvious, if there is only one patch or the patch that failed
> is a complete the ui will go to the errors page. If the patch that failed was a
> partial and there is a complete patch then it will go to the errorpatching
> page.

Thanks for that information. It make sense. I will implement it in that way for our software update tests. I assume there will be no backport for 3.6 and 3.5. So for Mozmill it should only be a part for Firefox 4.0.

Comment 20

[Robert Strong (they/them - no direct email)]

It might very well get backported... at first it was going to be, then it got pushed back due to OOPP, and now 3.6.6 is not letting all that much land so it isn't possible to tell one way or the other

Comment 21

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

(In reply to comment #20)
> It might very well get backported... at first it was going to be, then it got
> pushed back due to OOPP, and now 3.6.6 is not letting all that much land so it
> isn't possible to tell one way or the other

Can you please add this bug to the dependency list so we are aware when it gets checked onto branches?

Another change I have discovered for complete updates is that in a fallback case no other complete update is automatically started. The user has to close the software update dialog and check again for updates. What does it mean for our release testing work? Do we still have to check the fallback case for complete updates or can we simply ignore it and only run fallback checks for partial updates? Given all the automated tests (Robert already pointed out) I think we can save time and don't run those paths.

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to comment #21)
> (In reply to comment #20)
> > It might very well get backported... at first it was going to be, then it got
> > pushed back due to OOPP, and now 3.6.6 is not letting all that much land so it
> > isn't possible to tell one way or the other
> 
> Can you please add this bug to the dependency list so we are aware when it gets
> checked onto branches?
> 
> Another change I have discovered for complete updates is that in a fallback
> case no other complete update is automatically started.
Correct. If the complete fails there is no fallback besides downloading the installer / dmg / etc. and there is a link to do so on the page.

> The user has to close
> the software update dialog and check again for updates. What does it mean for
> our release testing work? Do we still have to check the fallback case for
> complete updates or can we simply ignore it and only run fallback checks for
> partial updates? Given all the automated tests (Robert already pointed out) I
> think we can save time and don't run those paths.
The fallback for a complete failure is to manually download.

Comment 23

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

(In reply to comment #22)
> > Can you please add this bug to the dependency list so we are aware when it gets
> > checked onto branches?

Would still be interested in that bug. Whenever you have time please add it.

> > 
> > Another change I have discovered for complete updates is that in a fallback
> > case no other complete update is automatically started.
> Correct. If the complete fails there is no fallback besides downloading the
> installer / dmg / etc. and there is a link to do so on the page.

Alright. I will update the test accordingly and we will not check for a fallback anymore for complete updates.

Comment 24

[Robert Strong (they/them - no direct email)]

Since you pinged me 3 times for this today and as usual I replied after finding the info perhaps I can get you to find the info and reply to bug 538372? ;)

It was fixed in Bug 538533 and additional fixes are in Bug 530872 as part of the rewrite. It was not split out of Bug 530872 since that bug originally was suppose to land for 3.6.4.

Comment 25

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

Attachment: Handle fallback updates for complete patches differently

Ok, I have to drop my last idea. In the case of a complete fallback update we absolutely have to download the update again. Otherwise our overall update results will be wrong. So in such a case we check for the errors page, close the update dialog, and directly start another update. Tested all possible paths with Minefield nightly builds.

Comment 26

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

(In reply to comment #24)
> Since you pinged me 3 times for this today and as usual I replied after finding
> the info perhaps I can get you to find the info and reply to bug 538372? ;)

Ouch, now you got me! This one I lost in my bug mail queue. :( I will clear it beginning of next month.

> It was fixed in Bug 538533 and additional fixes are in Bug 530872 as part of
> the rewrite. It was not split out of Bug 530872 since that bug originally was
> suppose to land for 3.6.4.

Thanks.

Comment 27

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

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/20c4b80dd6aa

Anthony, please test if all types of updates are working now.

Comment 28

[u279076]

Attachment: Results v1

Here are the results of Minefield (20100601 and 20100629) across all platforms.  All tests PASS.

Comment 29

[u279076]

Attachment: Results (3.7aX->4.0b1)

I'm having an issue checking 3.7aX -> 4.0b1 updates using the scripts.  I'm not sure if this is related to the recent changes so I'm attaching the Linux log.  The results are identical across all platforms.

Having watched the updates, the failing ones appear to display the billboard offering 4.0b1 then Firefox closes down a few seconds after, without downloading the update.  I can install the update manually without any issues.

Henrik, please make this a priority as it blocks us from running update checks automatically.  If it can't be resolved, we will need to run these checks manually.

Comment 30

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

(In reply to comment #29)
When you check the results, you can see that we have different kinds of issues here. 

> Having watched the updates, the failing ones appear to display the billboard
> offering 4.0b1 then Firefox closes down a few seconds after, without
> downloading the update.  I can install the update manually without any issues.

You will not be able to run updates prior to the developer preview alpha 3 release. Formerly builds are still using the "updatesfound" wizard page which has been changed in the development cycle for alpha 3. Now we are using "updatesfoundbasic" and "updatesfoundbillboard". That has been changed with bug 480178. I don't intend to change that.

> ERROR - Test Failure: {u'exception': {u'message': u"timeout exceeded for waitForEval('subject.currentPage == 'errors'')"

That happens because we are using the partial update block to update from a4 to b1, which is also a complete update. It's already filed as bug 514040.

https://aus2.mozilla.org/update/3/Firefox/3.7a4/20100407105356/Darwin_Universal-gcc3/en-US/betatest/Darwin%2010.4.0/default/default/update.xml?force=1

Comment 31

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

Rob, could it be that the software update component itself is using the wrong patch for the update? Checking the logic how to identify which type of update we have, doesn't show a failure to me:

http://hg.mozilla.org/qa/mozmill-tests/file/f6b1814ebb7b/shared-modules/testSoftwareUpdateAPI.js#l126

We have two patches with the same url, which should be qualified as a complete update. But the updater is selects the partial update for the update. Could that be a regression?

Comment 32

[Robert Strong (they/them - no direct email)]

It is most likely due to the update snippet having both a partial and complete that are the same. Try creating a new update snippet from the existing one with only a complete update and use the app.update.url.override pref to point to it.

Comment 33

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

(In reply to comment #32)
> It is most likely due to the update snippet having both a partial and complete
> that are the same. Try creating a new update snippet from the existing one with
> only a complete update and use the app.update.url.override pref to point to it.

Right. That's why we have talked about last year and we have created the logic in the SoftwareUpdateAPI, I have pointed above. It works fine for Firefox 3.5 and 3.6 but is surprisingly broken on 4.0 now.

I don't want to modify too much regarding how or which update gets downloaded. It's not something the user can do, and that's what we are testing.

Regarding the above case (3.7a4 -> 4.0b1) we really should download the complete update and not the partial. So it looks like there is a regression in the updater?

Comment 34

[Robert Strong (they/them - no direct email)]

What most likely happens is the complete is already cached. On trunk, it is incorrect for the update to provide the same patch for a complete and a partial which is most likely the cause. We could protect for the case of an update snippet doing this in app update with additional code or the update snippet could only provide a complete instead of a partial and complete patch that are the same. The second approach seems more sound without adding complexity to the client to me.

Comment 35

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

With bug 576939 approved for 1.9.2 we will have to back-port our updates to the default branch version of the SoftwareUpdateAPI to the 1.9.2 branch. As discussed in the QA test automation meeting we will have to support two versions of the API. One which is used before those mentioned changed have been checked-in (<Firefox 3.6.9) and a mostly 1:1 copy from the default branch for recent versions (>=3.6.9).

Once bug 514040 has been fixed, I will make sure to test our version on the default branch. If it's working we can immediately use it on the branch.

Comment 36

[Robert Strong (they/them - no direct email)]

Attachment: patch

Using the trunk mozmill repo this patch makes all tests pass for both trunk and 1.9.2.

I've tested it with:
3.6.6 -> 3.6.8
3.6.7 -> 3.6.8
3.7a5 -> 4.0b2
4.0b1 -> 4.0b2

trunk nightly one day old -> current nightly
trunk nightly two days old -> current nightly (only one patch which is what the update snippets will have when there is only a complete after bug 514040 is fixed)

I haven't tested it with the patch from bug 576939 but it definitely should work the same.

Comment 37

[Robert Strong (they/them - no direct email)]

The patch also makes this bug not dependent on bug 514040 or bug 576939

Comment 38

[Robert Strong (they/them - no direct email)]

Comment on attachment 461144 [details] [diff] [review]
patch

>diff --git a/shared-modules/testSoftwareUpdateAPI.js b/shared-modules/testSoftwareUpdateAPI.js
>--- a/shared-modules/testSoftwareUpdateAPI.js
>+++ b/shared-modules/testSoftwareUpdateAPI.js
>...
>@@ -151,45 +157,58 @@ softwareUpdate.prototype = {
>   /**
>    * Returns the current step of the software update dialog wizard
>    */
>   get currentPage() {
>     return this._wizard.getNode().getAttribute('currentpageid');
>   },
> 
>   /**
>-   * Returns if the offered update is a complete update
>+   * Returns true if the offered update is a complete update
>    */
>   get isCompleteUpdate() {
>-    // XXX: Bug 514040: _ums.isCompleteUpdate doesn't work at the moment
>-    if (this.activeUpdate.patchCount > 1) {
>-      var patch1 = this.activeUpdate.getPatchAt(0);
>-      var patch2 = this.activeUpdate.getPatchAt(1);
>+    var patchCount = this.activeUpdate.patchCount;
>+    // Test that the update snippet created by releng has less than 3 patches
>+    controller.assertJS("subject.patchCount < 3",
>+                        {patchCount: patchCount < 3});
>+    // Test that the update snippet created by releng has more than 0 patches
>+    controller.assertJS("subject.patchCount > 0",
>+                        {patchCount: patchCount > 0});
> 
>-      return (patch1.URL == patch2.URL);
>-    } else {
>-      return (this.activeUpdate.getPatchAt(0).type == "complete");
>+// After bug 514040 is fixed remove this line and uncomment out the following
>+// code
>+//    if (this.activeUpdate.patchCount == 2) {
>+//      var patch0URL = this.activeUpdate.getPatchAt(0).URL;
>+//      var patch1URL = this.activeUpdate.getPatchAt(1).URL;
>+      // Test that the update snippet created by releng doesn't have the same
>+      // url for both patches (bug 514040).
>+//      controller.assertJS("subject.patch0URL != subject.patch1URL",
>+//                          {patch0URL: patch0URL, patch1URL: patch1URL});
>+//    }
>+
>+    if (this.activeUpdate.selectedPatch) {
>+      return (this.activeUpdate.selectedPatch.type  == "complete");
>     }
>+    return (this.activeUpdate.getPatchAt(0).type == "complete");
>   },
I suspect that these tests don't end up in a state where selectedPatch is null... this is "just in case".

Comment 39

[Robert Strong (they/them - no direct email)]

Attachment: patch - slightly cleaner

Removed the "just in case" in favor of throwing if isCompleteUpdate is called without an activeUpdate. All tests still pass

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: Better detection of partial/complete patches (default) [checked-in]

Sorry about that... forgot I added a queue

Comment 41

[Robert Strong (they/them - no direct email)]

Now that I have approval to land the patch in bug 576939 for Firefox 3.6.9 I've also verified that the mozmill tests pass with 3.6.9pre using the patch in this bug with a build with the patch in bug 576939.

Comment 42

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

Comment on attachment 461158 [details] [diff] [review]
Better detection of partial/complete patches (default) [checked-in]

>@@ -105,16 +105,22 @@ function softwareUpdate()
>   this._aus = Cc["@mozilla.org/updates/update-service;1"].
>               getService(Ci.nsIApplicationUpdateService);
>+  // nsIApplicationUpdateService2 is required for Firefox 3.6 but doesn't exist
>+  // in Firefox 4.0. This will QI to nsIApplicationUpdateService2 only when it
>+  // is available.
>+  if ("nsIApplicationUpdateService2" in Ci) {
>+    this._aus.QueryInterface(Ci.nsIApplicationUpdateService2);
>+  }

I'm not that familiar with additional interfaces. Does it mean that running a QueryInterface call against an aus instance will make all the members of both interfaces available?

>+    var patchCount = this.activeUpdate.patchCount;
>+    // Test that the update snippet created by releng has less than 3 patches
>+    controller.assertJS("subject.patchCount < 3",
>+                        {patchCount: patchCount < 3});
>+    // Test that the update snippet created by releng has more than 0 patches
>+    controller.assertJS("subject.patchCount > 0",
>+                        {patchCount: patchCount > 0});
>+
>+// After bug 514040 is fixed remove this line and uncomment out the following
>+// code
>+//    if (this.activeUpdate.patchCount == 2) {
>+//      var patch0URL = this.activeUpdate.getPatchAt(0).URL;
>+//      var patch1URL = this.activeUpdate.getPatchAt(1).URL;
>+      // Test that the update snippet created by releng doesn't have the same
>+      // url for both patches (bug 514040).
>+//      controller.assertJS("subject.patch0URL != subject.patch1URL",
>+//                          {patch0URL: patch0URL, patch1URL: patch1URL});
>+//    }
>+
>+    return (this.activeUpdate.selectedPatch.type  == "complete");

This looks pretty good. You mentioned that activeUpdate can't be null for well written tests. With this change in the SoftwareUpdateAPI we don't have to change any of our existing tests?

Comment 43

[Robert Strong (they/them - no direct email)]

(In reply to comment #42)
> Comment on attachment 461158 [details] [diff] [review]
> patch - slightly cleaner
> 
> >@@ -105,16 +105,22 @@ function softwareUpdate()
> >   this._aus = Cc["@mozilla.org/updates/update-service;1"].
> >               getService(Ci.nsIApplicationUpdateService);
> >+  // nsIApplicationUpdateService2 is required for Firefox 3.6 but doesn't exist
> >+  // in Firefox 4.0. This will QI to nsIApplicationUpdateService2 only when it
> >+  // is available.
> >+  if ("nsIApplicationUpdateService2" in Ci) {
> >+    this._aus.QueryInterface(Ci.nsIApplicationUpdateService2);
> >+  }
> 
> I'm not that familiar with additional interfaces. Does it mean that running a
> QueryInterface call against an aus instance will make all the members of both
> interfaces available?
Yes. This isn't needed for trunk but is needed for 3.6 and doesn't hurt trunk.

> This looks pretty good. You mentioned that activeUpdate can't be null for well
> written tests. With this change in the SoftwareUpdateAPI we don't have to
> change any of our existing tests?
No changes are needed as long as all the tests are located under softwareUpdate in the mozmill repo (I didn't see any others so if there are I haven't tested them). I've also verified the tests with this patch using multiple combinations of 3.6 and greated.

Comment 44

[Robert Strong (they/them - no direct email)]

(In reply to comment #43)
> of 3.6 and greated.
s/greated/greater/

Comment 45

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

(In reply to comment #43)
> > I'm not that familiar with additional interfaces. Does it mean that running a
> > QueryInterface call against an aus instance will make all the members of both
> > interfaces available?
> Yes. This isn't needed for trunk but is needed for 3.6 and doesn't hurt trunk.

Good to know that detail of implementation. I can take this into account even for other API's. Thanks!

> > This looks pretty good. You mentioned that activeUpdate can't be null for well
> > written tests. With this change in the SoftwareUpdateAPI we don't have to
> > change any of our existing tests?
> No changes are needed as long as all the tests are located under softwareUpdate
> in the mozmill repo (I didn't see any others so if there are I haven't tested
> them). I've also verified the tests with this patch using multiple combinations
> of 3.6 and greated.

Looks like my last refactoring fixed that initial problem then. Given your patch I assume you have only tested it with the default branch of the mozmill-tests repository or have you also tested on 1.9.2 by copying the necessary files from default to mozilla1.9.2? The reason is that I still have to backport all those changes, but knowing that overwriting the files in older branches also make it work, sounds fantastic.

Comment 46

[Robert Strong (they/them - no direct email)]

(In reply to comment #45)
>...
> Looks like my last refactoring fixed that initial problem then. Given your
> patch I assume you have only tested it with the default branch of the
> mozmill-tests repository or have you also tested on 1.9.2 by copying the
> necessary files from default to mozilla1.9.2? The reason is that I still have
> to backport all those changes, but knowing that overwriting the files in older
> branches also make it work, sounds fantastic.
I haven't tested with mozmill mozilla1.9.2 but I can if you would like me to.

Comment 47

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

Comment on attachment 461158 [details] [diff] [review]
Better detection of partial/complete patches (default) [checked-in]

Lets get it into default. Rob, if you would have time to test it on mozilla1.9.2 it would be great. We will have to replace/update the API and all the tests. The important bits are that it will also have to work with Firefox 3.6 - 3.6.8.

Shall I create a backport patch or do you wanna do it?

Comment 48

[Robert Strong (they/them - no direct email)]

I can test it in the next day or two and will create the backport then.

Do you want me to push it to default (do I have privs to do so?).

Comment 49

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

Comment on attachment 461158 [details] [diff] [review]
Better detection of partial/complete patches (default) [checked-in]

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/5eb639052a6d

Thanks Rob!

Comment 50

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

Anthony, can you execute a test-run on our qa-set machine for trunk builds to make sure that the latest update works also on that machine? I don't wanna run into trouble when we have to start update testing tomorrow for beta3. Thanks.

Comment 51

[Robert Strong (they/them - no direct email)]

I copied the tests in softwareUpdate along with the shared module over to mozmill mozilla1.9.2 and the tests passed with 3.6.6 -> 3.6.8 and 3.6.7 -> 3.6.8. To test I hardcoded update.download("release"); in the test files instead of using update.download(persisted.channel); since it failed without persisted.channel being set and it isn't clear to me where persisted.channel is set.

Comment 52

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

Anthony has run some tests with out automation scripts and tests failed on trunk. Complete output can be found here:

http://spreadsheets.google.com/ccc?key=0AmkRt0ylPb8zdFJHSGNIMFdLQ2xXREgzMXJydl8tZ1E&hl=en&authkey=CM_1p_0K#gid=0

I will have to investigate that before we should go on with backporting.

The automation scripts can be found here:
http://hg.mozilla.org/qa/mozmill-automation/

Rob, what you need is:
http://hg.mozilla.org/qa/mozmill-automation/file/tip/testrun_update.py

Comment 53

[Robert Strong (they/them - no direct email)]

Can you provide an example of the command line to use and what the environment needs to be? I need to specify the path to the mozmill repo and the binary to use and it doesn't have those options as far as I can tell. I wonder if it is the python script causing this since with the channel hardcoded and running mozmill-restart with the appropriate command line options it passed for me.

Comment 54

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

testrun_update.py automatically pulls the latest version of the tests from mozmill-tests. You should start it like:

testrun_update.py --channel="beta" /Volumes/folder/with/installer-builds

> ls /Volumes/folder/with/installer-builds
firefox-3.6.dmg
firefox-3.6.7.dmg

It will check all those builds in the specified folder for updates and performs partial/complete and fallbacks. If no fallback is wanted add the --no-fallback option.

Comment 55

[Robert Strong (they/them - no direct email)]

Attachment: patch - clear the cache before downloading an update

This fixes the test by clearing the cache and can be removed after bug 514040 is fixed.

Comment 56

[Robert Strong (they/them - no direct email)]

Attachment: testrun.py - reorder tests [checked-in]

Henrik, I prefer this approach over clearing the cache. Basically what is going on is the client is downloading the same patch multiple times which places the download in the cache and makes it so the download is completed before the test gets to the downloading page for the fallback case. Since there is code that will skip the download page if it is already downloaded for the fallback case the download page immediately goes to the finished page. So, performing the fallback test before the direct test fixes it.

Comment 57

[Robert Strong (they/them - no direct email)]

Perhaps Anthony can apply the patch to testrun.py and run the tests to see if that fixes this?

Comment 58

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

Comment on attachment 464332 [details] [diff] [review]
testrun.py - reorder tests [checked-in]

>+        if not self.no_fallback:
>+            # Run fallback update test
>+            fallback_data = self.run_update_tests(True)
>+            fallback_result = fallback_data.get("success", False)
>+
>+            # Restoring application backup to run direct update tests
>+            self.restore_binary()
>+
>         # Run direct update test
>         direct_data = self.run_update_tests(False)
>         direct_result = direct_data.get("success", False)
>             
>-        # Restoring application backup and run fallback update tests
>-        if not self.no_fallback:
>-            self.restore_binary()
>-
>-            # Run fallback update test
>-            fallback_data = self.run_update_tests(True)
>-            fallback_result = fallback_data.get("success", False)
>-

This patch only flips the order how we execute a direct and a fallback update test. We backup the application binary right after the installation so it will not have anything in the cache when we run the restore after the direct update. Or do I miss something related to the cache?

Comment 59

[Robert Strong (they/them - no direct email)]

The cache is profile specific and not app installation specific. I haven't looked at the mozmill code that deals with profiles but it appeared that it was using the same profile from the failure I was seeing when using testrun_update.py. By running the fallback test first and direct second the tests passed for me several times.

Comment 60

[Robert Strong (they/them - no direct email)]

Should also mention that just clearing the cache before running the fallback tests also fixed this for me which is another reason why I suspect it is using the same profile.

Comment 61

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

Which version of Mozmill are you using Robert? With Mozmill 1.4.2b2 or later we have a global timeout which we trigger now if the download is too slow. I see this problem on my local box. Can you please check the output for the exact failure? Do you get something like the following for testFallbackUpdate_ErrorPatching:

TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed

Comment 62

[Robert Strong (they/them - no direct email)]

(In reply to comment #61)
> Which version of Mozmill are you using Robert? With Mozmill 1.4.2b2 or later we
> have a global timeout which we trigger now if the download is too slow. I see
> this problem on my local box. Can you please check the output for the exact
> failure? Do you get something like the following for
> testFallbackUpdate_ErrorPatching:
> 
> TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed
Since mozmill.exe doesn't contain version info and there isn't a version command line option how does one check the version?

Also, the problem wasn't that the download was too slow... it is about the download coming from the cache for the fallback case.

Comment 63

[Robert Strong (they/them - no direct email)]

btw: I don't have any more time to spend on this with my Firefox 4.0 workload though I believe the reordering will work and you can verify by trying it out. If for whatever reason it doesn't the patch I obsoleted before the current patch should also fix it.

Comment 64

[Robert Strong (they/them - no direct email)]

2nd btw: the tests passed with the builds listed in comment #36 when they are executed via mozmill-restart and they don't when executed with testrun_update.py which is another reason I believe it is using the same profile.

Comment 65

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

(In reply to comment #62)
> Since mozmill.exe doesn't contain version info and there isn't a version
> command line option how does one check the version?

Simply run mozmill without the -t option and check the add-ons manager for the version.

(In reply to comment #63)
> btw: I don't have any more time to spend on this with my Firefox 4.0 workload
> though I believe the reordering will work and you can verify by trying it out.
> If for whatever reason it doesn't the patch I obsoleted before the current
> patch should also fix it.

We have never experienced that problem you are describing here. In the testrun_update.py script we are using different profiles. We do not change any behavior supported by Mozmill itself. Can you tell us at least the commands you run when you see that problem?

Comment 66

[Robert Strong (they/them - no direct email)]

(In reply to comment #65)
> (In reply to comment #62)
> > Since mozmill.exe doesn't contain version info and there isn't a version
> > command line option how does one check the version?
> 
> Simply run mozmill without the -t option and check the add-ons manager for the
> version.
1.4.2b1

> (In reply to comment #63)
> > btw: I don't have any more time to spend on this with my Firefox 4.0 workload
> > though I believe the reordering will work and you can verify by trying it out.
> > If for whatever reason it doesn't the patch I obsoleted before the current
> > patch should also fix it.
> 
> We have never experienced that problem you are describing here. In the
> testrun_update.py script we are using different profiles. We do not change any
> behavior supported by Mozmill itself. Can you tell us at least the commands you
> run when you see that problem?
Of course... I ran the command you gave me in comment #54
testrun_update.py --channel="beta" <path to dir containing builds>

When I have a moment I'll verify whether the direct and the fallback tests are using different profiles and hopefully find time to figure out what is going on.

Comment 67

[Robert Strong (they/them - no direct email)]

It doesn't appear to be using the same profile. Regretfully, I don't have anymore time to spend on this until next month.

Comment 68

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

Comment on attachment 464332 [details] [diff] [review]
testrun.py - reorder tests [checked-in]

We haven't seen this behavior so far in the last year. It would need more investigation why it happens for you Robert. We should probably move this out to its own bug. For now I don't see a reason why we have to flip the execution of direct vs. fallback updates. As you have seen, we are using different profiles so caching shouldn't be the problem. Please file a new bug when you have a bit of time. Thanks.

Comment 69

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

Anthony, once bug 586652 has been landed, it would be great if you could execute another testrun for trunk builds.

Comment 70

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

(In reply to comment #69)
> Anthony, once bug 586652 has been landed, it would be great if you could
> execute another testrun for trunk builds.

The fix for bug 586652 has been landed and the scripts on our machines have been updated. Anthony, please execute a test-run for all kinds of builds we have for Firefox 3.7/4.0. Thanks.

Comment 71

[u279076]

I've posted the raw log and formatted results to a wiki page found here -> https://wiki.mozilla.org/User:Ashughes/bug567258

Comment 72

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

(In reply to comment #71)
> I've posted the raw log and formatted results to a wiki page found here ->
> https://wiki.mozilla.org/User:Ashughes/bug567258

So the failures I can see here are mostly the ones Robert has been mentioned earlier due to the downloading wizard page. Those failures we haven't seen in the past. Robert, was the skipping of this page part of your latest patches or how long do we handle it that way already?

If the download is cached, the question is from where do we get this file then. We have a new profile for the fallback case. This is suspicious.

As said above I'm not really happy by swapping both test-runs. Once I'm back from vacation over-next week I will have a look into it.

Thanks Anthony for those data.

Comment 73

[Robert Strong (they/them - no direct email)]

I suspect this is due to adding code that immediately move to the finished page when the download has already completed instead of showing the download page since that busted the ui.

Regarding "If the download is cached, the question is from where do we get this file then." - from the cache and it is applicable to the fallback case since fallback tests 1 - 3 use the same profile.

One thing to note is that my tests of the same builds using mozmill-restart succeeded vs. testrun_update.py failing... would be interesting if that is actually the case and it also uses the same profile which I suspect it must.

I understand not being happy about changing the test but that is by far the least evil of things to change if it does in fact fix mozmill which it did for me.

Comment 74

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

(In reply to comment #73)
> I understand not being happy about changing the test but that is by far the
> least evil of things to change if it does in fact fix mozmill which it did for
> me.

I would like to move this to its own bug. I have tried a couple of times to reproduce this behavior but I'm not able to yet. It only happened once with Windows XP but that's all. I will file a follow-up bug.

Comment 75

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

Looks like the backport patch isn't a 1:1 copy of the current files from the default branch. The fallback update is raising an error for the error patching wizard page. Sadly I'm also blocked by bug 569642 which doesn't let me manually run a fallback update.

Comment 76

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

Also when running a fallback for 3.6.8 to 3.6.9pre the test get stuck after the restart. Firefox tries to connect to the update server but never establishes a connection.

Comment 77

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

Attachment: Combined patch for 1.9.2

After sorting out the update issues from yesterday and having the the betatest snippets available now, I was able to verify the functionality of the backport patch on OS X:

* 3.6.8 => 3.6.9, minor, en-US, partial+fallback, betatest, 2010-08-25, '''PASS'''
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 ID:20100722150226
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9 ID:20100824144458
** Passed 7 :: Failed 0 :: Skipped 0

* 3.6.7 => 3.6.9, minor, en-US, complete+fallback, betatest, 2010-08-25, '''PASS'''
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7 ID:20100713121444
** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9 ID:20100824144458
** Passed 7 :: Failed 0 :: Skipped 0

Comment 78

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

Comment on attachment 464332 [details] [diff] [review]
testrun.py - reorder tests [checked-in]

See bug 590084 comment 1 why we want to take this change. The checkin will happen via bug 590084.

Comment 79

[u279076]

Comment on attachment 468961 [details] [diff] [review]
Combined patch for 1.9.2

There are a few places in this patch which disobey the 2-space indent convention for multi-line statements and declarations.  I won't block this patch on that but we should make not of it for the refactoring work.

r+

Comment 80

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

(In reply to comment #79)
> There are a few places in this patch which disobey the 2-space indent
> convention for multi-line statements and declarations.  I won't block this
> patch on that but we should make not of it for the refactoring work.

Once Geo and me are ready with the proposal of the new style of shared modules all of our shared modules will have to be updated. It will be part of it.

Landed on 1.9.2 as:
http://hg.mozilla.org/qa/mozmill-tests/rev/2915b433814d

Let's call it fixed. Any follow-up work should happen on new bugs.

Comment 81

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

(In reply to comment #80)
> Landed on 1.9.2 as:
> http://hg.mozilla.org/qa/mozmill-tests/rev/2915b433814d

This patch was terrible broken. I even wondered how we were able to run update tests this week: http://mozmill-release.brasstacks.mozilla.com/#/update/report/09feb10ebb4e3caf4bce854e9800920d

I pushed a bustage fix: http://hg.mozilla.org/qa/mozmill-tests/rev/3eda822a77e1

Comment 82

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

This also slipped through the 4.0 patch review. Looks like the move to CommonJS for our modules made those problems visible. Before we silently failed. :/

Bustage fix for default:
http://hg.mozilla.org/qa/mozmill-tests/rev/5a1a012a9e06

Comment 83

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.