Chrome updater test framework

pyautolib/fetch_prebuilt_pyauto.py

Index: pyautolib/fetch_prebuilt_pyauto.py
===================================================================
--- pyautolib/fetch_prebuilt_pyauto.py	(revision 136323)
+++ pyautolib/fetch_prebuilt_pyauto.py	(working copy)
@@ -28,70 +28,52 @@
 import urllib
 import urllib2
 import urlparse
+import zipfile
+import socket
 
 import pyauto_utils
 
-
+            
 class FetchPrebuilt(object):
-  """Util class to fetch prebuilt binaries to run PyAuto."""
+  """Util class to fetch prebuilt binaries to run PyAuto."""  
+  
+  def __init__(self, url, outdir, platform):
+    self._url = (lambda u: self.DoesUrlExist(u) and u or '')(url)    

[kkania, 2012/05/22 16:36:42]

i would assume that url is not None here; just check that it exists, and if it
doesn't throw an exception

+    self._outdir = outdir    
+    self._platform = ((lambda p: p and p or pyauto_utils.GetCurrentPlatform())
+                      (platform))
+    if not self._url:

[kkania, 2012/05/22 16:36:42]

i would move this to parse args

+      print 'The specified URL does not exist, aborting...'
+      sys.exit(-1)
 
-  def _ParseArgs(self):
-    parser = optparse.OptionParser()
-    parser.add_option(
-        '-d', '--outdir', type='string', default=None,
-        help='Directory in which to setup. This is typically the directory '
-        'where the binaries would go when compiled from source.')
-    parser.add_option(
-        '-p', '--platform', type='string',
-        default=pyauto_utils.GetCurrentPlatform(),
-        help='Platform. Valid options: win, mac, linux32, linux64. '
-        'Default: current platform (%s)' % pyauto_utils.GetCurrentPlatform())
-    parser.add_option(
-        '-l', '--latest', action='store_true', default=False,
-        help='Download the latest chromium build from commondatastorage. '
-        '[default=False]')
-    self._options, self._args = parser.parse_args()
-    if self._options.latest:
-      self._url = self._GetLastestDownloadURL(self._options.platform)
-    elif not self._args:
-      print >>sys.stderr, 'Need download url'
-      sys.exit(2)
+  def _GetZipName(self):

[kkania, 2012/05/22 16:36:42]

I don't like this name. I expect it to return the zip name, where instead it
sets some class variable. Change this to InitZipName and call it from __init__
instead of Run.

+    """Determines the name of the zip file based on platform."""
+    if not self._platform.startswith('linux'):
+      self._chrome_zip_name = 'chrome-%s' % { 'mac' : 'mac',
+                                              'win' : 'win32'
+                                            }[self._platform]
     else:
-      self._url = self._args[0]
-    if not self._options.outdir:
-      print >>sys.stderr, 'Need output directory: -d/--outdir'
-      sys.exit(1)
-    self._outdir = self._options.outdir
-    # Chromium continuous build archive has a non-standard format.
-    if 'index.html?path=' in self._url:
-      self._url = self._url.replace('index.html?path=', '')
-    self._url = self._url.rstrip('/')
-    # Determine name of zip.
-    if not self._options.platform.startswith('linux'):
-      self._chrome_zip_name = 'chrome-%s' % {'mac': 'mac',
-                                             'win': 'win32'
-                                            }[self._options.platform]
-    else:
       linux_32_names = ['linux', 'lucid32bit']
       linux_64_names = ['linux64', 'lucid64bit']
       linux_names = {'linux': linux_32_names + linux_64_names,
                      'linux32': linux_32_names,
                      'linux64': linux_64_names
-                    }[self._options.platform]
+                    }[self._platform]
       for name in linux_names:
         zip_name = 'chrome-' + name
-        if self._DoesURLExist('%s/%s.zip' % (self._url, zip_name)):
+        if self.DoesUrlExist('%s/%s.zip' % (self._url, zip_name)):
           self._chrome_zip_name = zip_name
           break
       else:
         raise RuntimeError('Could not find chrome zip at ' + self._url)
 
-    # Setup urls to download.
+  def _GetDownloadUrls(self):

[kkania, 2012/05/22 16:36:42]

Same as _GetZipName

+    """Generates URLs for files that are to be downloaded."""
     self._chrome_zip_url = '%s/%s.zip' % (self._url, self._chrome_zip_name)
     self._remoting_zip_url = self._url + '/' + 'remoting-webapp.zip'
     chrome_test_url = '%s/%s.test' % (self._url, self._chrome_zip_name)
     self._pyautolib_py_url = '%s/pyautolib.py' % chrome_test_url
-    if self._options.platform == 'win':
+    if self._platform == 'win':
       self._pyautolib_so_name = '_pyautolib.pyd'
       self._chromedriver_name = 'chromedriver.exe'
     else:
@@ -100,7 +82,8 @@
     self._pyautolib_so_url = chrome_test_url + '/' + self._pyautolib_so_name
     self._chromedriver_url = chrome_test_url + '/' + self._chromedriver_name
 
-  def _GetLastestDownloadURL(self, os_platform):
+  @staticmethod
+  def GetLastestDownloadURL(os_platform):
     os_type = {'win': 'Win',
                'mac': 'Mac',
                'linux': 'Linux',
@@ -120,26 +103,34 @@
                                                               last_change))
     return last_change_url
 
-  def _DoesURLExist(self, url):
+  def DoesUrlExist(self, url):
     """Determines whether a resource exists at the given URL."""
     parsed = urlparse.urlparse(url)
-    conn = httplib.HTTPConnection(parsed.netloc)
-    conn.request('HEAD', parsed.path)
-    response = conn.getresponse()
-    if response.status == 302:  # Redirect; follow it.
-      return self._DoesURLExist(response.getheader('location'))
+    try:
+      conn = httplib.HTTPConnection(parsed.netloc)
+      conn.request('HEAD', parsed.path)
+      response = conn.getresponse()
+    except(socket.gaierror, socket.error), err:

[kkania, 2012/05/22 16:36:42]

space between except and '('? check the style guide

+      print 'FetchPrebuilt.DoesUrlExist: %s' %(err)

[kkania, 2012/05/22 16:36:42]

% err

+      return False
+    # Redirect, follow it (301:Permanent/302:temporary, both should be caught)

[kkania, 2012/05/22 16:36:42]

'.' at end; use an english sentence, like:
# Follow both permanent (301) and temporary (302) redirects.

+    if response.status == 302 or response.status == 301:  
+      return self.DoesUrlExist(response.getheader('location'))
     return response.status == 200
 
   def Cleanup(self):
     """Remove old binaries, if any."""
     pass
 
-  def Run(self):
-    self._ParseArgs()
+  def Run(self):    
     if not os.path.isdir(self._outdir):
-      os.makedirs(self._outdir)
-    get_remoting = self._DoesURLExist(self._remoting_zip_url)
-
+      os.makedirs(self._outdir)      
+    # Generate name of the zip file that will be downloaded.
+    self._GetZipName()
+    # Generate URLs for files that are to be downloaded.
+    self._GetDownloadUrls()
+    get_remoting = self.DoesUrlExist(self._remoting_zip_url)
+    
     # Fetch chrome & pyauto binaries
     print 'Fetching', self._chrome_zip_url
     chrome_zip = urllib.urlretrieve(self._chrome_zip_url)[0]
@@ -164,13 +155,23 @@
       print 'Cleaning', chrome_unzip_dir
       pyauto_utils.RemovePath(chrome_unzip_dir)
     print 'Unzipping'
-    pyauto_utils.UnzipFilenameToDir(chrome_zip, self._outdir)
+    try:
+      pyauto_utils.UnzipFilenameToDir(chrome_zip, self._outdir)
+    except(zipfile.BadZipfile, OSError), err:

[kkania, 2012/05/22 16:36:42]

why do you want to catch this exception? What do you plan to do on the client
side if this returns -1?

[nkang, 2012/05/24 17:00:13]

I added this exception handler and the one below, so we could handle these
situations gracefully and perhaps provide the user a little more insight into
the problem. This script is also used by manual testers to fetch Chrome builds
during testing. I've had situations where people were trying to run this script
and it was failing (either because of an invalid zip or access problems), but
they couldn't understand why it was failing. Going through the stack trace can
be a little daunting if a person doesn't know python, which is why I handled
these exceptions, so at least the user would have an idea why the script failed
and can take appropriate steps to correct the problem. Also, there are times
when one of the build numbers passed to the test is invalid, while the other one
is correct. In this situation, urllib.urlretrieve will not raise an exception,
so you'll end up with an empty zip file, which, if not handled, will cause the
test to stop prematurely. By handling these exceptions, we could at least run
the test with the one build that is valid; whether or not we would want to is
another question, but at least we have that option. If, however, you still feel
that this is unnecessary or outside the scope of this CL, I will gladly remove
the exception handlers.

+      print 'FetchPrebuilt.Run: %s' % (err)
+      return -1
     if get_remoting:
       pyauto_utils.UnzipFilenameToDir(remoting_zip, self._outdir)
-      shutil.move(self._outdir + '/remoting-webapp',
-                  self._outdir + '/remoting/remoting.webapp')
+      # Added this try/catch block to prevent a run-time error, which occurs

[kkania, 2012/05/22 16:36:42]

How does this class work in general if the outdir is already filled with files?
Does it just overwrite existing files? Does it throw an exception. Do the same
here.

[nkang, 2012/05/24 17:00:13]

See above response.

+      # if remoting-webapp directory already exists at the destination.
+      try:
+        shutil.move(self._outdir + '/remoting-webapp',
+                    self._outdir + '/remoting/remoting.webapp')
+      except shutil.Error, err:
+        print 'FetchPrebuilt.Run: %s' % (err)
+        return -1
 
-    # Copy over the binaries to outdir
+    # Copy over the binaries to destination directory.
     items_to_copy = {
       pyautolib_py: os.path.join(self._outdir, 'pyautolib.py'),
       pyautolib_so: os.path.join(self._outdir, self._pyautolib_so_name),
@@ -189,11 +190,11 @@
 
     # Final setup (if any)
     # Set executable bit on chromedriver binary.
-    if not self._options.platform == 'win':
+    if not self._platform == 'win':
       os.chmod(items_to_copy[chromedriver], 0700)
 
     # Create symlink to .framework on Mac
-    if self._options.platform == 'mac':
+    if self._platform == 'mac':
       mac_app_name = os.path.basename([x for x in unzip_dir_contents
                                        if x.endswith('.app')][0])
       os.chdir(self._outdir)
@@ -209,5 +210,46 @@
     return 0
 
 
-if __name__ == '__main__':
-  sys.exit(FetchPrebuilt().Run())
+class Main:
+
+  def __init__(self):
+    self._ParseArgs()
+    FetchPrebuilt(self._url, self._outdir, self._platform).Run()  
+
+  def _ParseArgs(self):
+    parser = optparse.OptionParser()
+    parser.add_option(
+        '-d', '--outdir', type='string', default=None,
+        help='Directory in which to setup. This is typically the directory '
+        'where the binaries would go when compiled from source.')
+    parser.add_option(
+        '-p', '--platform', type='string',
+        default=pyauto_utils.GetCurrentPlatform(),
+        help='Platform. Valid options: win, mac, linux32, linux64. '
+        'Default: current platform (%s)' % pyauto_utils.GetCurrentPlatform())
+    parser.add_option(
+        '-l', '--latest', action='store_true', default=False,
+        help='Download the latest chromium build from commondatastorage. '
+        '[default=False]')
+    self._options, self._args = parser.parse_args()

[kkania, 2012/05/22 16:36:42]

change these from member vars to local vars

+    if not self._options.outdir:
+      print >>sys.stderr, 'Need output directory: -d/--outdir'
+      sys.exit(1)
+    if self._options.latest:
+      self._url = FetchPrebuilt.GetLastestDownloadURL(self._options.platform)
+    elif not self._args:
+      print >>sys.stderr, 'Need download url'
+      sys.exit(2)
+    else:
+      self._url = self._args[0]
+
+    self._platform = self._options.platform
+    self._outdir = self._options.outdir
+    # Chromium continuous build archive has a non-standard format.
+    if 'index.html?path=' in self._url:
+      self._url = self._url.replace('index.html?path=', '')
+    self._url = self._url.rstrip('/')
+    
+
+if __name__ == '__main__':  
+  Main()