596255 - talos should use optparse.OptionParser instead of getopt

Status: RESOLVED FIXED

(Testing :: Talos, enhancement)

Description

[Jeff Hammel]

adding and parsing command line options in Talos is more cumbersome than it could be due to the use of getopt.  using OptionParser would cut down on the number of lines of code and increase the ease of adding options

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: convert [remote]PerfConfigurator to use optparse (1.0)

passes a series of local tests and I feel comfortable for the first review.  Of course next stages would be staging run and anode review.

Comment 2

[Jeff Hammel]

Comment on attachment 481035 [details] [diff] [review]
convert [remote]PerfConfigurator to use optparse (1.0)

diff --git a/PerfConfigurator.py b/PerfConfigurator.py
--- a/PerfConfigurator.py
+++ b/PerfConfigurator.py
@@ -9,29 +9,29 @@ Modified by Rob Campbell on 2007-06-26 -
 Modified by Rob Campbell on 2007-07-06 - added -d testDate option
 Modified by Ben Hearsum on 2007-08-22 - bugfixes, cleanup, support for multiple platforms. Only works on Talos2
 Modified by Alice Nodelman on 2008-04-30 - switch to using application.ini, handle different time stamp formats/options
 Modified by Alice Nodelman on 2008-07-10 - added options for test selection, graph server configuration, nochrome
 Modified by Benjamin Smedberg on 2009-02-27 - added option for symbols path
 """
 
 import sys
-import getopt
 import re
 import time
 from datetime import datetime
 from os import path
 import os
+import optparse
 
 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 --testPrefix prefix --extension addon
+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 --testPrefix prefix --extension addon

Not sure the reason to list the options here.  Shouldn't this be
automatically illuminated by `PerfConfigurator.py --help` now that
we're using optparse?
 
 example testlist: tp:tsspider:tdhtml:twinopen
 '''
 
 class PerfConfigurator:
     exePath = ""
     configPath = "."
     sampleConfig = "sample.config"
@@ -46,30 +46,19 @@ class PerfConfigurator:
     testDate = ""
     useId = False
     resultsServer = ''
     resultsLink = ''
     activeTests = ''
     noChrome = False
     fast = False
     testPrefix = ''
-    remoteDevice = ''
-    webServer = ''
-    deviceRoot = ''
-    _remote = False
-    port = ''
     extension = ''
     masterIniSubpath = "application.ini"
 
-    def _setupRemote(self, host, port = 20701):
-        import devicemanager
-        self.testAgent = devicemanager.DeviceManager(host, port)
-        self._remote = True
-        self.deviceRoot = self.testAgent.getDeviceRoot()
-    


     def _dumpConfiguration(self):
         """dump class configuration for convenient pickup or perusal"""
         print "Writing configuration:"

Not part of the patch, but I would much prefer something like:

"""
attributes = ['title', 'exePath', 'configPath', ...]
for i in attributes:
  print " - %s = %s" % (i, getattr(self, i))
"""

This way, subclasses can just extend attributes

         print " - title = " + self.title
         print " - executablePath = " + self.exePath
         print " - configPath = " + self.configPath
         print " - sampleConfig = " + self.sampleConfig
         print " - outputName = " + self.outputName
@@ -79,21 +68,16 @@ class PerfConfigurator:
         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
     
     def _getCurrentDateString(self):
         """collect a date string to be used in naming the created config file"""
         currentDateTime = datetime.now()
         return currentDateTime.strftime("%Y%m%d_%H%M")
 
@@ -129,323 +113,258 @@ class PerfConfigurator:
         if len(self.buildid) == 14: 
           buildIdTime = time.strptime(self.buildid, "%Y%m%d%H%M%S")
         elif len(self.buildid) == 12: 
           buildIdTime = time.strptime(self.buildid, "%Y%m%d%H%M")
         else:
           buildIdTime = time.strptime(self.buildid, "%Y%m%d%H")
         return time.strftime("%a, %d %b %Y %H:%M:%S GMT", buildIdTime)
 
-    def convertUrlToRemote(self, line):
-        """
-          This can be filled in (preferred in a subclass) to modify 
-          a url line to support a remote webserver
-        """
-        return line
+    def convertLine(self, line, testMode, printMe):
+        buildidString = "'" + str(self.buildid) + "'"
+        activeList = self.activeTests.split(':')
+        newline = line
+        if 'browser_path:' in line:
+            newline = 'browser_path: ' + self.exePath + '\n'
+        if 'title:' in line:
+            newline = 'title: ' + self.title + '\n'
+            if self.testDate:
+                newline += '\n'
+                newline += 'testdate: "%s"\n' % self._getTimeFromTimeStamp()
+            elif self.useId:
+                newline += '\n'
+                newline += 'testdate: "%s"\n' % self._getTimeFromBuildId()
+            if self.branchName: 
+                newline += '\n'
+                newline += 'branch_name: %s\n' % self.branchName
+            if self.noChrome:
+                newline += '\n'
+                newline += "test_name_extension: _nochrome\n"
+            if self.symbolsPath:
+                newline += '\nsymbols_path: %s\n' % self.symbolsPath
+        if self.extension and ('extensions : {}' in line):
+            newline = 'extensions: ' + '\n- ' + self.extension
+        if 'buildid:' in line:
+            newline = 'buildid: %s\n' % buildidString
+        if 'talos.logfile:' in line:
+            parts = line.split(':')
+            if (parts[1] != None and parts[1].strip() == ''):
+                lfile = os.path.join(os.getcwd(), 'browser_output.txt')
+            else:
+                lfile = parts[1].strip().strip("'")
+                  
+            newline = '%s: %s\n' % (parts[0], lfile)
+        if 'testbranch' in line:
+            newline = 'branch: ' + self.branch
+
+        #only change the results_server if the user has provided one
+        if self.resultsServer and ('results_server' in line):
+            newline = 'results_server: ' + self.resultsServer + '\n'
+        #only change the results_link if the user has provided one
+        if self.resultsLink and ('results_link' in line):
+            newline = 'results_link: ' + self.resultsLink + '\n'
+        #only change the browser_wait if the user has provided one
+        if self.browserWait and ('browser_wait' in line):
+            newline = 'browser_wait: ' + self.browserWait + '\n'
+        if testMode:
+            #only do this if the user has provided a list of tests to turn on/off
+            # otherwise, all tests are considered to be active
+            if self.activeTests:
+                if line.startswith('- name'): 
+                    #found the start of an individual test description
+                    printMe = False
+                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 ','')
+        return newline
 
I'll ignore this for this patch, as this is just a code move, but I'd
love to see this cleaned up at some point.

     def writeConfigFile(self):
         configFile = open(path.join(self.configPath, self.sampleConfig))
         destination = open(self.outputName, "w")
         config = configFile.readlines()
         configFile.close()
-        buildidString = "'" + str(self.buildid) + "'"
-        activeList = self.activeTests.split(':')
         printMe = True
         testMode = False
         for line in config:
-            newline = line
-            if 'browser_path:' in line:
-                newline = 'browser_path: ' + self.exePath + '\n'
-            if 'title:' in line:
-                newline = 'title: ' + self.title + '\n'
-                if self.testDate:
-                    newline += '\n'
-                    newline += 'testdate: "%s"\n' % self._getTimeFromTimeStamp()
-                elif self.useId:
-                    newline += '\n'
-                    newline += 'testdate: "%s"\n' % self._getTimeFromBuildId()
-                if self.branchName: 
-                    newline += '\n'
-                    newline += 'branch_name: %s\n' % self.branchName
-                if self.noChrome:
-                    newline += '\n'
-                    newline += "test_name_extension: _nochrome\n"
-                if self.symbolsPath:
-                    newline += '\nsymbols_path: %s\n' % self.symbolsPath
-            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: %s\n' % buildidString
-            if 'talos.logfile:' in line:
-                parts = line.split(':')
-                if (parts[1] != None and parts[1].strip() == ''):
-                  lfile = os.path.join(os.getcwd(), 'browser_output.txt')
-                else:
-                  lfile = parts[1].strip().strip("'")
-                  
-                if self._remote == True:
-                    lfile = self.deviceRoot + '/' + lfile.split('/')[-1]
-                
-                newline = '%s: %s\n' % (parts[0], lfile)
-            if 'testbranch' in line:
-                newline = 'branch: ' + self.branch
-            if self._remote == True and ('init_url' in line):
-                newline = self.convertUrlToRemote(line)
-
-            #only change the results_server if the user has provided one
-            if self.resultsServer and ('results_server' in line):
-                newline = 'results_server: ' + self.resultsServer + '\n'
-            #only change the results_link if the user has provided one
-            if self.resultsLink and ('results_link' in line):
-                newline = 'results_link: ' + self.resultsLink + '\n'
-            #only change the browser_wait if the user has provided one
-            if self.browserWait and ('browser_wait' in line):
-                newline = 'browser_wait: ' + self.browserWait + '\n'
-            if testMode:
-                #only do this if the user has provided a list of tests to turn on/off
-                # otherwise, all tests are considered to be active
- 
-                if ('url' in line) and ('url_mod' not in line) and (self._remote == True):
-                    line = self.convertUrlToRemote(line)
-
-                if self.activeTests:
-                    if line.startswith('- name'): 
-                        #found the start of an individual test description
-                        printMe = False
-                    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 ','')
+            newline = self.convertLine(line, testMode, printMe)
             if printMe:
                 destination.write(newline)
             if line.startswith('tests :'): 
                 #enter into test writing mode
                 testMode = True
                 if self.activeTests:
                     printMe = False
         destination.close()
         if self.verbose:
             self._dumpConfiguration()
     
-    def __init__(self, **kwargs):
-        if 'title' in kwargs:
-            self.title = kwargs['title']
-        if 'branch' in kwargs:
-            self.branch = kwargs['branch']
-        if 'branchName' in kwargs:
-            self.branchName = kwargs['branchName']
-        if 'executablePath' in kwargs:
-            self.exePath = kwargs['executablePath']
-        if 'configFilePath' in kwargs:
-            self.configPath = kwargs['configFilePath']
-        if 'sampleConfig' in kwargs:
-            self.sampleConfig = kwargs['sampleConfig']
-        if 'outputName' in kwargs:
-            self.outputName = kwargs['outputName']
-        if 'buildid' in kwargs:
-            self.buildid = kwargs['buildid']
-        if 'verbose' in kwargs:
-            self.verbose = kwargs['verbose']
-        if 'testDate' in kwargs:
-            self.testDate = kwargs['testDate']
-        if 'browserWait' in kwargs:
-            self.browserWait = kwargs['browserWait']
-        if 'resultsServer' in kwargs:
-            self.resultsServer = kwargs['resultsServer']
-        if 'resultsLink' in kwargs:
-            self.resultsLink = kwargs['resultsLink']
-        if 'activeTests' in kwargs:
-            self.activeTests = kwargs['activeTests']
-        if 'noChrome' in kwargs:
-            self.noChrome = kwargs['noChrome']
-        if 'fast' in kwargs:
-            self.fast = kwargs['fast']
-        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.port == None or self.port == '' or self.port <= 0):
-                self.port = '20701'
-
-        if (self.remoteDevice <> ''):
-          self._setupRemote(self.remoteDevice, self.port)
+    def __init__(self, options):
+        self.title = options.title
+        self.branch = options.branch
+        self.branchName = options.branchName
+        self.exePath = options.exePath
+        self.configPath = options.configFilePath
+        self.sampleConfig = options.sampleConfig
+        self.outputName = options.outputName
+        self.buildid = options.buildid
+        self.verbose = options.verbose
+        self.testDate = options.testDate
+        self.browserWait = options.browserWait
+        self.resultsServer = options.resultsServer
+        self.resultsLink = options.resultsLink
+        self.activeTests = options.activeTests
+        self.noChrome = options.noChrome
+        self.fast = options.fast
+        self.symbolsPath = options.symbolsPath
+        self.testPrefix = options.testPrefix
+        self.extension = options.extension
 
I'd much prefer if this was not just a case of
self.${var} = options.${var}

If you're setting self.${var} for all cases in options, a few ideas:

1:
"""
def __init__(self, options):
  self.__dict__.update(options.__dict__)
"""

2.
"""
def __init__(self, **kwargs)
  self.__dict__.update(kwargs)

...
Perfconfigurator(**options.__dict__)
"""

Other variations on these patterns are possible.  A popular one is
putting something like

"""
class PerfConfigurator(object):
  defaults = { 'foo': 'bar', ...}
  def __init__(self, **kwargs):
    for key in defaults:
      setattr(self, key, kwargs.get(key, defaults[key]))
"""

If you're concerned about people passing all sorts of crazy things in
(I'm not) then this allows only things in defaults to be set on self.
And if you want even more introspection, you can use this as a seed to
the OptionParser.  IMHO, this is a bigger hammer than is needed;  i'd
be happy with either option 1. or 2.

         self.currentDate = self._getCurrentDateString()
         if not self.buildid:
             self.buildid = self._getCurrentBuildId()
         if not self.outputName:
             self.outputName = self.currentDate + "_config.yml"
 
 class Configuration(Exception):
     def __init__(self, msg):
         self.msg = "ERROR: " + msg
 
 class Usage(Exception):
     def __init__(self, msg):
         self.msg = msg
 
+class TalosOptions(optparse.OptionParser):

Is there any reason to sub-class here?  Why not just have an e.g. 
`def add_options(parser)` that adds the options to the parser?

+    """Parses Mochitest commandline options."""
+    def __init__(self, **kwargs):
+        optparse.OptionParser.__init__(self, **kwargs)
+        defaults = {}
+
+        self.add_option("-v", "--verbose",
+                        action = "store_true", dest = "verbose",
+                        help = "display verbose output")
+        defaults["verbose"] = False

Why use defaults as a standalone variable instead of passing them in
to the the add_option argument?  e.g.

+        self.add_option("-v", "--verbose", default=False,
+                        action = "store_true", dest = "verbose",
+                        help = "display verbose output")


+    
+        self.add_option("-e", "--executablePath",
+                        action = "store", dest = "exePath",
+                        help = "path to executable we are testing")
+        defaults["exePath"] = ''
+    
+        self.add_option("-c", "--configFilePath",
+                        action = "store", dest = "configFilePath",
+                        help = "path to config file")
+        defaults["configFilePath"] = ''
+
+        self.add_option("-f", "--sampleConfig",
+                        action = "store", dest = "sampleConfig",
+                        help = "Input config file")
+        defaults["sampleConfig"] = 'sample.config'
+
+        self.add_option("-t", "--title",
+                        action = "store", dest = "title",
+                        help = "Title of the test run")
+        defaults["title"] = defaultTitle
+    
+        self.add_option("--branchName",
+                        action = "store", dest = "branchName",
+                        help = "Name of the branch we are testing on")
+        defaults["branchName"] = ''
+
+        self.add_option("-b", "--branch",
+                        action = "store", dest = "branch",
+                        help = "Product branch we are testing on")
+        defaults["branch"] = ''
+
+        self.add_option("-o", "--output",
+                        action = "store", dest = "outputName",
+                        help = "Output file")
+        defaults["outputName"] = ''
+
+        self.add_option("-i", "--id",
+                        action = "store_true", dest = "buildid",
+                        help = "Build ID of the product we are testing")
+        defaults["buildid"] = ''
+
+        self.add_option("-u", "--useId",
+                        action = "store", dest = "useId",
+                        help = "Use the buildid as the testdate")
+        defaults["useId"] = False
+
+        self.add_option("-d", "--testDate",
+                        action = "store", dest = "testDate",
+                        help = "Test date for the test run")
+        defaults["testDate"] = ''
+
+        self.add_option("-w", "--browserWait",
+                        action = "store", dest = "browserWait",
+                        help = "Amount of time allowed for the browser to cleanly close")
+        defaults["browserWait"] = '5'

Why not store the int? `type=int`

+
+        self.add_option("-s", "--resultsServer",
+                        action = "store", dest = "resultsServer",
+                        help = "Address of the results server")
+        defaults["resultsServer"] = ''

Why not store None instead of an empty string?

+        self.add_option("-l", "--resultsLink",
+                        action = "store", dest = "resultsLink",
+                        help = "Link to the results from this test run")
+        defaults["resultsLink"] = ''
+
+        self.add_option("-a", "--activeTests",
+                        action = "store", dest = "activeTests",
+                        help = "List of tests to run, separated by ':' (ex. ts:tp4:tsvg)")
+        defaults["activeTests"] = ''
+
+        self.add_option("-n", "--noChrome",
+                        action = "store_true", dest = "noChrome",
+                        help = "do not run tests as chrome")
+        defaults["noChrome"] = False
+
+        self.add_option("--testPrefix",
+                        action = "store", dest = "testPrefix",
+                        help = "the prefix for the test we are running")
+        defaults["testPrefix"] = ''
+
+        self.add_option("--extension",
+                        action = "store", dest = "extension",
+                        help = "Extension to install while running")
+        defaults["extension"] = ''
+    
+        self.add_option("--fast",
+                        action = "store_true", dest = "fast",
+                        help = "Run tp tests as tp_fast")
+        defaults["fast"] = False
+    
+        self.add_option("--symbolsPath",
+                        action = "store", dest = "symbolsPath",
+                        help = "Path to the symbols for the build we are testing")
+        defaults["symbolsPath"] = ''
+        self.set_defaults(**defaults)
+    
+    def verifyOptions(self, options):
+        return options

What's the point of this function?  Just so it can be subclassed?

 def main(argv=None):
-    exePath = ""
-    configPath = ""
-    sampleConfig = "sample.config"
-    output = ""
-    title = defaultTitle
-    branch = ""
-    branchName = ""
-    testDate = ""
-    browserWait = "5"
-    verbose = False
-    buildid = ""
-    useId = False
-    resultsServer = ''
-    resultsLink = ''
-    activeTests = ''
-    noChrome = False
-    fast = False
-    symbolsPath = None
-    remoteDevice = ''
-    remotePort = ''
-    webServer = 'localhost'
-    deviceRoot = ''
-    testPrefix = ''
-    extension = ''
+    parser = TalosOptions()
+    options, args = parser.parse_args()
 
-    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", "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
-            if option in ("-h", "--help"):
-                raise Usage(help_message)
-            if option in ("-e", "--executablePath"):
-                exePath = value
-            if option in ("-c", "--configFilePath"):
-                configPath = value
-            if option in ("-f", "--sampleConfig"):
-                sampleConfig = value
-            if option in ("-t", "--title"):
-                title = value
-            if option in ("-b", "--branch"):
-                branch = value
-            if option in ("--branchName"):
-                branchName = value
-            if option in ("-o", "--output"):
-                output = value
-            if option in ("-i", "--id"):
-                buildid = value
-            if option in ("-d", "--testDate"):
-                testDate = value
-            if option in ("-w", "--browserWait"):
-                browserWait = value
-            if option in ("-u", "--useId"):
-                useId = True
-            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
-            if option in ("--fast",):
-                fast = True
-            if option in ("--symbolsPath",):
-                symbolsPath = value
-        
-    except Usage, err:
-        print >> sys.stderr, sys.argv[0].split("/")[-1] + ": " + str(err.msg)
-        print >> sys.stderr, "\t for help use --help"
-        return 2

I like to see those lines gone :)

+    options = parser.verifyOptions(options)
+    if options == None:
+        sys.exit(1)
 
How would this ever happen?

-    #remotePort will default to 20701 and is optional.
-    #webServer can be used without remoteDevice, but is required when using remoteDevice
-    if (remoteDevice != '' or deviceRoot != ''):
-        if (webServer == 'localhost' or remoteDevice == ''):
-            print "\nERROR: When running Talos on a remote device, you need to provide a webServer, and optionally a remotePort"
-            print help_message
-            return 2
-
-    configurator = PerfConfigurator(title=title,
-                                    executablePath=exePath,
-                                    configFilePath=configPath,
-                                    sampleConfig=sampleConfig,
-                                    buildid=buildid,
-                                    branch=branch,
-                                    branchName=branchName,
-                                    verbose=verbose,
-                                    testDate=testDate,
-                                    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)
+    configurator = PerfConfigurator(options)
     try:
         configurator.writeConfigFile()
     except Configuration, err:
         print >> sys.stderr, sys.argv[0].split("/")[-1] + ": " + str(err.msg)
         return 5
     return 0
 
 
diff --git a/remotePerfConfigurator.py b/remotePerfConfigurator.py
--- a/remotePerfConfigurator.py
+++ b/remotePerfConfigurator.py
@@ -1,14 +1,65 @@
 import PerfConfigurator as pc
-import os, sys, getopt
+import os, sys
+import optparse
 
 class remotePerfConfigurator(pc.PerfConfigurator):
-    def __init__(self, **kwargs):
-        pc.PerfConfigurator.__init__(self, **kwargs)
+    def __init__(self, options):
+        self.webServer = options.webServer
+        self.remoteDevice = options.remoteDevice
+        self.port = options.remotePort
+        self.deviceRoot = options.deviceRoot
+        self._remote = False

Again, see earlier for how I would do that __init__. And...

+        if (self.remoteDevice <> ''):
+            self._setupRemote()

You can do post-settinf configuration somewhere afterwards here.

+        #this depends on buildID which requires querying the device
+        pc.PerfConfigurator.__init__(self, options)
+
+    def _setupRemote(self):
+        import devicemanager
+        self.testAgent = devicemanager.DeviceManager(self.remoteDevice, self.port)
+        self.deviceRoot = self.testAgent.getDeviceRoot()
+        self._remote = True
+
+    def _dumpConfiguration(self):
+        pc.PerfConfigurator._dumpConfiguration(self)
+        print " - deviceIP = " + self.remoteDevice
+        print " - devicePort = " + self.port
+        print " - webServer = " + self.webServer
+        print " - deviceRoot = " + self.deviceRoot

Again, see earlier comment.

+    def convertLine(self, line, testMode, printMe):
+        newline = pc.PerfConfigurator.convertLine(self, line, testMode, printMe)
+        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 'remote:' in line:
+            newline = 'remote: %s\n' % self._remote
+        if 'init_url' in line:
+            newline = self.convertUrlToRemote(line)
+        if testMode:
+            if ('url' in line) and ('url_mod' not in line):
+                line = self.convertUrlToRemote(line)
+        if 'talos.logfile:' in line:
+            parts = line.split(':')
+            if (parts[1] != None and parts[1].strip() == ''):
+                lfile = os.path.join(os.getcwd(), 'browser_output.txt')
+            else:
+                lfile = parts[1].strip().strip("'")
+            lfile = self.deviceRoot + '/' + lfile.split('/')[-1]
+            newline = '%s: %s\n' % (parts[0], lfile)
+        return newline
 
     def convertUrlToRemote(self, line):
         """
           For a give url line in the .config file, add a webserver.
           In addition if there is a .manifest file specified, covert 
           and copy that file to the remote device.
         """
         parts = line.split(' ')
@@ -68,146 +119,62 @@ class remotePerfConfigurator(pc.PerfConf
         else:
             return pc.PerfConfigurator(self)
             
         data = master.read()
         master.close()
         return data.split('\n')
 
 
+class remoteTalosOptions(pc.TalosOptions):

Likewise, why use a freestanding class instead of just an add_options function?

+    def __init__(self, **kwargs):
+        defaults = {}
+        pc.TalosOptions.__init__(self)
+
+        self.add_option("-r", "--remoteDevice", action="store",
+                    type = "string", dest = "remoteDevice",
+                    help = "Device IP of the SUTAgent")
+        defaults["remoteDevice"] = ''
+
+        self.add_option("-p", "--remotePort", action="store",
+                    type = "string", dest = "remotePort",
+                    help = "port the SUTAgent uses (defaults to 20701")
+        defaults["remotePort"] = '20701'
+
+        self.add_option("--webServer", action="store",
+                    type = "string", dest = "webServer",
+                    help = "IP address of the webserver hosting the talos files")
+        defaults["webServer"] = ''
+
+        self.add_option("--deviceRoot", action="store",
+                    type = "string", dest = "deviceRoot",
+                    help = "path on the device that will hold files and the profile")
+        defaults["deviceRoot"] = ''
+        self.set_defaults(**defaults)
+
+    def verifyOptions(self, options):
+        #webServer can be used without remoteDevice, but is required when using remoteDevice
+        if (options.remoteDevice != '' or options.deviceRoot != ''):
+            if (options.webServer == 'localhost'  or options.remoteDevice == ''):
+                print "\nERROR: When running Talos on a remote device, you need to provide a webServer and optionally a remotePort"
+                return None

Isn't it better to raise an exception?

raise AssertionError("When running Talos on a remote device...")

Then you can catch it and print, etc. (or just let the error cause the sys.exit(1)

+        return options
+    
 def main(argv=None):
-    exePath = ""
-    configPath = ""
-    sampleConfig = "sample.config"
-    output = ""
-    title = "mobile-01"
-    branch = ""
-    branchName = ""
-    testDate = ""
-    browserWait = "5"
-    verbose = False
-    buildid = ""
-    useId = False
-    resultsServer = ''
-    resultsLink = ''
-    activeTests = ''
-    noChrome = False
-    fast = False
-    symbolsPath = None
-    remoteDevice = ''
-    remotePort = ''
-    webServer = 'localhost'
-    deviceRoot = ''
-    testPrefix = ''
-    extension = ''
+        
+    parser = remoteTalosOptions()
+    options, args = parser.parse_args()

 
-    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", "testPrefix=", "extension=", "branchName=", "fast", "symbolsPath=",
-                "remoteDevice=", "remotePort=", "webServer=", "deviceRoot="])
-        except getopt.error, msg:
-            raise pc.Usage(msg)
-        
-        # option processing
-        for option, value in opts:
-            if option in ("-v", "--verbose"):
-                verbose = True
-            if option in ("-h", "--help"):
-                raise Usage(help_message)
-            if option in ("-e", "--executablePath"):
-                exePath = value
-            if option in ("-c", "--configFilePath"):
-                configPath = value
-            if option in ("-f", "--sampleConfig"):
-                sampleConfig = value
-            if option in ("-t", "--title"):
-                title = value
-            if option in ("-b", "--branch"):
-                branch = value
-            if option in ("--branchName"):
-                branchName = value
-            if option in ("-o", "--output"):
-                output = value
-            if option in ("-i", "--id"):
-                buildid = value
-            if option in ("-d", "--testDate"):
-                testDate = value
-            if option in ("-w", "--browserWait"):
-                browserWait = value
-            if option in ("-u", "--useId"):
-                useId = True
-            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
-            if option in ("--fast",):
-                fast = True
-            if option in ("--symbolsPath",):
-                symbolsPath = value
-        
-    except pc.Usage, err:
-        print >> sys.stderr, sys.argv[0].split("/")[-1] + ": " + str(err.msg)
-        print >> sys.stderr, "\t for help use --help"
-        return 2

Likewise, so glad to see these lines gone.

+    options = parser.verifyOptions(options)

... and then call the free-standing verifyOptions here?

+    if options == None:
+        print pc.PerfConfigurator.help_message
+        sys.exit(1)
should use the parser function for this, e.g.:

parser.error(pc.PerfConfigurator.help_message)

See also comment above about try: except:
 
-    #remotePort will default to 20701 and is optional.
-    #webServer can be used without remoteDevice, but is required when using remoteDevice
-    if (remoteDevice != '' or deviceRoot != ''):
-        if (webServer == 'localhost'  or remoteDevice == ''):
-            print "\nERROR: When running Talos on a remote device, you need to provide a webServer and optionally a remotePort"
-            print pc.help_message
-            return 2
-
-    configurator = remotePerfConfigurator(title=title,
-                                    executablePath=exePath,
-                                    configFilePath=configPath,
-                                    sampleConfig=sampleConfig,
-                                    buildid=buildid,
-                                    branch=branch,
-                                    branchName=branchName,
-                                    verbose=verbose,
-                                    testDate=testDate,
-                                    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)
+    configurator = remotePerfConfigurator(options)
     try:
         configurator.writeConfigFile()
     except pc.Configuration, err:
         print >> sys.stderr, sys.argv[0].split("/")[-1] + ": " + str(err.msg)
         return 5
     return 0
     
 if __name__ == "__main__":

This is mostly really good -- thanks for doing this! I think it is
really close, but I'd like at least some of these issues to be
addressed, especially cleaning up the help message, not subclassing
(unless there's a more compelling reason than I see here), using the
default as an argument to add_option, and exiting with the
parser.error function.  If there are legitimate reasons for these,
then maybe I'm being too critical, but if not, I'd rather seen them
cleaned up (and it would be nice to lean towards DRY whenever
possible).

So much bad code gone...me likes, me likes

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: convert [remote]PerfConfigurator to use optparse (2.0)

updated to address feedback and bitrot.  100% green on tools-staging-master

Comment 4

[Jeff Hammel]

Comment on attachment 486639 [details] [diff] [review]
convert [remote]PerfConfigurator to use optparse (2.0)

looks good!  this is a huge improvement and i hope will make Talos easier to extend for future work

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks for the review :jhammel!

pushed:
http://hg.mozilla.org/build/talos/pushloghtml?changeset=0c063ab3aa31