634567 - Allow test runs to specify add-ons to be installed

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

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

In order to replicate reported performance related issues we need to be able to run endurance tests with add-ons installed.

Comment 1

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

Attachment: Parameter for specifying add-ons to install for the testrun. v1.0

Comment 2

[u279076]

Comment on attachment 514609 [details] [diff] [review]
Parameter for specifying add-ons to install for the testrun. v1.0

Seems ok to me, r+.

Comment 3

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

Comment on attachment 514609 [details] [diff] [review]
Parameter for specifying add-ons to install for the testrun. v1.0

As talked about in our 1-1 yesterday, we do not want to change the behavior of the --addons command line option. Please simply pass it through to Mozmill itself, so that all test-runs will also be able to use that option to run tests with the given local add-on installed.

Downloading add-ons is a nice enhancement but it's out of scope for this bug and can be filed as enhancement. And finally it should probably land in Mozmill itself.

Comment 4

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

Attachment: Parameter for specifying add-ons to install for the testrun. v2

Rewritten the patch to use the existing Mozmill command line parameter

Comment 5

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

Attachment: Add installed extension details to the persisted object. v1

Comment 6

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

Comment on attachment 514954 [details] [diff] [review]
Add installed extension details to the persisted object. v1

>+    persisted.addons = [];

Do it the same way as for the results and define that empty array already in the Endurance class of the testrun module.

>+  Components.utils.import("resource://gre/modules/AddonManager.jsm");

Put this at the top directly below the license.

>+  if (persisted.addons) {
>+    AddonManager.getAllAddons(function(aAddons) {

nit: space between function and (

>+      aAddons.forEach(function (addon) {
>+        if (addon.type === "extension") {
>+          persisted.addons.push({
>+            id : addon.id,
>+            name : addon.name,
>+            version : addon.version

Please also add isCompatible to that list. That's a kinda important information for us.

In general we should move the retrieval of addons into the addons.js module, which then simply returns the array. I would also like to use it for the update tests.

Also please try to file separate bugs for automation script and mozmill tests related patches. It's a bit confusing that way.

Comment 7

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

Comment on attachment 514952 [details] [diff] [review]
Parameter for specifying add-ons to install for the testrun. v2

>+        TestRun.__init__(self,
>+                         addon_list=addons,

Any reason why you are using addon_list in TestRun? 

>@@ -581,17 +587,19 @@ class EnduranceTestRun(TestRun):
[..]
>     def update_report(self, report):
>+        report['addons'] = self._mozmill.persisted['addons']

That has to be part of the TestRun class, which should also initialize the 'addons' array.

Otherwise it looks good.

Comment 8

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

Oh and please don't forget to file the Mozmill bug to enhance the handling of --addons to download the add-on if an URL has been specified.

Comment 9

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

Made this bug specific to the automation scripts. I will raise another bug to cover reporting the installed add-ons from with the tests.

Comment 10

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

Attachment: Allow test runs to specify add-ons to be installed v2.1

(In reply to comment #7)
> Comment on attachment 514952 [details] [diff] [review]
> Parameter for specifying add-ons to install for the testrun. v2
> 
> >+        TestRun.__init__(self,
> >+                         addon_list=addons,
> 
> Any reason why you are using addon_list in TestRun? 

Not sure I understand this comment. The argument was already named addon_list, are you suggesting I rename it?

> >@@ -581,17 +587,19 @@ class EnduranceTestRun(TestRun):
> [..]
> >     def update_report(self, report):
> >+        report['addons'] = self._mozmill.persisted['addons']
> 
> That has to be part of the TestRun class, which should also initialize the
> 'addons' array.

Done.

Comment 11

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

(In reply to comment #10)
> > Any reason why you are using addon_list in TestRun? 
> 
> Not sure I understand this comment. The argument was already named addon_list,
> are you suggesting I rename it?

As talked on IRC that was my fault. Leave it as is and I will take care of for the script rewrite.

Comment 12

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

Comment on attachment 515047 [details] [diff] [review]
Allow test runs to specify add-ons to be installed v2.1

>         self._mozmill.addon_list = self.addon_list
>+        self._mozmill.persisted['addons'] = [ ]
>+
>         self._mozmill.binary = self._application

Please don't inject this code in the middle of the mozmill wrapper assignments. Instead place it right before the persisted handling of the screenshot_path.

Otherwise it looks good. r=me with that change. I also assume you have tested all modified scripts.

Comment 13

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

Attachment: Allow test runs to specify add-ons to be installed v2.2

(In reply to comment #12)
> Comment on attachment 515047 [details] [diff] [review]
> Allow test runs to specify add-ons to be installed v2.1
> 
> >         self._mozmill.addon_list = self.addon_list
> >+        self._mozmill.persisted['addons'] = [ ]
> >+
> >         self._mozmill.binary = self._application
> 
> Please don't inject this code in the middle of the mozmill wrapper assignments.
> Instead place it right before the persisted handling of the screenshot_path.

Done.

> Otherwise it looks good. r=me with that change. I also assume you have tested
> all modified scripts.

Not all scripts as I'm not familiar with them all. I'd appreciate some assistance with this before landing the patch.

Comment 14

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

Comment on attachment 515107 [details] [diff] [review]
Allow test runs to specify add-ons to be installed v2.2

Looks fine. I'm checking the other testruns now to make it works everywhere.

Comment 15

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

Comment on attachment 515107 [details] [diff] [review]
Allow test runs to specify add-ons to be installed v2.2

Darn. We collide with the add-ons test-run because it already uses the --addons option to select a single add-on to run. We have to change this script. I would say lets rename it to --selected-addons for now and keep --addons for the forward to Mozmill.

Comment 16

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

Attachment: Allow test runs to specify add-ons to be installed v3

(In reply to comment #15)
> Comment on attachment 515107 [details] [diff] [review]
> Allow test runs to specify add-ons to be installed v2.2
> 
> Darn. We collide with the add-ons test-run because it already uses the --addons
> option to select a single add-on to run. We have to change this script. I would
> say lets rename it to --selected-addons for now and keep --addons for the
> forward to Mozmill.

Raised bug 637281 as a dependency and submitted a patch. This patch adds the functionality to run add-ons tests with additional add-ons installed.

Comment 17

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

Comment on attachment 515605 [details] [diff] [review]
Allow test runs to specify add-ons to be installed v3

>                 # Run normal tests if some exist
[..]
>                     try:
>                         self.restart_tests = False
>-                        self.addon_list = [xpi_path]
>+                        self.addon_list.append(xpi_path)
>                         TestRun.run_tests(self)
>+                        self.addon_list.remove(xpi_path)
>                     except Exception, inst:
>                         print "%s." % inst

That will not work if the testrun itself raises an exception. In such a case we have added the extension under test to the addon list but don't remove it anymore. We would need a finally clause here.

>-                        self.addon_list = [xpi_path]
>+                        self.addon_list.append(xpi_path)
>                         TestRun.run_tests(self)
>+                        self.addon_list.remove(xpi_path)
>                     except Exception, inst:

Same for the restart tests.

Comment 18

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

Attachment: Allow test runs to specify add-ons to be installed v3.1

(In reply to comment #17)
> Comment on attachment 515605 [details] [diff] [review]
> Allow test runs to specify add-ons to be installed v3
> 
> >                 # Run normal tests if some exist
> [..]
> >                     try:
> >                         self.restart_tests = False
> >-                        self.addon_list = [xpi_path]
> >+                        self.addon_list.append(xpi_path)
> >                         TestRun.run_tests(self)
> >+                        self.addon_list.remove(xpi_path)
> >                     except Exception, inst:
> >                         print "%s." % inst
> 
> That will not work if the testrun itself raises an exception. In such a case we
> have added the extension under test to the addon list but don't remove it
> anymore. We would need a finally clause here.
> 
> >-                        self.addon_list = [xpi_path]
> >+                        self.addon_list.append(xpi_path)
> >                         TestRun.run_tests(self)
> >+                        self.addon_list.remove(xpi_path)
> >                     except Exception, inst:
> 
> Same for the restart tests.

Done. Let me know if this is what you were expecting.

Comment 19

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

Comment on attachment 516348 [details] [diff] [review]
Allow test runs to specify add-ons to be installed v3.1

Ok, that seems to cover everything even with the exception I can't check if the add-ons are reported correctly. The entry is created in the reports, so I think this patch is good to be landed.

Comment 20

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

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/4ff22121251f