1071590 - Refactor update tests for stability and backup/restore of channel files

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

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

We should refactor the update tests and make use of the new restart tests style by having all code located in a single file. This might guard us from problems like bug 972912, when Mozmill does not run all the tests. 

With this done we should also extend the code, which I have added over on bug 1063584, to backup and restore the entries in the channel-prefs.js and update-settings.ini file. Best would be if we can do it in the tests, but if a crash would stop the test, we should forward the original content via the persisted object to the Python side, which can restore the files then.

Comment 1

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

Attachment: refactor v1

Ok, so here the first shot for the update test refactoring. Please have a look and test it also for nightly. I expect not all being perfect. 

Also please note that we can fail in resetting the update-settings.ini file.  But given that this is currently not used, I wouldn't worry that much.

Comment 2

[Andrei Eftimie]

Comment on attachment 8495255 [details] [diff] [review]
refactor v1

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

Big patch. Great work here.
Seems to work fine. Tested OSX Nightly, couple locales and different Nightly versions.

Just a few items mentioned inline.

::: firefox/lib/software-update.js
@@ +9,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  // Include required modules
>  var { assert, expect } = require("../../lib/assertions");

Since we're already here we can also remove this include.

@@ +266,3 @@
>        disabled_addons : prefs.preferences.getPref(PREF_DISABLED_ADDONS, ''),
>        locale : utils.appInfo.locale,
> +      mar_channels : this.marChannels.channels,

We still get an array here. AFAIK this is only for the dashboard, but we currently don't even show it in the dashboard.
Any specific plans for this?

For the reports I've checked we have 1 channel, so in the report it used to look like this:
> "mar_channels": "firefox-mozilla-central",

Now it looks like:
> "mar_channels": [
>    "firefox-mozilla-central"
>  ],

I'm not against this change, just that we are aware of it.

@@ +722,5 @@
> +/**
> + * Class to handle the update channel as listed in channel-prefs.js
> + */
> +function UpdateChannel() {
> +  var file = file = Services.dirsvc.get("PrfDef", Ci.nsIFile);

There's an extra `file =` here.

::: firefox/tests/update/testFallbackUpdate/test3.js
@@ -12,5 @@
> -function setupModule(aModule) {
> -  aModule.controller = mozmill.getBrowserController();
> -  aModule.update = new softwareUpdate.softwareUpdate();
> -
> -  persisted.updates[persisted.update.index].fallback = true;

We miss to mark this as a fallback update in the new refactored test.

::: firefox/tests/update/testFallbackUpdate/testUpdate.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Not sure how important this is, but for the DirectUpdate test, the new single-test is marked as a rename from test1, thus preserves _some_ history.
Now the refactoring is major, so I'm not sure how much help that might be.

But we should keep it similar for the fallback update test. Either keep both as renames from test1, or mark both as new files.

@@ +66,5 @@
> +  // Check if the user has permissions to run the update
> +  assert.ok(update.allowed, "User has permissions to update the build");
> +
> +  // Sanity check for the about dialog
> +  update.checkAboutDialog(controller);

This wasn't called before for the Fallback test, but I think it is welcome to have a check for it.

@@ +153,5 @@
> + *
> + * @param {boolean} aFallback
> + *        True in case it is a fallback update
> + */
> +function initUpdateTests(aFallback=false) {

This method is identical for both Direct and Fallback tests. Could we find a way to keep things DRY?

::: lib/files.js
@@ +10,1 @@
>  var { assert, expect } = require("assertions");

Same here, since this is refactored in a significant way, lets also remove this include.

@@ +91,5 @@
>    this._writer = iniFactory.createINIParser(this._file)
>                             .QueryInterface(Ci.nsIINIParserWriter);
>  }
>  
> +INIFile.prototype = Object.create(File.prototype);

yay for Object.create!

@@ +99,5 @@
> + * Return a nsIFile reference of the INI file
> + *
> + * @returns {nsIFile} File reference
> + */
> +INIFile.prototype.__defineGetter__('file', function() {

nit: please use double quotes for consistency.

Comment 3

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

(In reply to Andrei Eftimie from comment #2)
> ::: firefox/lib/software-update.js
> @@ +9,5 @@
> >  
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  
> >  // Include required modules
> >  var { assert, expect } = require("../../lib/assertions");
> 
> Since we're already here we can also remove this include.

Sounds like an idea. :) Did that.

> @@ +266,3 @@
> >        disabled_addons : prefs.preferences.getPref(PREF_DISABLED_ADDONS, ''),
> >        locale : utils.appInfo.locale,
> > +      mar_channels : this.marChannels.channels,
> 
> We still get an array here. AFAIK this is only for the dashboard, but we
> currently don't even show it in the dashboard.
> Any specific plans for this?

Sure, that's why I have added this to the reported data. That was actually a request by RelEng, so that we can see which channels we have used. Once it has been landed in the Mozmill tests, and gets send to the dashboard, it's code can be updated to show this data.

> For the reports I've checked we have 1 channel, so in the report it used to
> look like this:
> > "mar_channels": "firefox-mozilla-central",
> 
> Now it looks like:
> > "mar_channels": [
> >    "firefox-mozilla-central"
> >  ],
> 
> I'm not against this change, just that we are aware of it.

The above doesn't scale, and forces workarounds (always split the string when used) in our tests. I added this previously with my patch on bug 1063584. So it's another new enhancement, and the dashboard doesn't know about it yet.

> @@ +722,5 @@
> > +/**
> > + * Class to handle the update channel as listed in channel-prefs.js
> > + */
> > +function UpdateChannel() {
> > +  var file = file = Services.dirsvc.get("PrfDef", Ci.nsIFile);
> 
> There's an extra `file =` here.

fixed.

> ::: firefox/tests/update/testFallbackUpdate/test3.js
> @@ -12,5 @@
> > -function setupModule(aModule) {
> > -  aModule.controller = mozmill.getBrowserController();
> > -  aModule.update = new softwareUpdate.softwareUpdate();
> > -
> > -  persisted.updates[persisted.update.index].fallback = true;
> 
> We miss to mark this as a fallback update in the new refactored test.

No, this was actually broken so far, and there could be situations when a fallback test was reported as a direct test, e.g. early abort or test3.js not run. Please see the new initUpdateTests() method, which has the aFallback parameter, and which gets set directly during setup to avoid this possible race condition.

> ::: firefox/tests/update/testFallbackUpdate/testUpdate.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Not sure how important this is, but for the DirectUpdate test, the new
> single-test is marked as a rename from test1, thus preserves _some_ history.
> Now the refactoring is major, so I'm not sure how much help that might be.
> 
> But we should keep it similar for the fallback update test. Either keep both
> as renames from test1, or mark both as new files.

My fault here. I simply dropped the finished direct update test into the fallback folder, and deleted all other tests. I agree here and will not rename test1.js.

> @@ +66,5 @@
> > +  // Check if the user has permissions to run the update
> > +  assert.ok(update.allowed, "User has permissions to update the build");
> > +
> > +  // Sanity check for the about dialog
> > +  update.checkAboutDialog(controller);
> 
> This wasn't called before for the Fallback test, but I think it is welcome
> to have a check for it.

Yes, we missed to add it for the fallback tests. I just fixed that.

> @@ +153,5 @@
> > + *
> > + * @param {boolean} aFallback
> > + *        True in case it is a fallback update
> > + */
> > +function initUpdateTests(aFallback=false) {
> 
> This method is identical for both Direct and Fallback tests. Could we find a
> way to keep things DRY?

Maybe I can move out the modifications of the preferences, and only keep the persisted affected code. Lemme check what I can do here.

> ::: lib/files.js
> @@ +10,1 @@
> >  var { assert, expect } = require("assertions");
> 
> Same here, since this is refactored in a significant way, lets also remove
> this include.

done.

> @@ +91,5 @@
> >    this._writer = iniFactory.createINIParser(this._file)
> >                             .QueryInterface(Ci.nsIINIParserWriter);
> >  }
> >  
> > +INIFile.prototype = Object.create(File.prototype);
> 
> yay for Object.create!

That was actually a big problem here to forward the passed in nsIFile instance via the constructor. I would propose we do at least this call also for browser. I didn't wanted to change it here given the backport capabilities, due to this patch has to land on every branch.

> @@ +99,5 @@
> > + * Return a nsIFile reference of the INI file
> > + *
> > + * @returns {nsIFile} File reference
> > + */
> > +INIFile.prototype.__defineGetter__('file', function() {
> 
> nit: please use double quotes for consistency.

done.

Thanks for this deep review. Updated patch will be up soon.

Comment 4

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

Attachment: refactor v2

Updated patch. Requesting review from Andreea now to get another pair of eyes on it.

Comment 5

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8495859 [details] [diff] [review]
refactor v2

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

Tested, looks good. Just a small comment.

::: firefox/tests/update/testDirectUpdate/testUpdate.js
@@ +13,5 @@
> +
> +const PREF_UPDATE_LOG = "app.update.log";
> +const PREF_UPDATE_URL_OVERRIDE = "app.update.url.override";
> +
> +

There are double lines here and before the consts, I've seen that was like before too but maybe we could remove one since we're here.

::: firefox/tests/update/testFallbackUpdate/testUpdate.js
@@ +12,5 @@
> +
> +const PREF_UPDATE_LOG = "app.update.log";
> +const PREF_UPDATE_URL_OVERRIDE = "app.update.url.override";
> +
> +

Same here.

Comment 6

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #5)
> ::: firefox/tests/update/testDirectUpdate/testUpdate.js
> @@ +13,5 @@
> > +
> > +const PREF_UPDATE_LOG = "app.update.log";
> > +const PREF_UPDATE_URL_OVERRIDE = "app.update.url.override";
> > +
> > +
> 
> There are double lines here and before the consts, I've seen that was like
> before too but maybe we could remove one since we're here.

That's what I love for PEP8 in Python. It makes it easier to visually separate global blocks. As we talked on IRC it would also be good for us. I have updated the coding style on MDN appropriately.

Comment 7

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

Comment on attachment 8495859 [details] [diff] [review]
refactor v2

https://hg.mozilla.org/qa/mozmill-tests/rev/93e2b7ac3507 (default)

Comment 8

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

Everything looked fine for today's Nightly updates. So I'm going to backport this patch to Aurora.

https://hg.mozilla.org/qa/mozmill-tests/rev/cf25c3baedf2 (aurora)

If all is fine on Aurora too, I might even consider backporting to beta, so we can run some tests later today to be prepared for the next beta.

Comment 9

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

Attachment: backport beta v1

Except of an application disconnect on one Windows platform all tests were working as expected for Aurora:

http://mozmill-staging.blargon7.com/#/update/reports?app=All&branch=34.0&platform=All&from=2014-09-26&to=2014-09-26

I also applied the test on beta, but had to do some minor updates to comments and function signatures due to the prefix changes (SU_). There was no single code related change necessary. So I hope it's ok to take over the r+ for it. Updated patch just for reference.

Comment 10

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

https://hg.mozilla.org/qa/mozmill-tests/rev/86d2b052013f (beta)

I will trigger a couple of on-demand tests to check the patch on staging.

Comment 11

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

Everything runs even fine on beta with various ondemand runs. I'm finally going to backport it to the release and esr branches. I expect merge conflicts again due to the 'a' prefix change.

Comment 12

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

Attachment: backport release v1

For release the backport is a bit more than some single lines only. Please have a look at it.

Comment 13

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

Attachment: backport release v1.1

Actually missed to add the new files.

Comment 14

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

Attachment: backport esr24 v1

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8496170 [details] [diff] [review]
backport release v1.1

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

Diff showed changes to the parameters names only, so all good.

Comment 16

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

https://hg.mozilla.org/qa/mozmill-tests/rev/45129136e4a0 (release)
https://hg.mozilla.org/qa/mozmill-tests/rev/22a00016db62 (esr31)

Waiting for the last review until we can finally finish this off.

Comment 17

[Andrei Eftimie]

Comment on attachment 8496183 [details] [diff] [review]
backport esr24 v1

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

Works fine for me.

Comment 18

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

https://hg.mozilla.org/qa/mozmill-tests/rev/4da795d4e2e3 (esr24)

I will run some ondemand update tests for any of those newly landed patches to prove its working, also cross-channel wise.