Chrome updater test framework

install_test/chrome_checkout.py

+#!/usr/bin/python

[kkania, 2012/05/30 15:41:09]

wrong shebang

[nkang, 2012/05/30 21:42:05]

Fixed.

+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import httplib
+import optparse
+import os
+import re
+import sys

[kkania, 2012/05/30 15:41:09]

order

[nkang, 2012/05/30 21:42:05]

I'm assuming you meant put them in alphabetical order, so I did.

+import socket
+import subprocess
+
+__author__ = 'laforge/nkang'
+
+class ChromeCheckout:

[kkania, 2012/05/30 15:41:09]

I don't think you need to throw everything into a class. I don't really see any
useful state that this class maintains. I would change it all to bare funcs.

[nkang, 2012/05/30 21:42:05]

They were all bare functions before, in a standalone script. I checked with you
before putting them in a class. You agreed that we should put all the methods in
a class and create a ChromeCheckout object in install_test to do checkouts for
both specified builds. As far as the 'useful state', I'm not entirely clear what
you mean. For the most part, I just took the standalone methods that Anthony
wrote and put them in a class. I tested Anthony's code prior to that, and it
worked just fine. That's why I didn't fiddle with it too much. If you want me to
change it so there's more of a synergy or cohesiveness between the class and its
methods, I can do that. But I feel that we should leave everything in a class.
If I had to put this back to bare functions, it would be like starting all over
again, not to mention the changes I'd have to make to install_test.py as well. I
cannot use Anthony's original script either, as it contains a lot of style
violations, code redundancy, and lacks certain things, which is why I decided to
change it in the first place.

+  """A class to check out Chrome source files based on revision number."""
+  
+  def __init__(self, version, dest=None):

[kkania, 2012/05/30 15:41:09]

add a docstring for this. As a user of this class, I don't understand what
version this accepts (source or release version?). Also, if I give dest=None
what does that do?

[nkang, 2012/05/30 21:42:05]

Added a docstring and a brief description for each of the parameters.

+    # Verify that he version has the correct format.

[kkania, 2012/05/30 15:41:09]

the

[nkang, 2012/05/30 21:42:05]

Done.

+    self._version = ((lambda l: str(l[0]) if len(l) > 0 else '')

[kkania, 2012/05/30 15:41:09]

this is hard to read what is going on

[nkang, 2012/05/30 21:42:05]

It checks to make sure the version specified has the correct format. First
lambda function takes a list as an argument, which is provided by the second
lambda function. The first function returns a string version of the first list
element, if the list isn't empty, otherwise returns an empty string. The second
lambda function tries to find the 'n.n.n.n' pattern in the version number
specified by the user and returns a list containing the first match if
successful, otherwise an empty list. This list is what gets passed onto the
first lambda function.

+                     ((lambda v: re.findall('\S+\.\S+\.\S+\.\S+', v)
+                       if(type(v) == str) else [])(version)))                      
+    self._dest = dest
+    self._base_svn_url = 'svn://svn.chromium.org/chrome'
+    self._selenium_url = 'http://selenium.googlecode.com/svn/trunk/py'
+    self._pyftpdlib_url = 'http://pyftpdlib.googlecode.com/svn/trunk'
+    self._deps_cache = {}
+    self._GetPath = (lambda par, ch: os.path.join(par, ch) if par else ch)   
+    # By default the destination is the CWD, but if a different directory was
+    # specified, make sure it exists. If not, create it.
+    if self._dest:
+      if not os.path.isdir(self._dest):
+        try:
+          os.mkdir(self._dest)
+        # Failed to create test directory, CWD will be used for checkout.
+        except(OSError, IOError), err:
+          print 'ChromeCheckout.__init__: Failed to create %s. Checkout '\

[kkania, 2012/05/30 15:41:09]

I don't like ignoring the requested dir if we can't seem to create it. I'd just
let it throw the exception in mkdir. I'm not sure how you are using this class,
but do we need to delete the dir if it already exists?

[nkang, 2012/05/30 21:42:05]

We decided against deletion, as the existing directory could contain other data
that the user needs. So for now I will just raise a RuntimeException if the
destination directory does not exist and we are unable to create it.

+                'will be done in the current working directory.' % self._dest
+          self._dest = None
+
+  def _GetContentAndReturnResponse(self, server, path, content):
+    """Gets contents of DEPS, which contains info about source files needed.    

[kkania, 2012/05/30 15:41:09]

This function doesn't have anything to do with DEPS, its just a helper method
that issues a GET request

[nkang, 2012/05/30 21:42:05]

Actually it does have something to do with DEPS. While the method does issue a
GET request, subsequently it also calls HttpConnection.getresponse() to get a
response from the server, which returns a HttpResponse object. It then calls the
HttpResponse object's 'read' method to get the response body, which contains the
contents of the DEPS file. The contents are read into a string and the method
returns that string to the caller. 

+
+    Args:
+      server: Host address, in this case the SVN server.
+      path: The URL where the file is located.
+      content: A string of data to send after the headers are finished. By

[kkania, 2012/05/30 15:41:09]

Get rid of this arg. Nobody that calls this function uses it.

[nkang, 2012/05/30 21:42:05]

It was used by the 'request' method. I think that's why Anthony added it. But we
can pass an empty string directly to the 'request' method, rather than adding an
extra argument. Got rid of the extra argument, so now the method only takes two
arguments: server and path.

+               default its an empty string and should be left that way.
+
+    Returns:
+      A string containing the file's contents if successful, otherwise None.
+    """
+    try:
+      conn = httplib.HTTPConnection(server)
+      headers = {"Content-type": "text/html"}

[kkania, 2012/05/30 15:41:09]

use single quotes

[nkang, 2012/05/30 21:42:05]

Removed all double quotes and replaced them with single quotes, where
applicable.

+      conn.request('GET', path, content, headers)
+      response = conn.getresponse()
+    except socket.gaierror, err:
+      print 'ChromeCheckout._GetContentAndReturnResponse: %s' % err
+      conn.close()
+      return None
+    if response.status != 200:

[kkania, 2012/05/30 15:41:09]

don't you need to conn.close() here

[nkang, 2012/05/30 21:42:05]

Done.

+      return None
+    data = response.read()
+    buff = ''
+    for line in data:
+      buff += line

[kkania, 2012/05/30 15:41:09]

i'm not sure I understand the point of lines 66-68. What's the difference
between buff and data?

[nkang, 2012/05/30 21:42:05]

Theoretically they should be the same, but that's how Anthony wrote it, so I
just left it like that. I suppose we could just return data. Updated code to
reflect this change.

+    conn.close()
+    return buff
+
+  def _GetDeps(self, version):
+    """Returns cached version of DEPS if possible, otherwise gets its contents.    

[kkania, 2012/05/30 15:41:09]

line > 80

[nkang, 2012/05/30 21:42:05]

It was just a couple of spaces, that's why I missed it. The actual text ends at
line 79, but I moved part of the comment to the next line just to be safe.

+
+    Args:
+      version: Chrome version number (e.g. - 21.0.1136.0).

[kkania, 2012/05/30 15:41:09]

e.g., 21.0.1136.0

[nkang, 2012/05/30 21:42:05]

Done.

+
+    Returns:
+      string: A string buffer containing the contents of the DEPS file.

[kkania, 2012/05/30 15:41:09]

A string containing the contents of the DEPS file

[nkang, 2012/05/30 21:42:05]

Done.

+    """
+    if not version:
+      return None

[kkania, 2012/05/30 15:41:09]

I'd just remove the check from 81-82

[nkang, 2012/05/30 21:42:05]

This is also something that Anthony had added, so I just left it in. Got rid of
it now.

+    if version in self._deps_cache:
+      return self._deps_cache[version]
+    deps = self._GetContentAndReturnResponse('src.chromium.org',
+                                             '/viewvc/chrome/releases/%s/DEPS'
+                                             % version,  '')
+    self._deps_cache[version] = deps
+    return deps
+
+  def _ParseVersion(self, version):
+    """Parses the version number to get the different identifiers."""
+    match = re.search(r'((\d+)\.(\d+)\.(\d+)\.(\d+))', version)
+    if match:
+      version = {'version': match.group(1),
+                 'major': int(match.group(2)),
+                 'minor': int(match.group(3)),
+                 'build': int(match.group(4)),
+                 'patch': int(match.group(5))}
+      return version
+    return {}
+
+  def _GetRevisionInfo(self, version_str):
+    """Gets the revision info by parsing the contents of the DEPS file.
+
+    Args:
+      version_str: A string representing the Chrome version number.
+
+    Returns:
+      A string that contains pertinent information about the Chrome version.
+    """
+    version = self._ParseVersion(version_str)
+    if version:
+      body = self._GetDeps(version['version'])
+      if body:
+        match = re.search("'src':[\n\r ]+'(.*?)'", body)
+        if match:
+          match = re.search(r"@(\d+)", match.group(1))
+          if match:
+            version['revision'] = int(match.group(1))
+
+        match = re.search("""['"]src['"].*?:.*?['"]/branches/(.*?)/.*?,""",
+                          body, re.DOTALL | re.IGNORECASE)
+        if match:
+          version['branch'] = match.group(1)
+        else:
+          version['branch'] = 'trunk'
+    return version
+
+  def _GetRevision(self, version_str, rev_type='selenium'):
+    """Gets selenium/pyftpdlib rev. number by parsing contents of DEPS file.    
+
+    Args:
+      version_str: A string representing the Chrome build number.
+      rev_type: Type of revision number to look up - Selenium or Pyftpdlib.

[kkania, 2012/05/30 15:41:09]

Type of revision number to look up: 'selenium' or 'pyftpdlib'

[nkang, 2012/05/30 21:42:05]

Done.

+
+    Returns:
+      A string representing the revision no. if successful, otherwise None.

[kkania, 2012/05/30 15:41:09]

this isn't correct

[nkang, 2012/05/30 21:42:05]

Sorry I missed the typecast to int. It is initially a string (re.search(...),
which is changed to integer before the function returns. Went ahead and updated
the description to reflect this change.

+    """

[kkania, 2012/05/30 15:41:09]

assert that rev_type is either selenium or pyftpdlib

[nkang, 2012/05/30 21:42:05]

Done.

+    version = self._ParseVersion(version_str)
+    if version:
+      body = self._GetDeps(version_str)
+      if body:
+        if rev_type == 'selenium':
+          m = re.search(r"http://selenium\.googlecode\.com/svn/trunk/py@(\d+)",
+                        body, re.DOTALL | re.IGNORECASE | re.MULTILINE)
+        elif rev_type == 'pyftpdlib':
+          m = re.search(r"http://pyftpdlib\.googlecode\.com/svn/trunk@(\d+)",
+                        body, re.DOTALL | re.IGNORECASE | re.MULTILINE)
+        if m:
+          return int(m.group(1))
+    return None
+
+  def _SvnCo(self, path, revision=None, dest=None):
+    """Does a SVN checkout on specified source files.
+
+    Args:
+      path: URL that is to be checked out.
+      revision: Revision number.
+      dest: Destination where the data will be downloaded.
+    """
+    cmd = "svn co"

[kkania, 2012/05/30 15:41:09]

use single quotes, not double, here and everywhere in this file

[nkang, 2012/05/30 21:42:05]

Done.

+    if revision:
+      cmd += " --revision %d" % revision        
+    cmd += " %s" % path
+    if dest:
+      cmd += " %s" % dest
+    print cmd    
+    return subprocess.Popen.wait(subprocess.Popen(cmd, shell=True))
+
+  def CheckOut(self):
+    """Checks out all necessary source files."""
+    # No version was specified.
+    if not self._version:
+      print 'Please specify a Chrome version to checkout.'
+      return -1
+    rev_info = self._GetRevisionInfo(self._version)
+    print rev_info
+    # If its a patch, check out the branch.
+    if rev_info['patch']:
+      svn_url_base = self._base_svn_url + '/branches/%s' % rev_info['branch']
+    # If not, check out the trunk.
+    else:
+      svn_url_base = self._base_svn_url + '/trunk'
+    self._SvnCo('%s/src/chrome/test/functional' % svn_url_base, 
+                rev_info['revision'], self._GetPath(self._dest, 'functional'))
+    self._SvnCo('%s/src/chrome/test/pyautolib' % svn_url_base,
+                rev_info['revision'], self._GetPath(self._dest, 'pyautolib'))
+    self._SvnCo('%s/src/third_party/simplejson' % svn_url_base,
+                rev_info['revision'], self._GetPath(self._dest, 'simplejson'))
+    self._SvnCo('%s/src/third_party/tlslite' % svn_url_base,
+                rev_info['revision'], self._GetPath(self._dest, 'tlslite'))
+    self._SvnCo('%s/src/net/tools/testserver' % svn_url_base,
+                rev_info['revision'], self._GetPath(self._dest, 'testserver'))    
+    self._SvnCo(self._selenium_url, self._GetRevision(
+      self._version, 'selenium'), self._GetPath(self._dest, 'selenium'))   
+    self._SvnCo(self._pyftpdlib_url, self._GetRevision(
+      self._version, 'pyftpdlib'), self._GetPath(self._dest, 'pyftpdlib'))
+    return 0
+