559929 - Run performance tests on top addons

Status: RESOLVED FIXED

(Testing :: General, defect, P2)

Description

[Heather Arthur [:harth]]

We should run Talos on the top addons on a regular basis to identify and diagnose the addons that cause the biggest performance hits.

Comment 1

[Heather Arthur [:harth]]

targetting Ts first, some initial results from running on my Linux/Vista box are here: http://code.google.com/p/harthfiles/source/browse/.

The code is going on here: http://github.com/harthur/dirtyharry

Comment 2

[alice nodelman [:alice] [:anode]]

Attachment: initial list of top addons for testing

Comment 3

[alice nodelman [:alice] [:anode]]

Attachment: add support to install .xpi addons to talos

Comment 4

[alice nodelman [:alice] [:anode]]

Patch for installing .xpi addons with talos was tested with python2.4 and worked fine - so should be good with the current installed tools on production talos boxes.

Comment 5

[Jeff Hammel]

(In reply to comment #2)
> Created an attachment (id=444788) [details]
> initial list of top addons for testing

how is this list generated?

Comment 6

[Paul [pwd]]

(In reply to comment #5)
> (In reply to comment #2)
> > Created an attachment (id=444788) [details] [details]
> > initial list of top addons for testing
> 
> how is this list generated?

It appears to be on weekly downloads.

Comment 7

[Jeff Hammel]

Comment on attachment 445217 [details] [diff] [review]
add support to install .xpi addons to talos

 - what is the longer term prospect for this project?  will
   talos consume whatever profile management stuff goes in 
   mozmill.next?

 - as per my comment on class Profile.install_addon at
   https://bugzilla.mozilla.org/show_bug.cgi?id=565733#c1 , is there
   any reason to extract the files one at at time vs. using
   Zipfile.extractall? If so, it should be documented.  Compare this
   to
   http://github.com/k0s/mozrunner/blob/master/mozrunner/profile.py#L93

 - `+        tmpdir = None`: this line is unnecessary

 - No need to copy commented code, particularly undocumented ones:
{{{
+        # description_element =
+        #
tree.find('.//{http://www.w3.org/1999/02/22-rdf-syntax-ns#}Description/')
}}}

 - it is my understanding that a new profile directory is made
   per-test;  likewise, it appears to me that the api for
   CreateTempProfileDir() with respect to what extensions means:
{{{
-            extensions: Guids and paths of extensions to link to.
   Format:
-                    {"{GUID1}" : "c:\\Path\\to\\ext1", "{GUID2}",
   "c:\\Path\\to\\ext2"}
+            extensions: list of paths to .xpi files to be installed
}}}
{{{
-        for extension in extensions:
-            link_file = open(os.path.join(extension_dir, extension),
   'w')
-            link_file.write(extensions[extension])
-            link_file.close()
+        for addon in extensions:
+            self.install_addon(profile_dir, addon)
}}}
   This being the case, the conditional logic at the top of
   `install_addon` is not necessary: 
{{{
+        if addon.endswith('.xpi'):
}}}

 - the existing dom inspection neither ensures addon_id exists nor
   does it check for duplicates

 - taking these changes and if these are appropriate conclusions/restrictions, 
   the resulting `install_addon` function can
   look something like this:
{{{
compressed_file = zipfile.ZipFile('personas-1.5.3-fx+tb.xpi')
install_rdf = compressed_file.open('install.rdf')
doc = minidom.parse(install_rdf)
desc = doc.getElementsByTagName('Description')
addon_id = None
for elem in desc:
  apps = elem.getElementsByTagName('em:targetApplication')
  if apps:
    for app in apps:

      # remove targetApplication nodes, 
      # they contain id's we aren't interested in
      elem.removeChild(app)

    addon_id = str(elem.getElementsByTagName('em:id')[0].firstChild.data)
    break # there should only be one addon_id ! 
assert addon_id, 'Addon ID not found for %s' % addon
addon_path = os.path.join(profile_path, 'extensions', addon_id)

# if an old copy is already installed, remove it
if os.path.exists(addon_path):
  shutil.rmtree(addon_path, ignore_errors=True)

compressed_file.extractall(addon_path)
}}}
   You also don't need `tempfile` with this implementation

 - likewise, you don't do any cleanup, unlike the origin code in
   mozmill. since the profile directory is deleted in
   http://mxr.mozilla.org/mozilla/source/testing/performance/talos/ttest.py#360
   this is fine for this workflow

Implementing these changes or documenting why things are the way that they
are should be sufficient for a positive review.

Comment 8

[alice nodelman [:alice] [:anode]]

Attachment: add support to buildbotcustom for downloading/configuring addons

Currently have this patch up and running in stage.  Installs each addon in turn and then runs tests against that addon - will go with updates to talos and buildbot-configs to actually turn this on.

Comment 9

[alice nodelman [:alice] [:anode]]

Attachment: add support to buildbotcustom for downloading/configuring addons (take 2)

Some staging-ism left in the previous patch by mistake.

Comment 10

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]add support to install .xpi addons to talos (take 2)

Unfortunately, zipfile doesn't have extractall or open in python2.4.  I've done some cleanup, but this won't be as streamlined until python is upgraded on the talos slaves.

Comment 11

[Jeff Hammel]

Comment on attachment 446099 [details] [diff] [review]
[checked in]add support to install .xpi addons to talos (take 2)

>Index: PerfConfigurator.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/PerfConfigurator.py,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 PerfConfigurator.py
>--- PerfConfigurator.py	7 May 2010 02:15:11 -0000	1.18
>+++ PerfConfigurator.py	19 May 2010 00:06:59 -0000
>@@ -21,17 +21,17 @@ from datetime import datetime
> from os import path
> 
> masterIniSubpath = "application.ini"
> defaultTitle = "qm-pxp01"
> 
> help_message = '''
> This is the buildbot performance runner's YAML configurator.bean
> 
>-USAGE: python PerfConfigurator.py --title title --executablePath path --configFilePath cpath --buildid id --branch branch --testDate date --resultsServer server --resultsLink link --activeTests testlist --branchName branchFullName --fast --symbolsPath path --sampleConfig cfile --browserWait seconds --remoteDevice ip_of_device --remotePort port_on_device --webServer webserver_ipaddr --deviceRoot rootdir_on_device
>+USAGE: python PerfConfigurator.py --title title --executablePath path --configFilePath cpath --buildid id --branch branch --testDate date --resultsServer server --resultsLink link --activeTests testlist --branchName branchFullName --fast --symbolsPath path --sampleConfig cfile --browserWait seconds --remoteDevice ip_of_device --remotePort port_on_device --webServer webserver_ipaddr --deviceRoot rootdir_on_device --testPrefix prefix --extension addon
> 
> example testlist: tp:tsspider:tdhtml:twinopen
> '''
> 
> class PerfConfigurator:
>     exePath = ""
>     configPath = "."
>     sampleConfig = "sample.config"
>@@ -45,21 +45,23 @@ class PerfConfigurator:
>     verbose = False
>     testDate = ""
>     useId = False
>     resultsServer = ''
>     resultsLink = ''
>     activeTests = ''
>     noChrome = False
>     fast = False
>+    testPrefix = ''
>     remoteDevice = ''
>     webServer = ''
>     deviceRoot = ''
>     _remote = False
>     port = ''
>+    extension = ''
> 
>     def _setupRemote(self, host, port = 20701):
>         import devicemanager
>         self.testAgent = devicemanager.DeviceManager(host, port)
>         self._remote = True
>     
>     def _dumpConfiguration(self):
>         """dump class configuration for convenient pickup or perusal"""
>@@ -73,16 +75,18 @@ class PerfConfigurator:
>         print " - branchName = " + self.branchName
>         print " - buildid = " + self.buildid
>         print " - currentDate = " + self.currentDate
>         print " - browserWait = " + self.browserWait
>         print " - testDate = " + self.testDate
>         print " - resultsServer = " + self.resultsServer
>         print " - resultsLink = " + self.resultsLink
>         print " - activeTests = " + self.activeTests
>+        print " - extension = " + self.extension
>+        print " - testPrefix = " + self.testPrefix
>         if (self._remote == True):
>             print " - deviceIP = " + self.remoteDevice
>             print " - devicePort = " + self.port
>             print " - webServer = " + self.webServer
>             print " - deviceRoot = " + self.deviceRoot
>         if self.symbolsPath:
>             print " - symbolsPath = " + self.symbolsPath
>     
>@@ -175,16 +179,18 @@ class PerfConfigurator:
>             if 'deviceip:' in line:
>                 newline = 'deviceip: %s\n' % self.remoteDevice
>             if 'webserver:' in line:
>                 newline = 'webserver: %s\n' % self.webServer
>             if 'deviceroot:' in line:
>                 newline = 'deviceroot: %s\n' % self.deviceRoot
>             if 'deviceport:' in line:
>                 newline = 'deviceport: %s\n' % self.port
>+            if self.extension and ('extensions : {}' in line):
>+                newline = 'extensions: ' + '\n- ' + self.extension
>             if 'remote:' in line:
>                 newline = 'remote: %s\n' % self._remote
>             if 'buildid:' in line:
>                 newline = 'buildid: ' + buildidString + '\n'
>             if 'testbranch' in line:
>                 newline = 'branch: ' + self.branch
>             if self._remote == True and ('init_url' in line):
>                 newline = self.convertUrlToRemote(line)
>@@ -212,16 +218,18 @@ class PerfConfigurator:
>                     for test in activeList: 
>                         reTestMatch = re.compile('^-\s*name\s*:\s*' + test + '\s*$')
>                         #determine if this is a test we are going to run
>                         match = re.match(reTestMatch, line)
>                         if match:
>                             printMe = True
>                             if (test == 'tp') and self.fast: #only affects the tp test name
>                                 newline = newline.replace('tp', 'tp_fast')
>+                            if self.testPrefix:
>+                                newline = newline.replace(test, self.testPrefix + '_' + test)
>                 if self.noChrome: 
>                     #if noChrome is True remove --tpchrome option 
>                     newline = line.replace('-tpchrome ','')
>             if printMe:
>                 destination.write(newline)
>             if line.startswith('tests :'): 
>                 #enter into test writing mode
>                 testMode = True
>@@ -267,16 +275,20 @@ class PerfConfigurator:
>         if 'symbolsPath' in kwargs:
>             self.symbolsPath = kwargs['symbolsPath']
>         if 'remoteDevice' in kwargs:
>             self.remoteDevice = kwargs['remoteDevice']
>         if 'webServer' in kwargs:
>             self.webServer = kwargs['webServer']
>         if 'deviceRoot' in kwargs:
>             self.deviceRoot = kwargs['deviceRoot']
>+        if 'testPrefix' in kwargs:
>+            self.testPrefix = kwargs['testPrefix']
>+        if 'extension' in kwargs:
>+            self.extension = kwargs['extension']
>         if 'remotePort' in kwargs:
>             self.port = kwargs['remotePort']
> 
>         if (self.remoteDevice <> ''):
>           self._setupRemote(self.remoteDevice, self.port)
> 
>         self.currentDate = self._getCurrentDateString()
>         if not self.buildid:
>@@ -310,27 +322,29 @@ def main(argv=None):
>     activeTests = ''
>     noChrome = False
>     fast = False
>     symbolsPath = None
>     remoteDevice = ''
>     remotePort = ''
>     webServer = 'localhost'
>     deviceRoot = ''
>+    testPrefix = ''
>+    extension = ''
> 
>     if argv is None:
>         argv = sys.argv
>     try:
>         try:
>             opts, args = getopt.getopt(argv[1:], "hvue:c:t:b:o:i:d:s:l:a:n:r:p:w", 
>                 ["help", "verbose", "useId", "executablePath=", 
>                 "configFilePath=", "sampleConfig=", "title=", 
>                 "branch=", "output=", "id=", "testDate=", "browserWait=",
>                 "resultsServer=", "resultsLink=", "activeTests=", 
>-                "noChrome", "branchName=", "fast", "symbolsPath=",
>+                "noChrome", "testPrefix=", "extension=", "branchName=", "fast", "symbolsPath=",
>                 "remoteDevice=", "remotePort=", "webServer=", "deviceRoot="])
>         except getopt.error, msg:
>             raise Usage(msg)
>         
>         # option processing
>         for option, value in opts:
>             if option in ("-v", "--verbose"):
>                 verbose = True
>@@ -361,16 +375,20 @@ def main(argv=None):
>             if option in ("-s", "--resultsServer"):
>                 resultsServer = value
>             if option in ("-l", "--resultsLink"):
>                 resultsLink = value
>             if option in ("-a", "--activeTests"):
>                 activeTests = value
>             if option in ("-n", "--noChrome"):
>                 noChrome = True
>+            if option in ("--testPrefix",):
>+                testPrefix = value
>+            if option in ("--extension",):
>+                extension = value
>             if option in ("-r", "--remoteDevice"):
>                 remoteDevice = value
>             if option in ("-p", "--remotePort"):
>                 remotePort = value
>             if option in ("-w", "--webServer"):
>                 webServer = value
>             if option in ("--deviceRoot",):
>                 deviceRoot = value
>@@ -404,16 +422,18 @@ def main(argv=None):
>                                     browserWait=browserWait,
>                                     outputName=output,
>                                     useId=useId,
>                                     resultsServer=resultsServer,
>                                     resultsLink=resultsLink,
>                                     activeTests=activeTests,
>                                     noChrome=noChrome,
>                                     fast=fast,
>+                                    testPrefix=testPrefix,
>+                                    extension=extension,
>                                     symbolsPath=symbolsPath,
>                                     remoteDevice=remoteDevice,
>                                     remotePort=remotePort,
>                                     webServer=webServer,
>                                     deviceRoot=deviceRoot)
>     try:
>         configurator.writeConfigFile()
>     except Configuration, err:
>Index: ffsetup.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/ffsetup.py,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 ffsetup.py
>--- ffsetup.py	14 May 2010 13:36:40 -0000	1.18
>+++ ffsetup.py	19 May 2010 00:06:59 -0000
>@@ -46,16 +46,19 @@ __author__ = '[e-mail] (
> import platform
> import os
> import os.path
> import re
> import shutil
> import tempfile
> import time
> import glob
>+import zipfile
>+from xml.dom import minidom
>+import shutil
> 
> import utils
> from utils import talosError
> import subprocess
> 
> class FFSetup(object):
> 
>     ffprocess = None
>@@ -92,28 +95,72 @@ class FFSetup(object):
>         if type(value) == bool:
>             # Write bools as "true"/"false", not "True"/"False".
>             out_value = out_value.lower()
>         if type(value) == str:
>             # Write strings with quotes around them.
>             out_value = '"%s"' % value
>         return 'user_pref("%s", %s);%s' % (name, out_value, newline)
> 
>+    def install_addon(self, profile_path, addon):
>+        """Installs the given addon in the profile.
>+           most of this borrowed from mozrunner, except downgraded to work on python 2.4
>+           # Contributor(s) for mozrunner:
>+           # Mikeal Rogers <[e-mail]>
>+           # Clint Talbert <[e-mail]>
>+           # Henrik Skupin <[e-mail]>
>+        """

Don't need this line:
>+        tmpdir = None

>+        addon_id = ''
>+        tmpdir = tempfile.mkdtemp(suffix = "." + os.path.split(addon)[-1])
>+        compressed_file = zipfile.ZipFile(addon, "r")
>+        #in python2.6 can use extractall, currently limited to python2.4
>+        for name in compressed_file.namelist():
>+            if name.endswith('/'):
>+                os.makedirs(os.path.join(tmpdir, name))
>+            else:
>+                if not os.path.isdir(os.path.dirname(os.path.join(tmpdir, name))):
>+                    os.makedirs(os.path.dirname(os.path.join(tmpdir, name)))
>+                data = compressed_file.read(name)
>+                f = open(os.path.join(tmpdir, name), 'w')
>+                f.write(data) ; f.close()
>+        addon = tmpdir
>+
>+        doc = minidom.parse(os.path.join(addon, 'install.rdf')) 
>+        # description_element =
>+        # tree.find('.//{http://www.w3.org/1999/02/22-rdf-syntax-ns#}Description/')
>+ 
>+        desc = doc.getElementsByTagName('Description')
>+        for elem in desc:
>+            apps = elem.getElementsByTagName('em:targetApplication')
>+            if apps:
>+                for app in apps:
>+                    #remove targetApplication nodes, they contain id's we aren't interested in
>+                    elem.removeChild(app)
>+                addon_id = str(elem.getElementsByTagName('em:id')[0].firstChild.data)
>+        
>+        if not addon_id: #bail out, we don't have an addon id
>+            raise talosError("no addon_id found for extension " + addon)
>+                 
>+        addon_path = os.path.join(profile_path, 'extensions', addon_id)
>+        #if an old copy is already installed, remove it 
>+        if os.path.isdir(addon_path): 
>+            shutil.rmtree(addon_path, ignore_errors=True) 
>+        shutil.move(addon, addon_path) 
> 
>     def CreateTempProfileDir(self, source_profile, prefs, extensions):
>         """Creates a temporary profile directory from the source profile directory
>             and adds the given prefs and links to extensions.
> 
>         Args:
>             source_profile: String containing the absolute path of the source profile
>                             directory to copy from.
>             prefs: Preferences to set in the prefs.js file of the new profile.  Format:
>                     {"PrefName1" : "PrefValue1", "PrefName2" : "PrefValue2"}
>-            extensions: Guids and paths of extensions to link to.  Format:
>-                    {"{GUID1}" : "c:\\Path\\to\\ext1", "{GUID2}", "c:\\Path\\to\\ext2"}
>+            extensions: list of paths to .xpi files to be installed
> 
>         Returns:
>             String containing the absolute path of the profile directory.
>         """
> 
>         # Create a temporary directory for the profile, and copy the
>         # source profile to it.
>         temp_dir = tempfile.mkdtemp()
>@@ -131,20 +178,18 @@ class FFSetup(object):
> 
>         if (self._remoteWebServer <> 'localhost'):
>              self.ffprocess.addRemoteServerPref(profile_dir, self._remoteWebServer)
> 
>         # Add links to all the extensions.
>         extension_dir = os.path.join(profile_dir, "extensions")
>         if not os.path.exists(extension_dir):
>             os.makedirs(extension_dir)
>-        for extension in extensions:
>-            link_file = open(os.path.join(extension_dir, extension), 'w')
>-            link_file.write(extensions[extension])
>-            link_file.close()
>+        for addon in extensions:
>+            self.install_addon(profile_dir, addon)
> 
>         if (self._remoteWebServer <> 'localhost'):
>             remote_dir = self.ffprocess.copyDirToDevice(profile_dir)
>             profile_dir = remote_dir
>         return temp_dir, profile_dir
>         
>     def InstallInBrowser(self, browser_path, dir_path):
>         """

Looks good!

Comment 12

[alice nodelman [:alice] [:anode]]

Attachment: add support to buildbotcustom for downloading/configuring addons (take 3)

Small addition of --no-check-certificate for wget to pull down the addons - required on windows.  I really have to wait for green across the board before posting patches.

Comment 13

[alice nodelman [:alice] [:anode]]

Attachment: quick and dirty addons reporting for talos

Writes results to screen.  Lines starting with 'RETURN:' are picked up by the tinderbox waterfall.  You can see this in action on the MozillaTest waterfall, looks for the columns with 'addons' in the title.

Comment 14

[Jeff Hammel]

Comment on attachment 446377 [details] [diff] [review]
quick and dirty addons reporting for talos

>Index: run_tests.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/run_tests.py,v
>retrieving revision 1.61
>diff -u -8 -p -r1.61 run_tests.py
>--- run_tests.py	5 Mar 2010 16:02:48 -0000	1.61
>+++ run_tests.py	19 May 2010 22:58:39 -0000
>@@ -96,53 +96,77 @@ def process_Request(post):
>   if not links:
>     raise talosError("send failed, graph server says:\n" + post)
>   return links
> 
> def send_to_csv(csv_dir, results):
>   import csv 
>   for res in results:
>     browser_dump, counter_dump, print_format = results[res]
>-    writer = csv.writer(open(os.path.join(csv_dir, res + '.csv'), "wb"))
>+    if csv_dir:
>+      writer = csv.writer(open(os.path.join(csv_dir, res + '.csv'), "wb"))
>+    else: #working with stdout
>+      writer = csv.writer(sys.stdout)
I'd split up this logic:

if cvs_dir:
  f = open(os.path.join(csv_dir, res + '.csv'), "wb") 
else
  f = sys.stdout # working with stdout
writer = csv.writer(f)

>     if print_format == 'tsformat':
>       i = 0
>+      max = 0
should rename to not shadow the `max` builtin function (e.g. maximum=0)

>+      total = 0
>       writer.writerow(['i', 'val'])
>       for val in browser_dump:
>         val_list = val.split('|')
>         for v in val_list:
>+          if float(v) > max:
>+            max = float(v)
I'd probably use the max function here.  E.g. outside the inner loop
val_list = [float(v) for v in val.split('|')]
maximum = max(maximum, max(val_list))
total += sum(val_list)
for v in val_list:
  writer.writerow([i,v])

But this is mostly cosmetic

>           writer.writerow([i, v])
>+          total = total + float(v)
>           i += 1
>+      writer.writerow(['RETURN: ' + res + ': ' + str(round((total - max)/(i-1), 2)),])
That's a fair amount of logic for one line

>     elif print_format == 'tpformat':
>       writer.writerow(['i', 'page', 'median', 'mean', 'min' , 'max', 'runs'])
>       for bd in browser_dump:
>         bd.rstrip('\n')
Will the above work on windows with CRLF? 

>         page_results = bd.splitlines()
>         i = 0
>+        total = 0
>+        max = 0
>         for mypage in page_results:
>           r = mypage.split(';')
>           #skip this line if it isn't the correct format
>           if len(r) == 1:
>               continue
>           r[1] = r[1].rstrip('/')
>           if r[1].find('/') > -1 :
>              page = r[1].split('/')[1]
>           else:
>              page = r[1]
>+          if float(r[2]) > max:
>+             max = float(r[2])
>+          total = total + float(r[2])
>           writer.writerow([i, page, r[2], r[3], r[4], r[5], '|'.join(r[6:])])
>           i += 1
>+        writer.writerow(['RETURN: ' + res + ': ' + str(round((total - max)/(i-1), 2)), ])
>     else:
>       raise talosError("Unknown print format in send_to_csv")
>     for cd in counter_dump:
>       for count_type in cd:
>-        writer = csv.writer(open(os.path.join(csv_dir, res + '_' + count_type + '.csv'), "wb"))
>+        if csv_dir:
>+          writer = csv.writer(open(os.path.join(csv_dir, res + '_' + count_type + '.csv'), "wb"))
>+        else:
>+          writer = csv.writer(sys.stdout)
Again, I would do this as above

>         writer.writerow(['i', 'value'])
>         i = 0
>+        max = 0
>+        total = 0
>         for val in cd[count_type]:
>           writer.writerow([i, val])
>+          if float(val) > max: 
>+            max = float(val)
>+          total = total + float(val)
>           i += 1
>+        writer.writerow(['RETURN: ' + res + '_' + count_type + ': ' + str(round((total - max)/(i-1), 2)),])
> 
Not sure if the common logic can be meaningfully abstracted.  If the point is quick+dirty, maybe not be worth doing now, but probably worth thinking about later. [I would do it now, but that's how Iwork]

> def filesizeformat(bytes):
>     """
>     Format the value like a 'human-readable' file size (i.e. 13 KB, 4.1 MB, 102
>     bytes, etc).
>     """
>     bytes = float(bytes)
>     if bytes < 1024:
>@@ -301,17 +325,17 @@ def browserInfo(browser_config, devicema
>           browser_config['sourcestamp'] = match.group(1)
>   if ('repository' in browser_config) and ('sourcestamp' in browser_config):
>     print 'RETURN:<a href = "' + browser_config['repository'] + '/rev/' + browser_config['sourcestamp'] + '">rev:' + browser_config['sourcestamp'] + '</a>'
>   else:
>     browser_config['repository'] = 'NULL'
>     browser_config['sourcestamp'] = 'NULL'
>   return browser_config
> 
>-def test_file(filename):
>+def test_file(filename, to_screen):
>   """Runs the talos tests on the given config file and generates a report.
>   
>   Args:
>     filename: the name of the file to run the tests on

Should add the to_screen argument here.  If you don't change anything else, please document this argument!

>   """
>   
>   browser_config = []
>   tests = []
>@@ -422,16 +446,18 @@ def test_file(filename):
>     try:
>       mytest = TTest(browser_config['remote'])
>       browser_dump, counter_dump, print_format = mytest.runTest(browser_config, test)
>       utils.debug("Received test results: " + " ".join(browser_dump))
>       results[testname] = [browser_dump, counter_dump, print_format]
>       # If we're doing CSV, write this test immediately (bug 419367)
>       if csv_dir != '':
>         send_to_csv(csv_dir, {testname : results[testname]})
>+      if to_screen:
>+        send_to_csv('', {testname : results[testname]})
Should use None instead of ''

>     except talosError, e:
>       utils.stamped_msg("Failed " + testname, "Stopped")
>       print 'FAIL: Busted: ' + testname
>       print 'FAIL: ' + e.msg.replace('\n','\nRETURN:')
>     utils.stamped_msg("Completed test " + testname, "Stopped")
>   elapsed = utils.stopTimer()
>   print "RETURN: cycle time: " + elapsed + "<br>"
>   utils.stamped_msg(title, "Stopped")
>@@ -442,22 +468,26 @@ def test_file(filename):
>     try:
>       utils.stamped_msg("Sending results", "Started")
>       links = send_to_graph(results_server, results_link, title, date, browser_config, results)
>       results_from_graph(links, results_server)
>       utils.stamped_msg("Completed sending results", "Stopped")
>     except talosError, e:
>       utils.stamped_msg("Failed sending results", "Stopped")
>       print 'FAIL: ' + e.msg.replace('\n', '\nRETURN:')
>+
>   
> if __name__=='__main__':
>-  optlist, args = getopt.getopt(sys.argv[1:], 'dn', ['debug', 'noisy'])
>+  screen = False
>+  optlist, args = getopt.getopt(sys.argv[1:], 'dns', ['debug', 'noisy', 'screen'])
This entire part should be redone.  Probably not now but soon (since it continues to be editted):

 - there should be a main(args=sys.argv[1:]) function that `if __name__ == '__main__'` calls
 - getopt should be replaced with optparse.OptionParser.  So much of this code is unneeded!
It won't be hard, I promise!  I'd be glad to work with you to get you a patch.

>   for o, a in optlist:
>     if o in ('-d', "--debug"):
>       print 'setting debug'
>       utils.setdebug(1)
>     if o in ('-n', "--noisy"):
>       utils.setnoisy(1)
>+    if o in ('-s', "--screen"):
>+      screen = True
See? already more code ;)  I would also use the usual approach of `-o --output=/path/to/csv` with - being stdout, but non-blocking.

>   # Read in each config file and run the tests on it.
>   for arg in args:
>     utils.debug("running test file " + arg)
>-    test_file(arg)
>+    test_file(arg, screen)
>

Comment 15

[Chris AtLee [:catlee]]

Comment on attachment 446108 [details] [diff] [review]
add support to buildbotcustom for downloading/configuring addons (take 3)

>diff -r 6440cc540a94 process/factory.py
>--- a/process/factory.py	Mon May 17 21:26:10 2010 -0700
>+++ b/process/factory.py	Tue May 18 17:31:52 2010 -0700
>@@ -6150,7 +6150,7 @@
>     def __init__(self, OS, toolsDir, envName, buildBranch, branchName,
>             configOptions, talosCmd, customManifest=None, customTalos=None,
>             workdirBase=None, fetchSymbols=False, plugins=None, pageset=None,
>-            talosAddOns=[],
>+            talosAddOns=[], remoteExtensions=[],
>             cvsRoot=":pserver:[e-mail]:/cvsroot"):
> 
>         BuildFactory.__init__(self)
>@@ -6164,7 +6164,7 @@
>         self.toolsDir = toolsDir
>         self.buildBranch = buildBranch
>         self.branchName = branchName
>-        self.configOptions = configOptions
>+        self.configOptions = configOptions[:]
>         self.talosCmd = talosCmd
>         self.customManifest = customManifest
>         self.customTalos = customTalos
>@@ -6183,8 +6183,23 @@
>         if fetchSymbols and (self.OS not in ('snowleopard',)):
>             self.addDownloadSymbolsStep()
>         self.addSetupSteps()
>-        self.addUpdateConfigStep()
>-        self.addRunTestStep()
>+        if remoteExtensions:
>+            #format [['extension link', 'extension_name'],]
>+            for [ext, prefix] in remoteExtensions:
>+                self.addStep(SetBuildProperty(
>+                  property_name="extURL",
>+                  value=ext,
>+                ))
>+                self.addDownloadExtensionStep(ext

I don't think there's value here in using a build property.  You're passing the extension url to the download step here, so you can make use of it in addDownloadExtensionStep.

Would splitting this into a subclass make sense?

>+                cleanConfig = self.configOptions[:]
>+                self.configOptions.extend(['--testPrefix', prefix, '--extension', 'addon.xpi'])
>+                self.addUpdateConfigStep()
>+                self.configOptions = cleanConfig

Instead of doing this, change addUpdateConfigStep to accept a configOptions argument.

>+                self.addRunTestStep()
>+                self.addRemoveExtensionStep()
>+        else:
>+            self.addUpdateConfigStep()
>+            self.addRunTestStep()
>         self.addRebootStep()
> 
>     def addCleanupSteps(self):
>@@ -6451,6 +6466,26 @@
>          name="Unpack symbols",
>         ))
> 
>+    def addDownloadExtensionStep(self, ext):
>+        def get_extension_url(build):
>+          return build.getProperty('extURL')
>+        self.addStep(DownloadFile(
>+         url_fn=get_extension_url,
>+         filename_property="extensionFile",
>+         workdir=os.path.join(self.workdirBase, "talos"),
>+         name="Download extension",
>+         wget_args=['--no-check-certificate', '-O', 'addon.xpi'],
>+        ))
>+
>+    def addRemoveExtensionStep(self):
>+        self.addStep(ShellCommand(
>+         name='remove addon.xpi',
>+         workdir=self.workdirBase,
>+         description="Cleanup",
>+         command='nohup rm -vf addon.xpi',
>+         env=MozillaEnvironments[self.envName])
>+        )
>+
>     def addUpdateConfigStep(self):
>         self.addStep(talos_steps.MozillaUpdateConfig(
>          workdir=os.path.join(self.workdirBase, "talos/"),

Comment 16

[alice nodelman [:alice] [:anode]]

Attachment: add support to buildbotcustom for downloading/configuring addons (take 4)

Fixed up according to review comments.  

I don't believe that it should be split into a separate factory as that would involve more major changes to the buildbot-configs for talos - to put in checks there to special case and create the correct builder.  I think that this is a cleaner solution and easier to maintain.

Comment 17

[alice nodelman [:alice] [:anode]]

Attachment: quick and dirty addons reporting for talos (take 2)

Fixed an edge cases (results with a single data point).  Calculated results now match those reported by the graph server.

Comment 18

[Jeff Hammel]

Comment on attachment 447658 [details] [diff] [review]
quick and dirty addons reporting for talos (take 2)

>Index: run_tests.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/run_tests.py,v
>retrieving revision 1.62
>diff -u -8 -p -r1.62 run_tests.py
>--- run_tests.py	15 Apr 2010 18:00:16 -0000	1.62
>+++ run_tests.py	27 May 2010 00:13:16 -0000
>@@ -96,53 +96,89 @@ def process_Request(post):
>   if not links:
>     raise talosError("send failed, graph server says:\n" + post)
>   return links
> 
> def send_to_csv(csv_dir, results):
>   import csv
>   for res in results:
>     browser_dump, counter_dump, print_format = results[res]
>-    writer = csv.writer(open(os.path.join(csv_dir, res + '.csv'), "wb"))
>+    if csv_dir:
>+      writer = csv.writer(open(os.path.join(csv_dir, res + '.csv'), "wb"))
>+    else: #working with stdout
>+      writer = csv.writer(sys.stdout)
>     if print_format == 'tsformat':
>       i = 0
>+      max = 0
I would avoid overriding the max builtin, e.g. calling this _max or maximum

>+      total = 0
>       writer.writerow(['i', 'val'])
>       for val in browser_dump:
>         val_list = val.split('|')
>         for v in val_list:
>+          if float(v) > max:
>+            max = float(v)
Are you avoiding the max function (not your variable here) to save on iteration? That is, why not
maximum = [float(v) for v in val_list] (though, see below).

>           writer.writerow([i, v])
>+          total = total + float(v)

Again, why not let python do this for you?  Afaik, you only really need the loop for the writer.writerow.  See below for sample

>           i += 1
>+      if total > max:
>+        avg = str(round((total - max)/(i-1), 2))
>+      else:
>+        avg = str(round(total, 2))

To me, this whole loop reads much cleaner as

val_list = val.split('|')
for v in val_list:
  writer.writerow([i, v])
val_list = [float(v) for v in val_list]
maximum = max(maximum, max(val_list))
total += sum(val_list)
i += len(val_list)

>+      writer.writerow(['RETURN: ' + res + ': ' + avg,])
>     elif print_format == 'tpformat':
>       writer.writerow(['i', 'page', 'median', 'mean', 'min' , 'max', 'runs'])
>       for bd in browser_dump:
>         bd.rstrip('\n')
>         page_results = bd.splitlines()
>         i = 0
>+        total = 0
>+        max = 0
>         for mypage in page_results:
>           r = mypage.split(';')
>           #skip this line if it isn't the correct format
>           if len(r) == 1:
>               continue
>           r[1] = r[1].rstrip('/')
>           if r[1].find('/') > -1 :
>              page = r[1].split('/')[1]
>           else:
>              page = r[1]
>+          if float(r[2]) > max:
>+             max = float(r[2])
>+          total = total + float(r[2])
>           writer.writerow([i, page, r[2], r[3], r[4], r[5], '|'.join(r[6:])])
>           i += 1
>+        if total > max:
>+          avg = str(round((total - max)/(i-1), 2))
>+        else:
>+          avg = str(round(total, 2))
>+        writer.writerow(['RETURN: ' + res + ': ' + avg, ])

Again, same sort of thing here.  I'd probably also abstract the `if total > max:` subroutine that you repeat to a function (in fact, i'd probably abstract more than that...repeated code is no fun).

>     else:
>       raise talosError("Unknown print format in send_to_csv")
>     for cd in counter_dump:
>       for count_type in cd:
>-        writer = csv.writer(open(os.path.join(csv_dir, res + '_' + count_type + '.csv'), "wb"))
>+        if csv_dir:
>+          writer = csv.writer(open(os.path.join(csv_dir, res + '_' + count_type + '.csv'), "wb"))
>+        else:
>+          writer = csv.writer(sys.stdout)
>         writer.writerow(['i', 'value'])
>         i = 0
>+        max = 0
>+        total = 0
>         for val in cd[count_type]:
>           writer.writerow([i, val])
>+          if float(val) > max: 
>+            max = float(val)
>+          total = total + float(val)
>           i += 1
>+        if total > max:
>+          avg = str(round((total - max)/(i-1), 2))
>+        else:
>+          avg = str(round(total, 2))
See above ;)

>+        writer.writerow(['RETURN: ' + res + '_' + count_type + ': ' + avg,])
> 
> def filesizeformat(bytes):
>     """
>     Format the value like a 'human-readable' file size (i.e. 13 KB, 4.1 MB, 102
>     bytes, etc).
>     """
>     bytes = float(bytes)
>     if bytes < 1024:
>@@ -301,17 +337,17 @@ def browserInfo(browser_config, devicema
>           browser_config['sourcestamp'] = match.group(1)
>   if ('repository' in browser_config) and ('sourcestamp' in browser_config):
>     print 'RETURN:<a href = "' + browser_config['repository'] + '/rev/' + browser_config['sourcestamp'] + '">rev:' + browser_config['sourcestamp'] + '</a>'
>   else:
>     browser_config['repository'] = 'NULL'
>     browser_config['sourcestamp'] = 'NULL'
>   return browser_config
> 
>-def test_file(filename):
>+def test_file(filename, to_screen):
>   """Runs the talos tests on the given config file and generates a report.
>   
>   Args:
>     filename: the name of the file to run the tests on
>   """
Please document `to_screen` in the docstring.  This is actually my biggest complaint.

>   browser_config = []
>   tests = []
>@@ -422,16 +458,18 @@ def test_file(filename):
>     try:
>       mytest = TTest(browser_config['remote'])
>       browser_dump, counter_dump, print_format = mytest.runTest(browser_config, test)
>       utils.debug("Received test results: " + " ".join(browser_dump))
>       results[testname] = [browser_dump, counter_dump, print_format]
>       # If we're doing CSV, write this test immediately (bug 419367)
>       if csv_dir != '':
>         send_to_csv(csv_dir, {testname : results[testname]})
>+      if to_screen:
>+        send_to_csv('', {testname : results[testname]})
Use None instead of the empty string, please, since you only check for boolean truth.

>     except talosError, e:
>       utils.stamped_msg("Failed " + testname, "Stopped")
>       print 'FAIL: Busted: ' + testname
>       print 'FAIL: ' + e.msg.replace('\n','\nRETURN:')
>     utils.stamped_msg("Completed test " + testname, "Stopped")
>   elapsed = utils.stopTimer()
>   print "RETURN: cycle time: " + elapsed + "<br>"
>   utils.stamped_msg(title, "Stopped")
>@@ -442,22 +480,26 @@ def test_file(filename):
>     try:
>       utils.stamped_msg("Sending results", "Started")
>       links = send_to_graph(results_server, results_link, title, date, browser_config, results)
>       results_from_graph(links, results_server)
>       utils.stamped_msg("Completed sending results", "Stopped")
>     except talosError, e:
>       utils.stamped_msg("Failed sending results", "Stopped")
>       print 'FAIL: ' + e.msg.replace('\n', '\nRETURN:')
>+
>   
> if __name__=='__main__':
>-  optlist, args = getopt.getopt(sys.argv[1:], 'dn', ['debug', 'noisy'])
>+  screen = False
>+  optlist, args = getopt.getopt(sys.argv[1:], 'dns', ['debug', 'noisy', 'screen'])
>   for o, a in optlist:
>     if o in ('-d', "--debug"):
>       print 'setting debug'
>       utils.setdebug(1)
>     if o in ('-n', "--noisy"):
>       utils.setnoisy(1)
>+    if o in ('-s', "--screen"):
>+      screen = True

Oy!  I'll ignore the maintainence nightmare that is getopt vs optparse for now.  But if you start adding many more options, it might be worth to refactor to use a better parser.

>   # Read in each config file and run the tests on it.
>   for arg in args:
>     utils.debug("running test file " + arg)
>-    test_file(arg)
>+    test_file(arg, screen)
> 

Looks fine.  I'd appreciate it if you implement some of those changes.

Comment 19

[Chris AtLee [:catlee]]

Comment on attachment 446754 [details] [diff] [review]
add support to buildbotcustom for downloading/configuring addons (take 4)

Still need an review on this, or are you going to look at using properties?

Comment 20

[alice nodelman [:alice] [:anode]]

Comment on attachment 446754 [details] [diff] [review]
add support to buildbotcustom for downloading/configuring addons (take 4)

Removing review until I get it working so that it generates 100 jobs for 100 addons, instead of 1 job testing 100 addons.  This will help balance the load on the talos testing pool of slaves.

Comment 21

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]quick and dirty addons reporting for talos (take 3)

Final patch based upon jhammel's review comments.

Comment 22

[alice nodelman [:alice] [:anode]]

Comment on attachment 449155 [details] [diff] [review]
[checked in]quick and dirty addons reporting for talos (take 3)

Carrying over r+ from jhammel.

Comment 23

[alice nodelman [:alice] [:anode]]

Comment on attachment 449155 [details] [diff] [review]
[checked in]quick and dirty addons reporting for talos (take 3)

Checking in run_tests.py;
/cvsroot/mozilla/testing/performance/talos/run_tests.py,v  <--  run_tests.py
new revision: 1.63; previous revision: 1.62
done

Comment 24

[alice nodelman [:alice] [:anode]]

Comment on attachment 446099 [details] [diff] [review]
[checked in]add support to install .xpi addons to talos (take 2)

Checking in PerfConfigurator.py;
/cvsroot/mozilla/testing/performance/talos/PerfConfigurator.py,v  <--  PerfConfigurator.py
new revision: 1.19; previous revision: 1.18
done
Checking in ffsetup.py;
/cvsroot/mozilla/testing/performance/talos/ffsetup.py,v  <--  ffsetup.py
new revision: 1.19; previous revision: 1.18
done

Comment 25

[alice nodelman [:alice] [:anode]]

Attachment: add support to buildbotcustom for downloading/configuring addons (take 5)

Proposed method to install/test addons based upon build properties that should be set in the scheduler.

Comment 26

[Chris AtLee [:catlee]]

Comment on attachment 450196 [details] [diff] [review]
add support to buildbotcustom for downloading/configuring addons (take 5)

>diff -r 3c75ccfd152f process/factory.py
>--- a/process/factory.py	Mon Jun 07 14:57:33 2010 -0700
>+++ b/process/factory.py	Wed Jun 09 13:08:44 2010 -0700
>@@ -6185,7 +6185,7 @@
>         self.supportUrlBase = supportUrlBase
>         self.buildBranch = buildBranch
>         self.branchName = branchName
>-        self.configOptions = configOptions
>+        self.configOptions = configOptions[:]
>         self.talosCmd = talosCmd
>         self.customManifest = customManifest
>         self.customTalos = customTalos
>@@ -6206,6 +6206,7 @@
>         self.addSetupSteps()
>         self.addUpdateConfigStep()
>         self.addRunTestStep()
>+        self.addRunAddonTestStep()
>         self.addRebootStep()
> 
>     def addCleanupSteps(self):
>@@ -6416,6 +6417,7 @@
>              mastersrc=self.customTalos,
>              slavedest=self.customTalos,
>              workdir=self.workdirBase,
>+             blocksize=640*1024,
>             ))
>             self.addStep(UnpackFile(
>              filename=self.customTalos,
>@@ -6478,14 +6480,35 @@
>          name="Unpack symbols",
>         ))
> 
>-    def addUpdateConfigStep(self):
>+    def addDownloadExtensionStep(self, ext):
>+        def get_extension_url(build):
>+          return ext
>+        self.addStep(DownloadFile(
>+         url_fn=get_extension_url,
>+         filename_property="extensionFile",
>+         workdir=os.path.join(self.workdirBase, "talos"),
>+         name="Download extension",
>+         ignore_certs=True, 
>+         wget_args=['-O', 'addon.xpi'],
>+        ))

DownloadFile now supports an url parameter, you don't need the dummy get_extension_url function any more.

>+
>+    def addRemoveExtensionStep(self):
>+        self.addStep(ShellCommand(
>+         name='remove addon.xpi',
>+         workdir=self.workdirBase,
>+         description="Cleanup",
>+         command='nohup rm -vf addon.xpi',
>+         env=MozillaEnvironments[self.envName])
>+        )

Can you store MozillaEnvironments[self.envName] as self.env and use that here and below?

>+    def addUpdateConfigStep(self, extraOpts=[]):
>         self.addStep(talos_steps.MozillaUpdateConfig(
>          workdir=os.path.join(self.workdirBase, "talos/"),
>          branch=self.buildBranch,
>          branchName=self.branchName,
>          haltOnFailure=True,
>          executablePath=self.exepath,
>-         addOptions=self.configOptions,
>+         addOptions=self.configOptions + extraOpts,
>          env=MozillaEnvironments[self.envName],
>          useSymbols=self.fetchSymbols)
>         )
>@@ -6500,6 +6523,18 @@
>          env=MozillaEnvironments[self.envName])
>         )
> 
>+    def addRunAddonTestStep(self):
>+        try:
>+            addons = self.build.getProperty('addons')
>+            #format [['extension link', 'extension_name'],]
>+            for [ext, prefix] in addons:

nit: [ext, prefix] looks weird, I'd use (ext, prefix) instead

>+                self.addDownloadExtensionStep(ext)
>+                self.addUpdateConfigStep(['--testPrefix', prefix, '--extension', 'addon.xpi'])
>+                self.addRunTestStep()
>+                self.addRemoveExtensionStep()
>+         except KeyError: #no addons to test for this build
>+             continue

I think 'continue' here will generate an error, you probably want 'pass'

>+
>     def addRebootStep(self):
>         def do_disconnect(cmd):
>             try:

Comment 27

[alice nodelman [:alice] [:anode]]

Attachment: add support to buildbotcustom for downloading/configuring addons (take 6)

Takes into account previous comments.  Generally fixed how env is stored for the whole talos factory.

Comment 28

[alice nodelman [:alice] [:anode]]

For clarity, there is the current scope of the project:

1.  testing against one branch only (Firefox3.6 nightlies)
2.  testing once a week (currently planning for Saturday morning)
3.  top 100 addons (static list provided by addons team, not based upon
changes/checkins to addons)
4.  not reporting results to graph server, no automated regression tracking
5.  results to be reported to waterfall using new talos --screen option
6.  ts + tp4 only

Still tbd:
- can we limit what platforms that we test on to reduce load?

Comment 29

[Chris AtLee [:catlee]]

Comment on attachment 450213 [details] [diff] [review]
add support to buildbotcustom for downloading/configuring addons (take 6)

>+    def addRunAddonTestStep(self):
>+        try:
>+            addons = self.build.getProperty('addons')
>+            #format [['extension link', 'extension_name'],]
>+            for (ext, prefix) in addons:
>+                self.addDownloadExtensionStep(ext)
>+                self.addUpdateConfigStep(['--testPrefix', prefix, '--extension', 'addon.xpi'])
>+                self.addRunTestStep()
>+                self.addRemoveExtensionStep()
>+         except KeyError: #no addons to test for this build
>+             pass

Sorry, I missed this in the previous review.  This code won't work as it is now; the factory is instantiated when buildbot starts or is reconfigured, and so there is no self.build to access.  Having buildbot generate steps dynamically based on properties isn't well supported.

I'd recommend writing a helper script that you can pass in the 'addons' property to using WithProperties, which would then take care of downloading and installing the addons and then running talos.

Also, do you want to be running the test once without any addons installed, and then once with each extension?  If we're testing 100 addons, that's 200 test runs.

Comment 30

[alice nodelman [:alice] [:anode]]

Attachment: add support to buildbotcustom for downloading/configuring addons (take 7)

Uses build properties, cleans up how we handle the env var, adds some functions to steps/talos.py for downloading extensions (which may or may not be present, so shouldn't flunk if there is nothing to download) and installing the extension in the sample.config.

Comment 31

[Chris AtLee [:catlee]]

Comment on attachment 450722 [details] [diff] [review]
add support to buildbotcustom for downloading/configuring addons (take 7)

>diff -r 4dbf9815b1a7 steps/talos.py
>--- a/steps/talos.py	Thu Jun 10 10:47:02 2010 -0400
>+++ b/steps/talos.py	Fri Jun 11 12:36:44 2010 -0700
>@@ -7,7 +7,7 @@
>     name = "Update config"
> 
>     def __init__(self, branch, branchName, executablePath, addOptions=None,
>-            useSymbols=False, **kwargs):
>+            useSymbols=False, extName='', **kwargs):

This should default to None, and...

>@@ -29,6 +31,12 @@
>         ShellCommand.setBuild(self, build)
>         title = build.slavename
>         buildid = time.strftime("%Y%m%d%H%M", time.localtime(build.source.changes[-1].when))
>+        try:
>+            addon = self.build.getProperty('addon')
>+            ext, prefix = addon
>+            self.addOptions += ['--testPrefix', prefix, '--extension', self.extName]
>+        except:
>+            pass

...you should look for the 'addon' property if self.extName is not None.  Does this work with the rest of your logic?

Also, here you should be catching your particular exception (KeyError I
suppose), and logging something to indicate that it wasn't found as expected.

> 
>         extraopts = copy.copy(self.addOptions)
>         if self.useSymbols:
>@@ -79,3 +87,40 @@
>         if None != re.search('FAIL:', stdioText):
>             return WARNINGS
>         return SUCCESS
>+
>+class DownloadExtension(ShellCommand):
>+    name = "maybe download extension"
>+
>+    def __init__(self, ignore_certs=False, wget_args=[], **kwargs):
>+        self.ignore_certs = ignore_certs
>+        self.wget_args=wget_args[:]
>+        self.ext=None
>+        ShellCommand.__init__(self, **kwargs)
>+        self.addFactoryArguments(ignore_certs=ignore_certs, wget_args=wget_args)
>+
>+    def setBuild(self, build):
>+        ShellCommand.setBuild(self, build)
>+        try:
>+            addon = build.getProperty('addon')
>+            self.ext, prefix = addon
>+        except KeyError:
>+            pass

This logic can be moved into start() I think, no need to override setBuild.  self.build is available in start().

>+
>+    def start(self):
>+        if self.ext:
>+            if self.ignore_certs:
>+                self.setCommand(["wget"] + self.wget_args + ["-N", "--no-check-certificate", "--progress=dot:mega", self.ext])
>+            else:
>+                self.setCommand(["wget"] + self.wget_args + ["-N", "--progress=dot:mega", self.ext])
>+        else:
>+            self.setCommand(["echo", "no addon to download"])
>+        ShellCommand.start(self)
>+
>+    def evaluateCommand(self, cmd):
>+        superResult = ShellCommand.evaluateCommand(self, cmd)
>+        if SUCCESS != superResult:
>+            return FAILURE
>+        if None != re.search('ERROR', cmd.logs['stdio'].getText()):
>+            return FAILURE
>+        return SUCCESS
>+

You'll need to do the 'self.super_class = ShellCommand' trick that we use elsewhere and use self.super_class instead of ShellCommand so that reconfigs don't break.  See steps/misc.py, EvaluatingShellCommand as an example.

Comment 32

[alice nodelman [:alice] [:anode]]

There is always a default value for 'ext' as I won't know till runtime if the buildbot propery 'addon' has been set - so the factory itself has to have the default download name built in in case we do have an addon to install.

Comment 33

[Chris AtLee [:catlee]]

(In reply to comment #32)
> There is always a default value for 'ext' as I won't know till runtime if the
> buildbot propery 'addon' has been set - so the factory itself has to have the
> default download name built in in case we do have an addon to install.

So you're always going to be passing in the extName to the factory?  If so, extName shouldn't have any default in TalosFactory.

Can you show me how the buildbot-configs will look in this case?

Comment 34

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]add support to buildbotcustom for downloading/configuring addons (take 8)

Took into account feedback comments, also checked with catlee on irc to see if I'm (finally) on the right track.

Comment 35

[Chris AtLee [:catlee]]

Comment on attachment 451712 [details] [diff] [review]
[checked in]add support to buildbotcustom for downloading/configuring addons (take 8)

This looks good!  Just a new style nits:

>@@ -6247,6 +6247,8 @@
>         self.pageset = pageset
>         self.talosAddOns = talosAddOns[:]
>         self.exepath = None
>+        self.env=MozillaEnvironments[envName]
>+        self.addonTester=addonTester

please keep the spaces around '=' to keep these lines consistent with the rest.  Generally the rule is to use spaces around '=' for variable assignments like this, and no spaces when passing keyword arguments to functions.

>         self.addCleanupSteps()
>         self.addDmgInstaller()
>@@ -6255,6 +6257,8 @@
>         self.addGetBuildInfoStep()
>         if fetchSymbols and (self.OS not in ('snowleopard',)):
>             self.addDownloadSymbolsStep()
>+        if self.addonTester:
>+          self.addDownloadExtensionStep()

Looks like different indentation levels here?

>@@ -6530,6 +6535,20 @@
>          name="Unpack symbols",
>         ))
> 
>+    def addDownloadExtensionStep(self):
>+        def get_addon_url(build):
>+            addon = build.getProperty('addon')
>+            ext, prefix = addon
>+            return ext
>+
>+        self.addStep(DownloadFile(
>+         url_fn=get_addon_url,
>+         workdir=os.path.join(self.workdirBase, "talos"),
>+         name="Download extension",
>+         ignore_certs=True, 
>+         wget_args=['-O', TalosFactory.extName],
>+        ))
>+

self.extName should work here instead of TalosFactory.extName

>@@ -18,17 +18,26 @@
>         self.branchName = branchName
>         self.exePath = executablePath
>         self.useSymbols = useSymbols
>+        self.extName = extName
>+        self.addonTester=addonTester

same comment about '='

Comment 36

[alice nodelman [:alice] [:anode]]

Comment on attachment 451712 [details] [diff] [review]
[checked in]add support to buildbotcustom for downloading/configuring addons (take 8)

changeset:   774:9fb8200c4cb7

Comment 37

[alice nodelman [:alice] [:anode]]

Attachment: minor changes to allow system to work with sendchange

buildbot sendchange wasn't very fond of full urls in build properties (as it does a split over ':'), splitting things up into two properties (addonName, addonUrl) and then removing the repeated http address chunk allows everything to work from the command line.

Comment 38

[alice nodelman [:alice] [:anode]]

Attachment: manually trigger addons tests with sendchanges

This is a bash script + data file to manually run addons tests.  This assumes that there is a dummy 'addontester' branch set up.

To use:
> ./triggeraddons.sh addons.txt

Comment 39

[alice nodelman [:alice] [:anode]]

Attachment: set up dummy addontester branch

With the dummy branch + the bash script to generate sendchanges we can trigger tests for the 100 top addons anytime we want.

Win!

I do have this running in staging and it looks good.

Comment 40

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]minor changes to allow system to work with sendchange (take 2)

Comment 41

[alice nodelman [:alice] [:anode]]

Comment on attachment 453127 [details] [diff] [review]
[checked in]minor changes to allow system to work with sendchange (take 2)

changeset:   786:9f75dcb5082f

Comment 42

[alice nodelman [:alice] [:anode]]

Attachment: expanded support for discovering em:id in addon install.rdf

Some addons use a slightly different install.rdf format that we don't currently handle.  This let's us discover the correct em:id from these install.rdf's so that we can install the addon to the profile.

Comment 43

[alice nodelman [:alice] [:anode]]

Attachment: expanded support for discovering em:id in addon install.rdf (take 2)

Typos in previous patch.

Comment 44

[alice nodelman [:alice] [:anode]]

Attachment: set up dummy addontester branch (take 2)

Previous patch had typos.

Also updated to latest code (includes addition of mozilla-2.0 branch).

Comment 45

[Jeff Hammel]

Comment on attachment 453549 [details] [diff] [review]
expanded support for discovering em:id in addon install.rdf (take 2)

looks fine

Comment 46

[alice nodelman [:alice] [:anode]]

Attachment: manually trigger addons tests with sendchanges (take 2)

Minor fixes to sendchange shell script.

Comment 47

[[e-mail] (:bhearsum)]

Comment on attachment 453554 [details] [diff] [review]
set up dummy addontester branch (take 2)

I realize this has been reviewed already, but why is addonstester a separate branch? Why not use additional builders on the same branch?

Comment 48

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]set up dummy addontester branch (take 3)

Fixing missed configuration settings, carrying over review from catlee.

Comment 49

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]expanded support for discovering em:id in addon install.rdf (take 3)

More edge cases!

Comment 50

[Jeff Hammel]

Comment on attachment 453908 [details] [diff] [review]
[checked in]expanded support for discovering em:id in addon install.rdf (take 3)

>Index: ffsetup.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/ffsetup.py,v
>retrieving revision 1.19
>diff -u -8 -p -r1.19 ffsetup.py
>--- ffsetup.py	7 Jun 2010 21:48:35 -0000	1.19
>+++ ffsetup.py	24 Jun 2010 22:44:10 -0000
>@@ -103,16 +103,30 @@ class FFSetup(object):
>     def install_addon(self, profile_path, addon):
>         """Installs the given addon in the profile.
>            most of this borrowed from mozrunner, except downgraded to work on python 2.4
>            # Contributor(s) for mozrunner:
>            # Mikeal Rogers <[e-mail]>
>            # Clint Talbert <[e-mail]>
>            # Henrik Skupin <[e-mail]>
>         """
>+        def find_id(desc):
>+            addon_id = ''
>+            for elem in desc:
>+                apps = elem.getElementsByTagName('em:targetApplication')
>+                if apps:
>+                    for app in apps:
>+                        #remove targetApplication nodes, they contain id's we aren't interested in
>+                        elem.removeChild(app)
>+                    if elem.getElementsByTagName('em:id'):
>+                        addon_id = str(elem.getElementsByTagName('em:id')[0].firstChild.data)
>+                    elif elem.hasAttribute('em:id'):
>+                        addon_id = str(elem.getAttribute('em:id'))
>+            return addon_id
>+
>         tmpdir = None
>         addon_id = ''

If you set this inside the find_id, you don't need to set it here.  Also, should be None and not empty string

>         tmpdir = tempfile.mkdtemp(suffix = "." + os.path.split(addon)[-1])
>         compressed_file = zipfile.ZipFile(addon, "r")
>         #in python2.6 can use extractall, currently limited to python2.4
>         for name in compressed_file.namelist():
>             if name.endswith('/'):
>                 os.makedirs(os.path.join(tmpdir, name))
>@@ -122,25 +136,22 @@ class FFSetup(object):
>                 data = compressed_file.read(name)
>                 f = open(os.path.join(tmpdir, name), 'w')
>                 f.write(data) ; f.close()
>         addon = tmpdir
> 
>         doc = minidom.parse(os.path.join(addon, 'install.rdf')) 
>         # description_element =
>         # tree.find('.//{http://www.w3.org/1999/02/22-rdf-syntax-ns#}Description/')
>- 
>+
>         desc = doc.getElementsByTagName('Description')
>-        for elem in desc:
>-            apps = elem.getElementsByTagName('em:targetApplication')
>-            if apps:
>-                for app in apps:
>-                    #remove targetApplication nodes, they contain id's we aren't interested in
>-                    elem.removeChild(app)
>-                addon_id = str(elem.getElementsByTagName('em:id')[0].firstChild.data)
>+        addon_id = find_id(desc)
>+        if not addon_id:
>+          desc = doc.getElementsByTagName('RDF:Description')
>+          addon_id = find_id(desc)
>         
>         if not addon_id: #bail out, we don't have an addon id
>             raise talosError("no addon_id found for extension")
>                  
>         addon_path = os.path.join(profile_path, 'extensions', addon_id)
>         #if an old copy is already installed, remove it 
>         if os.path.isdir(addon_path): 
>             shutil.rmtree(addon_path, ignore_errors=True) 

r+ with those changes

Comment 51

[alice nodelman [:alice] [:anode]]

Currently blocked on scheduling a downtime - once the code gets rolled out the addons results will appear on the waterfall.

Comment 52

[alice nodelman [:alice] [:anode]]

Comment on attachment 453907 [details] [diff] [review]
[checked in]set up dummy addontester branch (take 3)

changeset:   2634:803bdbd45295

Comment 53

[alice nodelman [:alice] [:anode]]

Comment on attachment 453908 [details] [diff] [review]
[checked in]expanded support for discovering em:id in addon install.rdf (take 3)

/cvsroot/mozilla/testing/performance/talos/ffsetup.py,v  <--  ffsetup.py
new revision: 1.20; previous revision: 1.19
done

Comment 54

[alice nodelman [:alice] [:anode]]

All code checked in and pushed to talos masters.

Will trigger the initial addon test runs manually before fixing bug 572486.

Comment 55

[alice nodelman [:alice] [:anode]]

First results (for top 5 addons only) now showing up at:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox3.6

Comment 56

[alice nodelman [:alice] [:anode]]

winxp results:

s: talos-r3-xp-016
id:20100701042414
rev:69bc40a0e08c
Personas_Plus_ts: 362.21
Personas_Plus_ts_shutdown: 287.37
Personas_Plus_tp4: 310.81
Personas_Plus_tp4_% Processor Time: 57.83
Personas_Plus_tp4_Private Bytes: 83404023.17
Personas_Plus_tp4_Working Set: 88343799.17
Personas_Plus_tp4_shutdown: 1924.0
cycle time: 00:16:39

s: talos-r3-xp-004
id:20100701042414
rev:69bc40a0e08c
Adblock_Plus_ts: 362.95
Adblock_Plus_ts_shutdown: 279.68
Adblock_Plus_tp4: 310.02
Adblock_Plus_tp4_% Processor Time: 58.59
Adblock_Plus_tp4_Private Bytes: 79999117.24
Adblock_Plus_tp4_Working Set: 84146529.1
Adblock_Plus_tp4_shutdown: 1770.0
cycle time: 00:16:35

s: talos-r3-xp-014
id:20100701042414
rev:69bc40a0e08c
Video_DownloadHelper_ts: 374.89
Video_DownloadHelper_ts_shutdown: 280.47
Video_DownloadHelper_tp4: 312.98
Video_DownloadHelper_tp4_% Processor Time: 58.41
Video_DownloadHelper_tp4_Private Bytes: 82747250.76
Video_DownloadHelper_tp4_Working Set: 87166411.03
Video_DownloadHelper_tp4_shutdown: 1243.0
cycle time: 00:16:56

s: talos-r3-xp-013
id:20100701042414
rev:69bc40a0e08c
NoScript_ts: 365.84
NoScript_ts_shutdown: 290.0
NoScript_tp4: 309.99
NoScript_tp4_% Processor Time: 58.38
NoScript_tp4_Private Bytes: 81412378.48
NoScript_tp4_Working Set: 85630552.28
NoScript_tp4_shutdown: 1627.0
cycle time: 00:16:33

s: talos-r3-xp-034
id:20100701042414
rev:69bc40a0e08c
FlashGot_ts: 429.68
FlashGot_ts_shutdown: 256.89
FlashGot_tp4: 312.41
FlashGot_tp4_% Processor Time: 58.55
FlashGot_tp4_Private Bytes: 82773662.9
FlashGot_tp4_Working Set: 86674326.07
FlashGot_tp4_shutdown: 1796.0
cycle time: 00:16:32

Comment 57

[alice nodelman [:alice] [:anode]]

Attachment: cleaner tinderbox summaries for addons tests

Addons tests are currently a bit of a mess on the waterfall because we are using long names (Private Bytes) instead of compressed names (pbytes).  This will just bring it in line with the output for standard tests.

Comment 58

[Jeff Hammel]

Comment on attachment 457596 [details] [diff] [review]
cleaner tinderbox summaries for addons tests

>Index: run_tests.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/run_tests.py,v
>retrieving revision 1.64
>diff -u -8 -p -r1.64 run_tests.py
>--- run_tests.py	7 Jun 2010 21:55:24 -0000	1.64
>+++ run_tests.py	15 Jul 2010 17:31:25 -0000
>@@ -65,16 +65,34 @@ def shortName(name):
>     return "rss"
>   elif name == "XRes":
>     return "xres"
>   elif name == "Modified Page List Bytes":
>     return "modlistbytes"
>   else: 
>     return name
> 
>+def isMemoryMetric(resultName):
>+  memory_metric = ['memset', 'rss', 'pbytes', 'xres', 'modlistbytes']
>+  return filter(lambda x: x in resultName, memory_metric) #measured in bytes

Maybe better with a list comprehension ?:

return bool([ i for i in memory_metric if i in resultName])

>+def filesizeformat(bytes):
>+    """
>+    Format the value like a 'human-readable' file size (i.e. 13 KB, 4.1 MB, 102
>+    bytes, etc).
>+    """
>+    bytes = float(bytes)
>+    if bytes < 1024:
>+        return "%dB" % (bytes)
>+    if bytes < 1024 * 1024:
>+        return "%.1fKB" % (bytes / 1024)
>+    if bytes < 1024 * 1024 * 1024:
>+        return "%.1fMB" % (bytes / (1024 * 1024))
>+    return "%.1fGB" % (bytes / (1024 * 1024 * 1024))
>+

Maybe this is nicer ?:

def filesizeformat(bytes):
  bytes = float(bytes)
  formats = ('B', 'KB', 'MB')
  for f in formats:
    if bytes < 1024:
      return "%d%s" % (bytes, f)
    bytes /= 1024
  return "%dGB" % bytes


> def process_tpformat(line):
>   # each line of the string is of the format i;page_name;median;mean;min;max;time vals\n
>   r = line.split(';')
>   #skip this line if it isn't the correct format
>   if len(r) == 1:
>       return -1, ''
>   r[1] = r[1].rstrip('/')
>   if r[1].find('/') > -1 :
>@@ -150,40 +168,30 @@ def send_to_csv(csv_dir, results):
>           res_list.append(r[2])
>           writer.writerow([i, page, r[2], r[3], r[4], r[5], '|'.join(r[6:])])
>           i += 1
>         writer.writerow(['RETURN: ' + res + ': ' + avg_excluding_max(res_list), ])
>     else:
>       raise talosError("Unknown print format in send_to_csv")
>     for cd in counter_dump:
>       for count_type in cd:
>+        counterName = res + '_' + shortName(count_type)
>         if csv_dir:
>-          writer = csv.writer(open(os.path.join(csv_dir, res + '_' + count_type + '.csv'), "wb"))
>+          writer = csv.writer(open(os.path.join(csv_dir, counterName + '.csv'), "wb"))
>         else:
>           writer = csv.writer(sys.stdout)
>         writer.writerow(['i', 'value'])
>         i = 0
>         for val in cd[count_type]:
>           writer.writerow([i, val])
>           i += 1
>-        writer.writerow(['RETURN: ' + res + '_' + count_type + ': ' + avg_excluding_max(cd[count_type]),])
>-
>-def filesizeformat(bytes):
>-    """
>-    Format the value like a 'human-readable' file size (i.e. 13 KB, 4.1 MB, 102
>-    bytes, etc).
>-    """
>-    bytes = float(bytes)
>-    if bytes < 1024:
>-        return "%dB" % (bytes)
>-    if bytes < 1024 * 1024:
>-        return "%.1fKB" % (bytes / 1024)
>-    if bytes < 1024 * 1024 * 1024:
>-        return "%.1fMB" % (bytes / (1024 * 1024))
>-    return "%.1fGB" % (bytes / (1024 * 1024 * 1024))
>+        if isMemoryMetric(shortName(count_type)):
>+          writer.writerow(['RETURN: ' + counterName + ': ' + filesizeformat(avg_excluding_max(cd[count_type])),])
>+        else:
>+          writer.writerow(['RETURN: ' + counterName + ': ' + avg_excluding_max(cd[count_type]),])
> 
> def construct_results (machine, testname, branch, sourcestamp, buildid, date, vals):
>   """ 
>   Creates string formated for the collector script of the graph server
>   Returns the completed string
>   """
>   #machine_name,test_name,branch_name,sourcestamp,buildid,date_run
>   info_format = "%s,%s,%s,%s,%s,%s\n"
>@@ -264,44 +272,42 @@ def send_to_graph(results_server, result
> 
> def results_from_graph(links, results_server):
>   #take the results from the graph server collection script and put it into a pretty format for the waterfall
>   url_format = "http://%s/%s"
>   link_format= "<a href=\'%s\'>%s</a>"
>   first_results = 'RETURN:<br>'
>   last_results = '' 
>   full_results = '\nRETURN:<p style="font-size:smaller;">Details:<br>'  
>-  memory_metric = ['memset', 'rss', 'pbytes', 'xres', 'modlistbytes']
>   lines = links.split('\n')
>   for line in lines:
>     if line == "":
>       continue
>     linkvalue = -1
>     linkdetail = ""
>     values = line.split("\t")
>     linkName = values[0]
>     if len(values) == 2:
>       linkdetail = values[1]
>     else:
>       linkvalue = float(values[1])
>       linkdetail = values[2]
>     if linkvalue > -1:
>-      if filter(lambda x: x in linkName, memory_metric): #measured in bytes
>+      if isMemoryMetric(linkName):
>         linkName += ": " + filesizeformat(linkvalue)
>       else:
>         linkName += ": " + str(linkvalue)
>       url = url_format % (results_server, linkdetail)
>       link = link_format % (url, linkName)
>       first_results = first_results + "\nRETURN:" + link + "<br>"
>     else:
>       url = url_format % (results_server, linkdetail)
>       link = link_format % (url, linkName)
>       last_results = last_results + '| ' + link + ' '
>   full_results = first_results + full_results + last_results + '|</p>'
>-  print 'RETURN: new graph links'
>   print full_results
> 
> def browserInfo(browser_config, devicemanager = None):
>   """Get the buildid and sourcestamp from the application.ini (if it exists)
>   """
>   appIniFileName = "application.ini"
>   appIniPath = os.path.join(os.path.dirname(browser_config['browser_path']), appIniFileName)
>   if os.path.isfile(appIniPath) or devicemanager != None:

Comment 59

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]cleaner tinderbox summaries for addons tests (take 2)

Fixed review suggestions.

Comment 60

[Jeff Hammel]

Comment on attachment 459152 [details] [diff] [review]
[checked in]cleaner tinderbox summaries for addons tests (take 2)

looks good!

Comment 61

[alice nodelman [:alice] [:anode]]

Comment on attachment 459152 [details] [diff] [review]
[checked in]cleaner tinderbox summaries for addons tests (take 2)

/cvsroot/mozilla/testing/performance/talos/run_tests.py,v  <--  run_tests.py
new revision: 1.65; previous revision: 1.64
done

Comment 62

[alice nodelman [:alice] [:anode]]

Going to follow up with reporting issues in bug 582104, otherwise we're all done here.