636772 - Download add-ons specified by URL instead of local path

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

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

We may want a workaround in our automation scripts for this until bug 636769 has been fixed.

Comment 1

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

It will first need the landing of bug 634567.

Comment 2

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

Attachment: WIP. Download add-ons specified by URL instead of local path. v1

This patch feels like too much of a hack to me, but it does work. I'd appreciate some feedback.

Comment 3

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

Comment on attachment 516436 [details] [diff] [review]
WIP. Download add-ons specified by URL instead of local path. v1

>+    def prepare_addons(self):
>+        """ Prepare the addons for the test run. """
>+        [self.download_addon(location, tempfile.gettempdir()) for location in self.addon_list if location.startswith("http") or location.startswith("ftp")]

Just do it in a normal 'for' loop. Only use this way if you return a built-up array and re-assign it to a variable. Also if the add-on location is an URL update it's value to the the path in the temp folder. We shouldn't refer to the URL later on anymore and always use the local path.

We should probably hold those downloaded XPI files in a separate array to ensure that we really only remove those XPIs and no others the user has been specified as local path.

Otherwise it looks fine.

Comment 4

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

Attachment: WIP. Download add-ons specified by URL instead of local path. v2

(In reply to comment #3)
> Comment on attachment 516436 [details] [diff] [review]
> WIP. Download add-ons specified by URL instead of local path. v1
> 
> >+    def prepare_addons(self):
> >+        """ Prepare the addons for the test run. """
> >+        [self.download_addon(location, tempfile.gettempdir()) for location in self.addon_list if location.startswith("http") or location.startswith("ftp")]
> 
> Just do it in a normal 'for' loop. Only use this way if you return a built-up
> array and re-assign it to a variable. Also if the add-on location is an URL
> update it's value to the the path in the temp folder. We shouldn't refer to the
> URL later on anymore and always use the local path.
> 
> We should probably hold those downloaded XPI files in a separate array to
> ensure that we really only remove those XPIs and no others the user has been
> specified as local path.
> 
> Otherwise it looks fine.

Now stores remote and local add-ons in separate lists for clean removal of downloaded add-ons. Please provide feedback.

Comment 5

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

Comment on attachment 516592 [details] [diff] [review]
WIP. Download add-ons specified by URL instead of local path. v2

>+        self.remote_addons = [ ]
>+        self.local_addons = [ ]

Only store the addons we have downloaded in a separate list which can then be used in the clear up method of the testrun. The local path of the downloaded addon should directly replace the entry in addon_list. I would suggest to call the new array removable_addons.

>+        for path in self.remote_addons:
>+            try:
>+                # Remove downloaded add-on
>+                if os.path.exists(path):
>+                    print "*** Removing downloaded add-on '%s'." % path
>+                    os.remove(path)
>+            except:
>+                pass

If the download wasn't successful we directly exit the test run. So there should be no need for the if clause. Remove it directly, and add logging output to the except part, so we know if deleting a file has was not successful.

>+        self.prepare_addons()
>         self.prepare_tests()
>         self._mozmill.run()
>+        self.remove_downloaded_addons()

I was checking the code in detail now and those lines should be part of Testrun.run method. Otherwise we would download and remove the addons for each binary we execute.

>-                # Download the add-on
>-                xpi_path = self.download_addon(url, tempfile.gettempdir())
>-
>                 # Run normal tests if some exist
>                 self.test_path = os.path.join(self._base_path, 'tests')
>                 if os.path.isdir(self.test_path):
>                     try:
>                         self.restart_tests = False
>-                        self.addon_list.append(xpi_path)
>+                        self.addon_list.append(url)
>                         TestRun.run_tests(self)
>                     except Exception, inst:
>                         print "%s." % inst
>-                    finally:
>-                        self.addon_list.remove(xpi_path)

Leave this part as it is. The add-ons testrun has to handle that add-on on its own.

Comment 6

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

Attachment: Download add-ons specified by URL instead of local path. v3

(In reply to comment #5)
> Comment on attachment 516592 [details] [diff] [review]
> WIP. Download add-ons specified by URL instead of local path. v2
> 
> >+        self.remote_addons = [ ]
> >+        self.local_addons = [ ]
> 
> Only store the addons we have downloaded in a separate list which can then be
> used in the clear up method of the testrun. The local path of the downloaded
> addon should directly replace the entry in addon_list. I would suggest to call
> the new array removable_addons.

I've called the new array downloaded_addons as this seemed to make the most sense. I've also renamed addon_list to simply addons so that we can define addon_list in the prepare_addons method.

> >+        for path in self.remote_addons:
> >+            try:
> >+                # Remove downloaded add-on
> >+                if os.path.exists(path):
> >+                    print "*** Removing downloaded add-on '%s'." % path
> >+                    os.remove(path)
> >+            except:
> >+                pass
> 
> If the download wasn't successful we directly exit the test run. So there
> should be no need for the if clause. Remove it directly, and add logging output
> to the except part, so we know if deleting a file has was not successful.

Done.

> >+        self.prepare_addons()
> >         self.prepare_tests()
> >         self._mozmill.run()
> >+        self.remove_downloaded_addons()
> 
> I was checking the code in detail now and those lines should be part of
> Testrun.run method. Otherwise we would download and remove the addons for each
> binary we execute.

Good catch. Moved.

> >-                # Download the add-on
> >-                xpi_path = self.download_addon(url, tempfile.gettempdir())
> >-
> >                 # Run normal tests if some exist
> >                 self.test_path = os.path.join(self._base_path, 'tests')
> >                 if os.path.isdir(self.test_path):
> >                     try:
> >                         self.restart_tests = False
> >-                        self.addon_list.append(xpi_path)
> >+                        self.addon_list.append(url)
> >                         TestRun.run_tests(self)
> >                     except Exception, inst:
> >                         print "%s." % inst
> >-                    finally:
> >-                        self.addon_list.remove(xpi_path)
> 
> Leave this part as it is. The add-ons testrun has to handle that add-on on its
> own.

Reverted.

Comment 7

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

Comment on attachment 516628 [details] [diff] [review]
Download add-ons specified by URL instead of local path. v3

>     def __init__(self, addon_list=None, application="firefox", binaries=None,
>                  debug=False, logfile=None, report_url=None,
>                  repository_path=None, repository_url=None,
>                  restart_tests=False, screenshot_path=None,
>                  test_path=None, timeout=None):
>-        self.addon_list = addon_list or []
>+        self.addons = addon_list or []

When you change that please also change the parameter name. We have to line up with the sub-classes.

>+    def prepare_addons(self):
>+        """ Prepare the addons for the test run. """
>+        self.downloaded_addons = [ ]
>+        self.addon_list = [ ]

Initializing class properties should be done in the constructor.

>+        for location in self.addons:

Instead of 'location' use 'addon'.

>             # Run tests for each binary
>             for binary in self.binaries:
>                 try:
>                     self.prepare_binary(binary)
>                     self.prepare_repository()
>+                    self.prepare_addons()

It has to be located outside of the for loop. Now you are still executing it for each binary, while we want to do it only once.

>                 finally:
>                     self.cleanup_binary(binary)
>+                    self.remove_downloaded_addons()

Same for removing the downloaded add-ons.

Comment 8

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

Attachment: Download add-ons specified by URL instead of local path. v3.1

(In reply to comment #7)
> Comment on attachment 516628 [details] [diff] [review]
> Download add-ons specified by URL instead of local path. v3
> 
> >     def __init__(self, addon_list=None, application="firefox", binaries=None,
> >                  debug=False, logfile=None, report_url=None,
> >                  repository_path=None, repository_url=None,
> >                  restart_tests=False, screenshot_path=None,
> >                  test_path=None, timeout=None):
> >-        self.addon_list = addon_list or []
> >+        self.addons = addon_list or []
> 
> When you change that please also change the parameter name. We have to line up
> with the sub-classes.

Done.

> >+    def prepare_addons(self):
> >+        """ Prepare the addons for the test run. """
> >+        self.downloaded_addons = [ ]
> >+        self.addon_list = [ ]
> 
> Initializing class properties should be done in the constructor.

Done.

> >+        for location in self.addons:
> 
> Instead of 'location' use 'addon'.

Done.

> >             # Run tests for each binary
> >             for binary in self.binaries:
> >                 try:
> >                     self.prepare_binary(binary)
> >                     self.prepare_repository()
> >+                    self.prepare_addons()
> 
> It has to be located outside of the for loop. Now you are still executing it
> for each binary, while we want to do it only once.
> 
> >                 finally:
> >                     self.cleanup_binary(binary)
> >+                    self.remove_downloaded_addons()
> 
> Same for removing the downloaded add-ons.

My mistake, sorry. Fixed now.

Comment 9

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

Comment on attachment 516725 [details] [diff] [review]
Download add-ons specified by URL instead of local path. v3.1

Looks and works perfectly. Thanks Dave!

Comment 10

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

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/6fa7dd00ffb2