Allow test runs to specify add-ons to be installed

RESOLVED FIXED

Status

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: davehunt, Assigned: davehunt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

In order to replicate reported performance related issues we need to be able to run endurance tests with add-ons installed.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #514609 - Flags: review?(anthony.s.hughes)
Comment on attachment 514609 [details] [diff] [review]
Parameter for specifying add-ons to install for the testrun. v1.0

Seems ok to me, r+.
Attachment #514609 - Flags: review?(anthony.s.hughes) → review+
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.
Attachment #514609 - Flags: review?(hskupin) → review-
Rewritten the patch to use the existing Mozmill command line parameter
Attachment #514609 - Attachment is obsolete: true
Attachment #514952 - Flags: review?(hskupin)
Attachment #514952 - Attachment is obsolete: true
Attachment #514954 - Flags: review?(hskupin)
Attachment #514952 - Flags: review?(hskupin)
Attachment #514952 - Attachment is obsolete: false
Attachment #514952 - Flags: review?(hskupin)
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.
Attachment #514954 - Flags: review?(hskupin) → review-
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.
Attachment #514952 - Flags: review?(hskupin) → review-
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.
Made this bug specific to the automation scripts. I will raise another bug to cover reporting the installed add-ons from with the tests.
Component: Mozmill Tests → Mozmill Automation
QA Contact: mozmill-tests → mozmill-automation
Summary: Run endurance tests with add-ons installed → Allow test runs to specify add-ons to be installed
(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.
Attachment #514952 - Attachment is obsolete: true
Attachment #515047 - Flags: review?(hskupin)
(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 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.
Attachment #515047 - Flags: review?(hskupin) → review+
(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.
Attachment #515107 - Flags: review+
Attachment #515107 - Flags: review+ → review?(hskupin)
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.
Attachment #515107 - Flags: review?(hskupin) → review+
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.
Attachment #515107 - Flags: review+ → review-
(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.
Attachment #515107 - Attachment is obsolete: true
Attachment #515605 - Flags: review?(hskupin)
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.
Attachment #515605 - Flags: review?(hskupin) → review-
(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.
Attachment #515605 - Attachment is obsolete: true
Attachment #516348 - Flags: review?(hskupin)
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.
Attachment #516348 - Flags: review?(hskupin) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/4ff22121251f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.