Issue 10384104: Chrome updater test framework

nkang

Resolved all the conflicts in pyauto.py and fetch_prebuilt_pyauto.py.

8 years, 7 months ago (2012-05-10 20:37:51 UTC) #1

kkania

http://codereview.chromium.org/10384104/diff/1/pyautolib/py_unittest_util.py File pyautolib/py_unittest_util.py (right): http://codereview.chromium.org/10384104/diff/1/pyautolib/py_unittest_util.py#newcode52 pyautolib/py_unittest_util.py:52: """Test Runner for PyAuto tests that displays results in ...

8 years, 7 months ago (2012-05-22 16:36:42 UTC) #2

nkang

http://codereview.chromium.org/10384104/diff/2001/pyautolib/fetch_prebuilt_pyauto.py File pyautolib/fetch_prebuilt_pyauto.py (right): http://codereview.chromium.org/10384104/diff/2001/pyautolib/fetch_prebuilt_pyauto.py#newcode160 pyautolib/fetch_prebuilt_pyauto.py:160: except(zipfile.BadZipfile, OSError), err: On 2012/05/22 16:36:42, kkania wrote: > ...

8 years, 7 months ago (2012-05-24 17:00:13 UTC) #3

http://codereview.chromium.org/10384104/diff/2001/pyautolib/fetch_prebuilt_py...
File pyautolib/fetch_prebuilt_pyauto.py (right):

http://codereview.chromium.org/10384104/diff/2001/pyautolib/fetch_prebuilt_py...
pyautolib/fetch_prebuilt_pyauto.py:160: except(zipfile.BadZipfile, OSError),
err:
On 2012/05/22 16:36:42, kkania wrote:
> why do you want to catch this exception? What do you plan to do on the client
> side if this returns -1?

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.

http://codereview.chromium.org/10384104/diff/2001/pyautolib/fetch_prebuilt_py...
pyautolib/fetch_prebuilt_pyauto.py:165: # Added this try/catch block to prevent
a run-time error, which occurs
On 2012/05/22 16:36:42, kkania wrote:
> 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.

See above response.

kkania

http://codereview.chromium.org/10384104/diff/9001/pyautolib/fetch_prebuilt_pyauto.py File pyautolib/fetch_prebuilt_pyauto.py (right): http://codereview.chromium.org/10384104/diff/9001/pyautolib/fetch_prebuilt_pyauto.py#newcode47 pyautolib/fetch_prebuilt_pyauto.py:47: # Generate name of the zip file that will ...

8 years, 7 months ago (2012-05-25 00:50:20 UTC) #4

nkang

Made following changes to fetch_prebuilt_pyauto.py: - Got rid of unnecessary comments. - Changed the way ...

8 years, 7 months ago (2012-05-25 21:37:14 UTC) #5

kkania

https://chromiumcodereview.appspot.com/10384104/diff/16002/pyautolib/fetch_prebuilt_pyauto.py File pyautolib/fetch_prebuilt_pyauto.py (right): https://chromiumcodereview.appspot.com/10384104/diff/16002/pyautolib/fetch_prebuilt_pyauto.py#newcode124 pyautolib/fetch_prebuilt_pyauto.py:124: lst_dirs = ['PepperFlash', 'remoting', 'locales', 'resources', 'installer'] rename this ...

8 years, 6 months ago (2012-05-29 18:23:14 UTC) #6

nkang

See comments for list of changes. https://chromiumcodereview.appspot.com/10384104/diff/16002/pyautolib/fetch_prebuilt_pyauto.py File pyautolib/fetch_prebuilt_pyauto.py (right): https://chromiumcodereview.appspot.com/10384104/diff/16002/pyautolib/fetch_prebuilt_pyauto.py#newcode124 pyautolib/fetch_prebuilt_pyauto.py:124: lst_dirs = ['PepperFlash', ...

8 years, 6 months ago (2012-05-29 19:29:57 UTC) #7

kkania

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrome_checkout.py#newcode1 install_test/chrome_checkout.py:1: #!/usr/bin/python wrong shebang https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrome_checkout.py#newcode10 install_test/chrome_checkout.py:10: import sys order https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrome_checkout.py#newcode16 ...

8 years, 6 months ago (2012-05-30 15:41:09 UTC) #8

nkang

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrome_checkout.py#newcode1 install_test/chrome_checkout.py:1: #!/usr/bin/python On 2012/05/30 15:41:09, kkania wrote: > wrong shebang ...

8 years, 6 months ago (2012-05-30 21:42:05 UTC) #9

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
File install_test/chrome_checkout.py (right):

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:1: #!/usr/bin/python
On 2012/05/30 15:41:09, kkania wrote:
> wrong shebang

Fixed.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:10: import sys
On 2012/05/30 15:41:09, kkania wrote:
> order

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

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:16: class ChromeCheckout:
On 2012/05/30 15:41:09, kkania wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:19: def __init__(self, version, dest=None):
On 2012/05/30 15:41:09, kkania wrote:
> 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?

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

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:20: # Verify that he version has the correct
format.
On 2012/05/30 15:41:09, kkania wrote:
> the

Done.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:21: self._version = ((lambda l: str(l[0]) if
len(l) > 0 else '')
On 2012/05/30 15:41:09, kkania wrote:
> this is hard to read what is going on

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:38: print 'ChromeCheckout.__init__: Failed to
create %s. Checkout '\
On 2012/05/30 15:41:09, kkania wrote:
> 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?

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:43: """Gets contents of DEPS, which contains
info about source files needed.
On 2012/05/30 15:41:09, kkania wrote:
> This function doesn't have anything to do with DEPS, its just a helper method
> that issues a GET request

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:48: content: A string of data to send after the
headers are finished. By
On 2012/05/30 15:41:09, kkania wrote:
> Get rid of this arg. Nobody that calls this function uses it.

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:56: headers = {"Content-type": "text/html"}
On 2012/05/30 15:41:09, kkania wrote:
> use single quotes

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

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:63: if response.status != 200:
On 2012/05/30 15:41:09, kkania wrote:
> don't you need to conn.close() here

Done.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:68: buff += line
On 2012/05/30 15:41:09, kkania wrote:
> i'm not sure I understand the point of lines 66-68. What's the difference
> between buff and data?

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:73: """Returns cached version of DEPS if
possible, otherwise gets its contents.
On 2012/05/30 15:41:09, kkania wrote:
> line > 80

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:76: version: Chrome version number (e.g. -
21.0.1136.0).
On 2012/05/30 15:41:09, kkania wrote:
> e.g., 21.0.1136.0

Done.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:79: string: A string buffer containing the
contents of the DEPS file.
On 2012/05/30 15:41:09, kkania wrote:
> A string containing the contents of the DEPS file

Done.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:82: return None
On 2012/05/30 15:41:09, kkania wrote:
> I'd just remove the check from 81-82

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

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:135: rev_type: Type of revision number to look
up - Selenium or Pyftpdlib.
On 2012/05/30 15:41:09, kkania wrote:
> Type of revision number to look up: 'selenium' or 'pyftpdlib'

Done.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:138: A string representing the revision no. if
successful, otherwise None.
On 2012/05/30 15:41:09, kkania wrote:
> this isn't correct

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.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:139: """
On 2012/05/30 15:41:09, kkania wrote:
> assert that rev_type is either selenium or pyftpdlib

Done.

https://chromiumcodereview.appspot.com/10384104/diff/18001/install_test/chrom...
install_test/chrome_checkout.py:162: cmd = "svn co"
On 2012/05/30 15:41:09, kkania wrote:
> use single quotes, not double, here and everywhere in this file

Done.

nkang

Modified chrome_checkout.py. Got rid of ChromeCheckout class and put all the methods from ChromeCheckout at ...

8 years, 6 months ago (2012-05-31 23:57:18 UTC) #10

kkania

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checkout.py#newcode5 install_test/chrome_checkout.py:5: have a description of this module http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checkout.py#newcode19 install_test/chrome_checkout.py:19: def ...

8 years, 6 months ago (2012-06-09 00:06:10 UTC) #11

nkang

Made all the changes that were suggested below. http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checkout.py#newcode5 install_test/chrome_checkout.py:5: On ...

8 years, 6 months ago (2012-06-12 23:48:33 UTC) #12

Made all the changes that were suggested below.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
File install_test/chrome_checkout.py (right):

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:5: 
On 2012/06/09 00:06:10, kkania wrote:
> have a description of this module

Added a brief description.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:19: def _GetContentAndReturnResponse(server,
path):
On 2012/06/09 00:06:10, kkania wrote:
> two newlines between module-level definitions, in several places in this file
> and in other files too:
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines

Added two blank lines, where needed.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:20: """Returns contents of DEPS file, which
contains info about source files
On 2012/06/09 00:06:10, kkania wrote:
> This func may be involved with the process of getting the DEPS file, but it is
> generic and is not tied to DEPS file at all. I could use it to fetch the
latest
> news feed. Change this comment to reflect the function.
> 
> Also, the first line of doc should take no more than 1 line.  See
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

I don't dispute that you could get the latest news feed using this method. It's
just that in your last response you said that 'its just a helper method that
issues a GET request'. So I thought you were trying to imply that it only issued
a GET request and nothing else, and that's clearly not the case; that's all I
was trying to say. I see where you're coming from, though, so I went ahead and
changed the docstring to the following, 'Issues a GET requst to server and
returns the response body'.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:37: print
'ChromeCheckout._GetContentAndReturnResponse: %s' % err
On 2012/06/09 00:06:10, kkania wrote:
> either
> 1) catch this exception, do conn.close() and raise the exception again (drop
the
> print) or 
> 2) just don't catch the exception at all and wait for python to close the
socket
> automatically
> 
> I'm fine with either choice.

Got rid of the print statement. I now catch socket.gaierror, close the
connection, and raise the exception again.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:42: return None
On 2012/06/09 00:06:10, kkania wrote:
> throw an exception here instead of returning None, and change the Returns:
> comment

RuntimeError is now raised if status code in not 200. Also, updated the
'Returns' comment to reflect this change.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:56: if version in _DEPS_CACHE:
On 2012/06/09 00:06:10, kkania wrote:
> get rid of the cache, and call getdeps in checkout and pass DEPS to the two
> funcs that need it

Updated _GetDeps and got rid of _DEPS_CACHE. _GetDeps is now called in the
CheckOut method. The value returned by _GetDeps is then passed onto _GetRevision
and _GetRevisionInfo. Also updated _GetRevision and _GetRevisionInfo, as they do
not call _GetDeps directly now, instead the contents of DEPS are now passed to
them in the form of an argument.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:60: % version)
On 2012/06/09 00:06:10, kkania wrote:
> This would be more readable:
> deps = _GetContentAndReturnResponse(
>     'src.chromium.org',
>     '/viewvc/chrome/releases/%s/DEPS' % version)

Done and done!

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:74: return {}
On 2012/06/09 00:06:10, kkania wrote:
> throw

Instead of returning an empty dictionary, the method now raises a RuntimeError
exception, if it cannot find a match in the version number that was passed.
Also, since we're throwing an exception now, I also updated the two methods that
use _ParseVersion: _GetRevisionInfo and _GetRevision. Removed the 'if' statement
that confirmed that value returned by _ParseVersion wasn't empty from both of
the aforementioned methods.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:126: return None
On 2012/06/09 00:06:10, kkania wrote:
> throw and change doc

This method now raises a RuntimeError, instead of returning None. Also updated
the docstring to reflect this change.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:142: print cmd
On 2012/06/09 00:06:10, kkania wrote:
> use logging module or drop

I have replaced all the print statements with logging. However, the 'logging'
information page on the Python website states that to 'display console output
for ordinary usage of a command line script or program', the best tool for the
job is the 'print' statement. I think 'logging' is good for more complex
scripts, where one could have several different types of messages, warnings,
errors, or general info, etc. In this script, however, we only have two print
statements. I think 'logging' might be a bit of an overkill for our situation.
What do you think? I've already applied the changes. If you agree, I'll revert
the changes. If not, we can leave it as is. Let me know.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:143: return
subprocess.Popen.wait(subprocess.Popen(cmd, shell=True))
On 2012/06/09 00:06:10, kkania wrote:
> can you just do subprocess.Popen(cmd, shell=True).wait() instead?

Done.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:149: return ((lambda v: True if(type(v) == str
and re.findall
On 2012/06/09 00:06:10, kkania wrote:
> Make this more readable.

Very readable now.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:153: """Checks out all necessary source
files."""
On 2012/06/09 00:06:10, kkania wrote:
> Args:

Added description about 'Args'.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:155: print 'Invalid version no. was specified.'
On 2012/06/09 00:06:10, kkania wrote:
> throw instead

Thrown.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:163: print rev_info
On 2012/06/09 00:06:10, kkania wrote:
> take a look at the logging module instead, or just drop this

See response I left on the logging comment above.

http://codereview.chromium.org/10384104/diff/21002/install_test/chrome_checko...
install_test/chrome_checkout.py:184: return 0
On 2012/06/09 00:06:10, kkania wrote:
> don't return anything

The return has been returned. Also updated install_test to reflect this change.

kkania

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checkout.py#newcode38 install_test/chrome_checkout.py:38: server: Host address, in this case the SVN server. ...

8 years, 6 months ago (2012-06-13 16:47:01 UTC) #13

nkang

Made all the changes suggested below. http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checkout.py#newcode38 install_test/chrome_checkout.py:38: server: Host address, ...

8 years, 6 months ago (2012-06-13 23:35:09 UTC) #14

Made all the changes suggested below.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
File install_test/chrome_checkout.py (right):

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:38: server: Host address, in this case the SVN
server.
On 2012/06/13 16:47:01, kkania wrote:
> drop 'in this case the SVN server'

Dropped it like it was hot.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:39: path: The URL where the file is located.
On 2012/06/13 16:47:01, kkania wrote:
> path != URL

Replaced URL with path, so it now states, 'The path where the file is located'.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:51: raise socket.gaierror(err)
On 2012/06/13 16:47:01, kkania wrote:
> just do 'raise'

Done.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:58: assert(data)
On 2012/06/13 16:47:01, kkania wrote:
> drop this

Dropped it like a bad habit.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:74: def _ParseVersion(version):
On 2012/06/13 16:47:01, kkania wrote:
> version->version_str

Changed argument name from version to version_str. Updated the docstring to
reflect this change.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:91: def _GetRevisionInfo(version_str, body):
On 2012/06/13 16:47:01, kkania wrote:
> i'd change var name body to deps

Changed argument name from 'body' to 'deps'. Updated the docstring to reflect
this change.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:117: def _GetRevision(version_str, body,
rev_type='selenium'):
On 2012/06/13 16:47:01, kkania wrote:
> body->deps

Done. Also updated the docstring to reflect this change.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:129: version = _ParseVersion(version_str)
On 2012/06/13 16:47:01, kkania wrote:
> this doesn't look like it serves a purpose; remove the version_str arg

Nice catch! This function and the one above it are somewhat similar, which is
why, I'm assuming, Anthony had the version_str argument for both. I didn't catch
it either. Anyhow, its fixed now. Got rid of the version_str argument, removed
the superfluous line, updated the docstring, and also updated the CheckOut
method, which calls _GetRevision on two occasions, to reflect this change.

http://codereview.chromium.org/10384104/diff/23001/install_test/chrome_checko...
install_test/chrome_checkout.py:201: rev_info['revision'], _GetPath(dest,
'functional'))
On 2012/06/13 16:47:01, kkania wrote:
> doesn't this need to be checked out under <dest>/src/chrome/test/functional
> instead of <dest>/functional? Also, get rid of uses of _GetPath and remove
that
> function. 'dest' shouldn't be None.

Got rid of the _GetPath method. Also updated all the paths so correct folder
hierarchies are created during checkout.

kkania

http://codereview.chromium.org/10384104/diff/30001/functional/protector_updater.py File functional/protector_updater.py (right): http://codereview.chromium.org/10384104/diff/30001/functional/protector_updater.py#newcode16 functional/protector_updater.py:16: $ python Chrome-Protector.py --url=http://chrome-master2.mtv.corp.google. \ this is out of ...

8 years, 6 months ago (2012-06-14 17:20:33 UTC) #15

nkang

Several changes were made. The changes are listed below: - Updated InstallTest so it existing ...

8 years, 5 months ago (2012-06-28 19:02:39 UTC) #16

Several changes were made. The changes are listed below:
- Updated InstallTest so it existing Chrome build is deleted before the test
begins.
- Created a new Main class in install_test.py. The Main class is somewhat
similar to the Pyauto's Main class, though it does not offer as much granularity
in how the tests can be run.
- Command parsing was moved out of InstallTest and moved into Main.
- Updated ProtectorUpdater so it no longer loads the unittests or executes them.
That task is performed by Main. ProtectorUpdater simply calls
InstallTest.Main(), a la Pyauto.
- Changed how InstallTest keeps track of builds by adding iterators instead of a
counter.
- Wrote new _SetUp method for InstallTest. This method is somewhat similar in
functionality to the PyUITest.__SetUp method. It validates test parameters,
among other things.
- chrome_installer.py was completely gutted. ChromeInstaller class is no more.
All of its utility methods were moved to a new class called Installation, which
will provide information about the installed version of Chrome. UninstallChrome
was also moved out of ChromeInstaller and moved into Installation. InstallChrome
was renamed to Install and is now a modular level method. Install now returns an
"Installation" object on successful installation of Chrome. Updated InstallTest
to reflect these changes.
- Chrome installer is also not downloaded by ChromeInstaller anymore. This is
now done by InstallTest. InstallTest downloads installers for both specified
builds at the beginning of the test and then passes the paths to
chrome_installer.Install to do the installation. 
- Created an enumerator that specifies the different types of Chrome
installation, i.e., user or system. Also install_test.py and chrome_installer.py
so they use the enumerator to identify the installation type, instead of string
variables.
- Made other minor changes that were recommended.

http://codereview.chromium.org/10384104/diff/30001/functional/protector_updat...
File functional/protector_updater.py (right):

http://codereview.chromium.org/10384104/diff/30001/functional/protector_updat...
functional/protector_updater.py:16: $ python Chrome-Protector.py
--url=http://chrome-master2.mtv.corp.google. \
On 2012/06/14 17:20:33, kkania wrote:
> this is out of date

Fixed.

http://codereview.chromium.org/10384104/diff/30001/functional/protector_updat...
functional/protector_updater.py:17: com/official_builds/
--builds=19.0.1069.0,190.0.1070.0
On 2012/06/14 17:20:33, kkania wrote:
> 190!

It was supposed to be 19. Just making sure you were paying attention Ken :)
Fixed now.

http://codereview.chromium.org/10384104/diff/30001/functional/protector_updat...
functional/protector_updater.py:36: """Sample Protector test"""
On 2012/06/14 17:20:33, kkania wrote:
> Comments on their own line are always supposed to have periods at the end, i
> think

Yes they are and so shall this one ;)

http://codereview.chromium.org/10384104/diff/30001/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/30001/install_test/chrome_instal...
install_test/chrome_installer.py:30: def __init__(self, url, build, dest,
opts='', clean=True):
On 2012/06/14 17:20:33, kkania wrote:
> we should make the installer not know about the URL. Someone else should
> download the build/installer/etc and just give the path to the installer here.
> I'd move the 'opts' and 'clean' arguments to the InstallChrome method.

Done. ChromeInstaller is no more, which makes Install a standalone method.
Install does not download the installer anymore. The installer will now be
downloaded by InstallTest and it will pass the path of the installer to the
'Install' method.

http://codereview.chromium.org/10384104/diff/30001/install_test/chrome_instal...
install_test/chrome_installer.py:39: self.SetBuild(build)
On 2012/06/14 17:20:33, kkania wrote:
> i don't like SetBuild. Remove it and GetBuild. This class should just
represent
> the installer, and should only provide an Install method

SetBuild and GetBuild are gone. In fact, so is the whole ChromeInstaller class.

http://codereview.chromium.org/10384104/diff/30001/install_test/chrome_instal...
install_test/chrome_installer.py:179: def InstallChrome(self):
On 2012/06/14 17:20:33, kkania wrote:
> change this name to just Install, and return an instance of ChromeVersion
(which
> in a later comment I say to rename to Installation)

Changed name to install. Although, since ChromeInstaller no longer exists, all
of its methods were moved to modular level. So Install now is not a member of
ChromeInstaller; it is just a standalone method.

http://codereview.chromium.org/10384104/diff/30001/install_test/chrome_instal...
install_test/chrome_installer.py:220: class ChromeVersion():
On 2012/06/14 17:20:33, kkania wrote:
> Change this class name from ChromeVersion to just Install/Installation, move
the
> UninstallChrome method from the above class to here.

Changed class name to Installation and moved the Uninstall method out of
ChromeInstaller and into Installation.

http://codereview.chromium.org/10384104/diff/30001/install_test/chrome_instal...
install_test/chrome_installer.py:226: 
On 2012/06/14 17:20:33, kkania wrote:
> add a static method GetInstallations() which returns the list of all found
> installations on the system

I am assuming you mean user and system level installations. If so, then this
method wouldn't make sense since user and system level versions aren't allowed.
If you already have a user level version and you install a system level version
on top of it, the user level version will automatically be deleted. If, however,
the current version is system level and you're trying to install a user level
version on top of it, it will not be allowed. If you mean different versions of
Chrome, that wouldn't work either since multiple version cannot reside on the
same system, unless one of the builds is a Canary build. So, since Ken is on
vacation, I am going to defer this for now, just in case he meant something
else.

http://codereview.chromium.org/10384104/diff/30001/install_test/chrome_instal...
install_test/chrome_installer.py:228: self._type = install_type
On 2012/06/14 17:20:33, kkania wrote:
> is this just a string. change it to a simple enum. i think something like this
> would be good:
> 
> class InstallationType:
>   SYSTEM = 0
>   USER = 1

Done.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test.py
File install_test/install_test.py (right):

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:17: 
On 2012/06/14 17:20:33, kkania wrote:
> no newline here, i think; check the style guide

It's gone.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:20: os.sys.path.append(os.path.join(os.path.pardir,
'pyautolib'))
On 2012/06/14 17:20:33, kkania wrote:
> os.sys? I thought it was just sys

They are the same, but in order to use just sys, you have to explicitly import
sys. I didn't want to do an additional import just so I could use the shorter
version. That's why I decided to use os.sys, since os was already imported. But
I didn't realize that sys had also been imported, albeit for some other purpose.
So I went ahead and changed all os.sys.path to just sys.path.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:62: def _ParseArgs(self):
On 2012/06/14 17:20:33, kkania wrote:
> move the global initialization stuff out to a main method

Done. Although, I hope you don't mind - since you said that you would be okay
with either a method or a class - that I created a class called Main, which does
the command parsing, loads all the unit-tests, and runs them, just like Pyauto.
Child classes, like ProtectorUpdater, can simply call install_test.Main() to run
all the tests. At this point, per your request, we only have one option, which
is to run all the tests in a suite. Later on, as our test suite grows, we can
add more granularity and provide options to run tests by class name or
individual tests.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:81: self._bld_counter = (lambda lst: 0 if len(lst)
> 0 else None)(self._builds)
On 2012/06/14 17:20:33, kkania wrote:
> you use a bit too many abbreviations ('opts', 'bld', 'c_installer', ...),
which
> the style guide tries to discourage. Use abbreviations sparingly only when the
> abbreviation is standard (e.g., 'args')

Got rid of abbreviations wherever possible.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:101: """
On 2012/06/14 17:20:33, kkania wrote:
> i think we should uninstall first to make sure the system is clean

Done, now if Chrome is already installed, we uninstall it before proceeding with
the test. Although, doing this in 'setUp' might not be a good idea, since we
only need to do this once not for every test case. So I put this code in the
constructor. There's code in the constructor that only gets executed once during
the first instance (code that downloads the builds and checks out the source
files). I put the uninstall code in the same code block. If its the first
instance, we check to make sure if there's already a version of Chrome installed
on the system, and if so, we uninstall it before the test begins.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:118: def SetCurrentBuild(self, nVal):
On 2012/06/14 17:20:33, kkania wrote:
> check these, i don't think they're used

Removed both of them.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:134: os.sys.modules.pop('pyauto')
On 2012/06/14 17:20:33, kkania wrote:
> i thought it was just sys.modules

You can either call sys.modules or os.sys.modules, same thing. The only
difference is, in order to use sys.modules, you have to import sys. I didn't
want to do an additional import just for this; os was already being imported, so
I used os.sys.modules instead. But I've since discovered that sys is being
imported, for some other purpose, anyway. So I went ahead and changed all the
statements from os.sys.modules to simply sys.modules.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:138: print 'CBaseUpdater._Refresh: ', err
On 2012/06/14 17:20:33, kkania wrote:
> when could this occur? I think we should just let it throw instead of catching
> and printing or we should check that each one is actually loaded before
popping

This scenario, though unlikely, could occur if there's an error during update.
The '_Refresh' method is called twice: once before the update and then in the
'tearDown' method. After the initial installation is complete, we have to clear
the modules registry before we update; that way we remove any references to the
modules from the previous version. Then after the upgrade is done, 'tearDown' is
called on completion, which clears the modules registry again so the next test
case can re-import the appropriate version of pyauto and pyautolib files.
Theoretically, this exception could occur if something goes wrong during the
upgrade. If an error occurs in upgrade after  '_Refresh' is called, pyauto,
pyautolib, and _pyautolib will be deleted from the modules registry, but newer
versions of pyautolib files will not get added to the registry, as a result of
the failure. So when '_Refresh' gets called again in the tearDown method, it
could invoke a 'KeyError'. Having said that, I agree with you that it would be
better to check if the keys exist, before we attempt to 'pop' them, which is
what I am doing in other parts of the code; I don't know why I didn't do the
same thing here. In either case, I went ahead and made this change. Now we only
'pop' if the aforementioned modules exist in the modules registry. I think its
better this way rather than throwing.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:160: if _DOWNLOAD_DIR[1] in os.sys.path:
On 2012/06/14 17:20:33, kkania wrote:
> same here and many places in this file; i think all these could be simplified
by
> dropping the os.

Done.

http://codereview.chromium.org/10384104/diff/30001/install_test/install_test....
install_test/install_test.py:179: print 'No more builds left to install. The
following builds have '\
On 2012/06/14 17:20:33, kkania wrote:
> raise, not print

Done.

Nirnimesh

Please make the CL title and description a bit clearer. State in one line what ...

8 years, 5 months ago (2012-07-03 23:29:20 UTC) #17

Nirnimesh

http://codereview.chromium.org/10384104/diff/44001/install_test/py_unittest_util.py File install_test/py_unittest_util.py (right): http://codereview.chromium.org/10384104/diff/44001/install_test/py_unittest_util.py#newcode1 install_test/py_unittest_util.py:1: #!/usr/bin/env python Move all of this to chrome/test/pyautolib/pyauto_utils.py http://codereview.chromium.org/10384104/diff/44001/pyautolib/fetch_prebuilt_pyauto.py ...

8 years, 5 months ago (2012-07-03 23:37:02 UTC) #18

Nirnimesh

I haven't finished reviewing entirely, but here are a few more comments. http://codereview.chromium.org/10384104/diff/44001/functional/protector_updater.py File functional/protector_updater.py ...

8 years, 5 months ago (2012-07-04 00:39:19 UTC) #19

nkang

Made all the changes recommended by Nirnimesh. Modules impacted by these changes include install_test.py, protector_updater.py, ...

8 years, 5 months ago (2012-07-13 23:50:50 UTC) #20

Made all the changes recommended by Nirnimesh. Modules impacted by these changes
include install_test.py, protector_updater.py, fetch_prebuilt_pyauto.py,
pyauto_utils.py, pyauto.py, py_unittest_util.py, and chrome_checkout.py.

http://codereview.chromium.org/10384104/diff/44001/functional/protector_updat...
File functional/protector_updater.py (right):

http://codereview.chromium.org/10384104/diff/44001/functional/protector_updat...
functional/protector_updater.py:1: #!/usr/bin/env python
On 2012/07/04 00:39:19, Nirnimesh wrote:
> It should go in install_test dir, not functional

Moved protector_updater.py to install_test.

http://codereview.chromium.org/10384104/diff/44001/functional/protector_updat...
functional/protector_updater.py:6: """protector_updater.py: Runs Protector
updater tests using two Chrome builds.
On 2012/07/04 00:39:19, Nirnimesh wrote:
> Remove file name.

It's gone!

http://codereview.chromium.org/10384104/diff/44001/functional/protector_updat...
functional/protector_updater.py:16: $ python Chrome-Protector.py
--url=http://master.chrome.corp.google.com/ \
On 2012/07/04 00:39:19, Nirnimesh wrote:
> do not include internal URLs

Removed the internal URL.

http://codereview.chromium.org/10384104/diff/44001/functional/protector_updat...
functional/protector_updater.py:33: """Class to run Protector Updater tests."""
On 2012/07/04 00:39:19, Nirnimesh wrote:
> Update tests for protector

Changed docstring to 'Update tests for Protector'.

http://codereview.chromium.org/10384104/diff/44001/functional/protector_updat...
functional/protector_updater.py:35: def __init__(self, methodName='runTest'):
On 2012/07/04 00:39:19, Nirnimesh wrote:
> why override __init__ if you're going to do the default anyway?

This is just a sample test that I created for illustration purposes. I wasn't
sure what a real Protector updater test would entail, as someone else is going
to write that. I added the __init__ method, so if there's anything that needs to
be initialized or any test related parameters that need to be set, it can be
done here. But I guess if its not being used at the moment, it would make sense
to just remove it. So its gone.

http://codereview.chromium.org/10384104/diff/44001/install_test/chrome_checko...
File install_test/chrome_checkout.py (right):

http://codereview.chromium.org/10384104/diff/44001/install_test/chrome_checko...
install_test/chrome_checkout.py:6: """Checks out Chrome source files from SVN.
On 2012/07/04 00:39:19, Nirnimesh wrote:
> Checks out -> Utilities for checking out

Changed to "Utilities for checking out Chrome source files from SVN".

http://codereview.chromium.org/10384104/diff/44001/install_test/chrome_checko...
install_test/chrome_checkout.py:54: raise RuntimeError('HTTP request returned
the following status code: %d' %
On 2012/07/04 00:39:19, Nirnimesh wrote:
> include (server, path) in the message too

Server and path are now included in the error message.

http://codereview.chromium.org/10384104/diff/44001/install_test/chrome_checko...
install_test/chrome_checkout.py:182: except (OSError, IOError):
On 2012/07/04 00:39:19, Nirnimesh wrote:
> do not try to catch it. The default behavior is fine. You're doing something
> equivalent anyway.

Got rid of the try/except block altogether. I suppose Python's response will be
good enough. Like you said, we were doing something similar by raising
RuntimeError, anyway.

http://codereview.chromium.org/10384104/diff/44001/install_test/chrome_checko...
install_test/chrome_checkout.py:193: _SvnCo('%s/src/chrome/test/functional' %
svn_url_base,
On 2012/07/04 00:39:19, Nirnimesh wrote:
> Checking out individual files like these is very hacky. Use the
functional.DEPS
> dependency instead to be futureproof.

We deliberately decided to check out only select files/folders. Entire
functional.DEPS is too large. It, therefore, takes longer to checkout. Plus,
there are several files we don't need. Ken had suggested that we only need
certain folders, which is why I'm only checking out the ones he recommended. If
you still feel that we should be using functional.DEPS, we can discuss it when
Ken returns on Monday.

http://codereview.chromium.org/10384104/diff/44001/install_test/install_test.py
File install_test/install_test.py (right):

http://codereview.chromium.org/10384104/diff/44001/install_test/install_test....
install_test/install_test.py:1: #!/usr/bin/env python
On 2012/07/04 00:39:19, Nirnimesh wrote:
> Why is the dir called 'install_test' if the intention is to cover 'update'
> scenarios?
> 
> We're not using the chrome installer, are we?

I asked Ken about this on Tuesday. His said that installation is a major part of
this framework. Update is just one aspect of what we're trying to do. If you
disagree, we can discuss this on Monday, when Ken returns, and come up with a
more appropriate name.

http://codereview.chromium.org/10384104/diff/44001/install_test/install_test....
install_test/install_test.py:27: _DOWNLOAD_DIR = []
On 2012/07/04 00:39:19, Nirnimesh wrote:
> Do not use globals please

The naming scheme being used previously for download directories necessitated
the use of globals. Each Chrome build specified by the user has a corresponding
folder in the temp directory where the installer, pyautolib/_pyautolib, and
other source files are downloaded. The names of these folders were randomly
generated before using tempfile.mkdtemp, which creates the folder and returns
the folder's path. Folders are created only once, at the very beginning of the
test. Even though folder creation and download occurs only once, the locations
are used by every unit-test for installing/updating Chrome and also for updating
the sys.path variable. We couldn't use a member variable for storing these
locations because a new instance of InstallTest is created for each unit-test.
Since folders are created only once, subsequent instances wouldn't know about
the location of these folders. We needed some way for subsequent instances of
InstallTest to know where the download directories were, which is why I decided
to use globals and why Ken was okay with it. But we have since changed the
naming scheme. We now use a fixed prefix (__CHRBLD__) and append the version
number to it (for e.g: __CHRBLD__21.0.1180.0). Also, tempdir.mkdtemp is no
longer used for creation of folders, so folder names aren't random. Using the
new naming scheme, subsequent instances of InstallTest can determine the
download directory name based on the Chrome version number. So we can do without
the globals. So, I went ahead and removed all the global variables except for
_OPTIONS, which holds the command line options. Initially the command parsing
was done by InstallTest class, so I had a member variable for this purpose. But
Ken asked me the do the command parsing in a separate class (a la PyAuto), so I
created a 'Main' class that does the command parsing. Since we needed a way to
access the command variables in InstallTest, I created a global for this
purpose. This is the same way that PyAuto does it.

http://codereview.chromium.org/10384104/diff/44001/install_test/install_test....
install_test/install_test.py:70: def _SetUp(self):
On 2012/07/04 00:39:19, Nirnimesh wrote:
> Choose a different name, not to confuse with setUp()

I chose the name _SetUp because PyUITest also has a method by the same name that
it uses for initialization and such. But I can see how this could cause
confusion, so I changed the name from _SetUp to _Initialize.

http://codereview.chromium.org/10384104/diff/44001/install_test/install_test....
install_test/install_test.py:94: self._source_dirs = [os.path.join('src',
'chrome', 'test', 'functional'),
On 2012/07/04 00:39:19, Nirnimesh wrote:
> This is a maintenance burden. Please do not do this.
> When things fail, the python crash would be instructive enough.

Got rid of this method. Its main purpose was to make sure that after the
checkout, the source directory contains everything that's needed. But now, we're
using a different approach. Ken and I had a discussion on Tuesday and came up
with an idea. Now, everything that is downloaded/checked-out is first put into a
temp folder. If everything goes smoothly(i.e. - no errors), we rename that temp
directory in accordance with our naming scheme. That way, subsequent test cases
can be sure that if the build folder exists, its not missing anything. If the
build folder doesn't exist, its either because it hasn't been created, or
because something went wrong in the previously. If that's the case, the
download/checkout process will be repeated and a new build folder will be
created. With this approach, we don't need any additional checks to ensure that
the build folder is not incomplete.

http://codereview.chromium.org/10384104/diff/44001/install_test/py_unittest_u...
File install_test/py_unittest_util.py (right):

http://codereview.chromium.org/10384104/diff/44001/install_test/py_unittest_u...
install_test/py_unittest_util.py:1: #!/usr/bin/env python
On 2012/07/03 23:37:02, Nirnimesh wrote:
> Move all of this to chrome/test/pyautolib/pyauto_utils.py

Moved all this code to pyauto_utils.py. Updated pyauto.py accordingly.

http://codereview.chromium.org/10384104/diff/44001/pyautolib/fetch_prebuilt_p...
File pyautolib/fetch_prebuilt_pyauto.py (right):

http://codereview.chromium.org/10384104/diff/44001/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:1: #!/usr/bin/env python
On 2012/07/03 23:37:02, Nirnimesh wrote:
> I committed a one-line fix to this file today. Please rebase.

Resynced fetch_prebuilt_pyauto, so the newly added code is included in it.

http://codereview.chromium.org/10384104/diff/44001/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:46: self._platform = ((lambda p: p and p or
pyauto_utils.GetCurrentPlatform())
On 2012/07/03 23:37:02, Nirnimesh wrote:
> I can't tell what you're trying to do with the lambda

This lambda is the equivalent of:

if p:
  return p
else:
  return pyauto_utils.GetCurrentPlatform()

p and p basically means return p if p is not None or not an empty string(''),
otherwise return the current platform. But I didn't realize, up until now, that
_ParseArgs is doing the same thing. If user specifies a platform, the user
specified platform is returned. If not, the default value is
pyauto_utils.GetCurrentPlatform(), so using the lambda here is redundant. I am
just going to get rid of it and simply assign self._platform = platform.

http://codereview.chromium.org/10384104/diff/44001/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:222: 
On 2012/07/03 23:37:02, Nirnimesh wrote:
> docstring

Added a docstring here.

http://codereview.chromium.org/10384104/diff/44001/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:263: Main()
On 2012/07/03 23:37:02, Nirnimesh wrote:
> wrap inside sys.exit()

Done.

kkania

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py File install_test/chrome_installer.py (right): http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py#newcode21 install_test/chrome_installer.py:21: _CSIDL_COMMON_APPDATA = 0x1C how about move these 2 to ...

8 years, 5 months ago (2012-07-23 16:46:23 UTC) #21

nkang

Addressed all the comments below, plus fixed a couple of bugs in fetch_prebuilt_pyauto.py and install_test.py. ...

8 years, 5 months ago (2012-07-25 23:39:20 UTC) #22

Addressed all the comments below, plus fixed a couple of bugs in
fetch_prebuilt_pyauto.py and install_test.py.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:21: _CSIDL_COMMON_APPDATA = 0x1C
On 2012/07/23 16:46:23, kkania wrote:
> how about move these 2 to class Installation?

Moved both these variables to Installation or ChromeInstallation, as it is now
called.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:23: _PLATFORM = platform.system().lower()
On 2012/07/23 16:46:23, kkania wrote:
> I see there's a few checks in this file that check if we're on windows. I
> thought these scripts were only for windows. If so, these checks seem a bit
> unnecessary.

I added this check because I was told that ultimately we will support other
platforms, as well. You certainly don't want to import ctypes or windll when on
other platforms, as it will invoke an error. But since this script will only run
on Windows for the time being, I will go ahead and remove the check. We can
always add it later on, if need be.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:50: """Installs the specified Chrome build."""
On 2012/07/23 16:46:23, kkania wrote:
> Document the args and return

Done.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:73: options = _RemoveDuplicates(options)
On 2012/07/23 16:46:23, kkania wrote:
> what's an example of options that might want to be passed in?    Why would
there
> be duplicates? Do duplicates actually cause a problem?

Example of an option that needs to be passed in is --system-level, which
specifies a system-level installation. Duplicates can occur either due to user
error, or if the user unknowingly passes an option that's already included in
the "UninstallArguments" string in the registry. The latter, as the name
implies, only applies to uninstall. The uninstall arguments string is obtained
from the registry and contains command line args to be passed to the installer.
When uninstalling a chrome build, we get the uninstall arguments from the
registry, per Anantha's request, and combine it with any user specified command
arguments; this is the scenario that's most likely to produce duplicate options.
As for whether or not duplicates cause any problems, I am not entirely sure. I
am inclined to believe that they do not cause any problems and are simply
ignored, but I cannot say conclusively whether they are or are not. So
eliminating duplicates is more of a sanity check for our own peace of mind. I
think we should just leave it; it certainly can't hurt. But if you feel strongly
that we should remove it, I am willing to reconsider. Let me know.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:80: logging.log(logging.ERROR, 'Installation
failed.')
On 2012/07/23 16:46:23, kkania wrote:
> How about throw an error here instead, which includes the exit code

Got rid of the logging.log message and the return value. If installation fails,
a RuntimeError exception is raised, which gets propagated to InstallTest.
InstallTest, which previously raised this exception, would not have to do so
anymore.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:91: HKEY_LOCAL =
(r'SOFTWARE\Wow6432Node\Google\Update\ClientState'
On 2012/07/23 16:46:23, kkania wrote:
> make these private?

Made both of them private. I had initially held off on making them private
because I wanted the names to adhere to Windows' naming convention. Therefore,
HKEY_USER fit the bill better than _HKEY_USER. But I went ahead and changed the
names, so they're now private.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:116: return None
On 2012/07/23 16:46:23, kkania wrote:
> when could this occur? Should we throw an exception instead?

This could occur for a number of reasons. It could occur if the key_name is
wrong (i.e., doesn't exist or misspelled), if the key_type is wrong (user
instead of system or vice versa), or if the caller doesn't have sufficient
privileges. I think we should leave this method as is. I think we should return
None instead of raising. Returning a handle if successful and None or zero on
failure is behavior that is more inline with the Windows APIs used for
manipulating the registry.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:148: """Returns Chrome's registry key name
based on installation type."""
On 2012/07/23 16:46:23, kkania wrote:
> The replace arg here is a bit strange. Can you find a cleaner way to do this?

Changed the replace arg with a 'value' arg. If the value specified is 'pv',
which stands for Product Version and represents the current Chrome version, then
ClientState in key name is replaced with Clients (since 'pv' is in
HKEY_CURRENT_USER\Software\Google\Update\Clients\{8A69D345-D564-463c-AFF1-A69D9E530F96}).
If not, which would be the case for 'UninstallArguments' or 'UninstallString',
it returns the key name as is. The key name itself is determined by the type of
installation. If user, it uses self._HKEY_USER, and for system level
installation, it uses self._HKEY_LOCAL.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:223: def _Delete(self, _path):
On 2012/07/23 16:46:23, kkania wrote:
> i don't think this is used

Yes, you are right. This method is currently not being used. The reason for that
is, we had decided that for the time being we're not going to delete the Chrome
build folders we create in the temp directory. But the plan was that down the
line we will start deleting them, which is why I wrote this method. But since
we're not using it at the moment, I went ahead and excluded it from the code. I
moved it to a utility module I wrote that contains similar methods. This is fine
for the time being, but ultimately I think it would be a good idea to move the
'_Delete' method to pyauto_utils.py. That way we can import and use it as
needed.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:243: def GetChromeInstallationType(self):
On 2012/07/23 16:46:23, kkania wrote:
> a lot of functions in this class have Chrome in the name, which I think is
> redundant. I would just drop the Chrome part of the name. If you want, you can
> rename the class name to ChromeInstallation.  Also, I would drop the
> Installation part from this name too, since that is implied by the class name.

> So this should just be GetType

Changed the class name from Installation to ChromeInstallation. The following
methods were renamed from _DeleteChromeRegSettings, GetChromeVersion,
GetChromeExePath, GetChromeInstallationType, and UninstallChrome to
_DeleteRegistrySettings, GetVersion, GetPath, GetType, and Uninstall,
respectively. Updated chrome_installer.py and install_test.py to reflect this
change.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:273: logging.log(logging.ERROR, 'Could not find
chrome, aborting uninstall.')
Since we're now raising an exception instead of returning False below, I also
went ahead and changed this return value. This method no longer returns True or
False; it simply raises on error or not if everything goes smoothly. Updated
InstallTest accordingly.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:276: ret = self._LaunchInstaller(uninstall_str,
options)
On 2012/07/23 16:46:23, kkania wrote:
> this function doesn't look like it returns anything

LaunchInstaller did at one point return a value, but it was later changed. So it
now raises instead of returning, but I guess where this method was being called
was left unchanged. Went ahead and removed the 'ret ='.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:279: # TODO: add clean option like
ChromeInstaller
On 2012/07/23 16:46:23, kkania wrote:
> remove this TODO

TODO: Remove the comment. Done!

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:285: logging.log(logging.ERROR, 'Uninstall
failed.')
On 2012/07/23 16:46:23, kkania wrote:
> how about throw error instead

This method now raises a RuntimeError exception. Updated InstallTest
accordingly.

kkania

Some comments on the changes http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py File install_test/chrome_installer.py (right): http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py#newcode23 install_test/chrome_installer.py:23: _PLATFORM = platform.system().lower() On ...

8 years, 5 months ago (2012-07-25 23:59:38 UTC) #23

Some comments on the changes

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:23: _PLATFORM = platform.system().lower()
On 2012/07/25 23:39:21, nkang wrote:
> On 2012/07/23 16:46:23, kkania wrote:
> > I see there's a few checks in this file that check if we're on windows. I
> > thought these scripts were only for windows. If so, these checks seem a bit
> > unnecessary.
> 
> I added this check because I was told that ultimately we will support other
> platforms, as well. You certainly don't want to import ctypes or windll when
on
> other platforms, as it will invoke an error. But since this script will only
run
> on Windows for the time being, I will go ahead and remove the check. We can
> always add it later on, if need be. 

There's a few other places you need to update that assert or check this. Remove
this var when you're done.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:73: options = _RemoveDuplicates(options)
Try running the installer/uninstaller manually and see if duplicate args cause
problems. If the installer uses the same command line parser as chrome (which it
probably does), this shouldn't be an issue. I'd go ahead and remove the
duplicate check.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:116: return None
On 2012/07/25 23:39:21, nkang wrote:
> On 2012/07/23 16:46:23, kkania wrote:
> > when could this occur? Should we throw an exception instead?
> 
> This could occur for a number of reasons. It could occur if the key_name is
> wrong (i.e., doesn't exist or misspelled), if the key_type is wrong (user
> instead of system or vice versa), or if the caller doesn't have sufficient
> privileges. I think we should leave this method as is. I think we should
return
> None instead of raising. Returning a handle if successful and None or zero on
> failure is behavior that is more inline with the Windows APIs used for
> manipulating the registry.

I don't think it's too important that we try to follow the windows API. I think
trying to access a nonexistent or key that you don't have privileges for should
count as an error, and it would better to just have the exception be raised from
_winreg. I would just remove this function and use _winreg directly. The only
thing it really does for us is add a 0 as the third parameter.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:148: """Returns Chrome's registry key name
based on installation type."""
On 2012/07/25 23:39:21, nkang wrote:
> On 2012/07/23 16:46:23, kkania wrote:
> > The replace arg here is a bit strange. Can you find a cleaner way to do
this?
> 
> 
> Changed the replace arg with a 'value' arg. If the value specified is 'pv',
> which stands for Product Version and represents the current Chrome version,
then
> ClientState in key name is replaced with Clients (since 'pv' is in
>
HKEY_CURRENT_USER\Software\Google\Update\Clients\{8A69D345-D564-463c-AFF1-A69D9E530F96}).
> If not, which would be the case for 'UninstallArguments' or 'UninstallString',
> it returns the key name as is. The key name itself is determined by the type
of
> installation. If user, it uses self._HKEY_USER, and for system level
> installation, it uses self._HKEY_LOCAL.

I see. I think this function is still confusing. See if you can rework it
somehow to make it more understandable. Also, why do you have to pass in the
install type? Can we just use self._type?

kkania

On 2012/07/25 23:59:38, kkania wrote: > Some comments on the changes > > http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py > ...

8 years, 4 months ago (2012-07-30 17:31:42 UTC) #24

On 2012/07/25 23:59:38, kkania wrote:
> Some comments on the changes
> 
>
http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
> File install_test/chrome_installer.py (right):
> 
>
http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
> install_test/chrome_installer.py:23: _PLATFORM = platform.system().lower()
> On 2012/07/25 23:39:21, nkang wrote:
> > On 2012/07/23 16:46:23, kkania wrote:
> > > I see there's a few checks in this file that check if we're on windows. I
> > > thought these scripts were only for windows. If so, these checks seem a
bit
> > > unnecessary.
> > 
> > I added this check because I was told that ultimately we will support other
> > platforms, as well. You certainly don't want to import ctypes or windll when
> on
> > other platforms, as it will invoke an error. But since this script will only
> run
> > on Windows for the time being, I will go ahead and remove the check. We can
> > always add it later on, if need be. 
> 
> There's a few other places you need to update that assert or check this.
Remove
> this var when you're done.
> 
>
http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
> install_test/chrome_installer.py:73: options = _RemoveDuplicates(options)
> Try running the installer/uninstaller manually and see if duplicate args cause
> problems. If the installer uses the same command line parser as chrome (which
it
> probably does), this shouldn't be an issue. I'd go ahead and remove the
> duplicate check.
> 
>
http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
> install_test/chrome_installer.py:116: return None
> On 2012/07/25 23:39:21, nkang wrote:
> > On 2012/07/23 16:46:23, kkania wrote:
> > > when could this occur? Should we throw an exception instead?
> > 
> > This could occur for a number of reasons. It could occur if the key_name is
> > wrong (i.e., doesn't exist or misspelled), if the key_type is wrong (user
> > instead of system or vice versa), or if the caller doesn't have sufficient
> > privileges. I think we should leave this method as is. I think we should
> return
> > None instead of raising. Returning a handle if successful and None or zero
on
> > failure is behavior that is more inline with the Windows APIs used for
> > manipulating the registry.
> 
> I don't think it's too important that we try to follow the windows API. I
think
> trying to access a nonexistent or key that you don't have privileges for
should
> count as an error, and it would better to just have the exception be raised
from
> _winreg. I would just remove this function and use _winreg directly. The only
> thing it really does for us is add a 0 as the third parameter.
> 
>
http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
> install_test/chrome_installer.py:148: """Returns Chrome's registry key name
> based on installation type."""
> On 2012/07/25 23:39:21, nkang wrote:
> > On 2012/07/23 16:46:23, kkania wrote:
> > > The replace arg here is a bit strange. Can you find a cleaner way to do
> this?
> > 
> > 
> > Changed the replace arg with a 'value' arg. If the value specified is 'pv',
> > which stands for Product Version and represents the current Chrome version,
> then
> > ClientState in key name is replaced with Clients (since 'pv' is in
> >
>
HKEY_CURRENT_USER\Software\Google\Update\Clients\{8A69D345-D564-463c-AFF1-A69D9E530F96}).
> > If not, which would be the case for 'UninstallArguments' or
'UninstallString',
> > it returns the key name as is. The key name itself is determined by the type
> of
> > installation. If user, it uses self._HKEY_USER, and for system level
> > installation, it uses self._HKEY_LOCAL.
> 
> I see. I think this function is still confusing. See if you can rework it
> somehow to make it more understandable. Also, why do you have to pass in the
> install type? Can we just use self._type?

Do you have an update on this yet?

nkang1

Hi guys, I've been sick for the past few days. That's why I haven't been ...

8 years, 4 months ago (2012-07-31 16:28:29 UTC) #25

Hi guys,

I've been sick for the past few days. That's why I haven't been able to
finish this off. But I am back now, so I will wrap it up today.

Navdeep

On Mon, Jul 30, 2012 at 10:31 AM, <kkania@chromium.org> wrote:

> On 2012/07/25 23:59:38, kkania wrote:
>
>> Some comments on the changes
>>
>
>
> http://codereview.chromium.**org/10384104/diff/49001/**
>
install_test/chrome_installer.**py<http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py>
>
>> File install_test/chrome_installer.**py (right):
>>
>
>
> http://codereview.chromium.**org/10384104/diff/49001/**
>
install_test/chrome_installer.**py#newcode23<http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py#newcode23>
>
>> install_test/chrome_installer.**py:23: _PLATFORM =
>> platform.system().lower()
>> On 2012/07/25 23:39:21, nkang wrote:
>> > On 2012/07/23 16:46:23, kkania wrote:
>> > > I see there's a few checks in this file that check if we're on
>> windows. I
>> > > thought these scripts were only for windows. If so, these checks seem
>> a
>>
> bit
>
>> > > unnecessary.
>> >
>> > I added this check because I was told that ultimately we will support
>> other
>> > platforms, as well. You certainly don't want to import ctypes or windll
>> when
>> on
>> > other platforms, as it will invoke an error. But since this script will
>> only
>> run
>> > on Windows for the time being, I will go ahead and remove the check. We
>> can
>> > always add it later on, if need be.
>>
>
>  There's a few other places you need to update that assert or check this.
>>
> Remove
>
>> this var when you're done.
>>
>
>
> http://codereview.chromium.**org/10384104/diff/49001/**
>
install_test/chrome_installer.**py#newcode73<http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py#newcode73>
>
>> install_test/chrome_installer.**py:73: options =
>> _RemoveDuplicates(options)
>> Try running the installer/uninstaller manually and see if duplicate args
>> cause
>> problems. If the installer uses the same command line parser as chrome
>> (which
>>
> it
>
>> probably does), this shouldn't be an issue. I'd go ahead and remove the
>> duplicate check.
>>
>
>
> http://codereview.chromium.**org/10384104/diff/49001/**
>
install_test/chrome_installer.**py#newcode116<http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py#newcode116>
>
>> install_test/chrome_installer.**py:116: return None
>> On 2012/07/25 23:39:21, nkang wrote:
>> > On 2012/07/23 16:46:23, kkania wrote:
>> > > when could this occur? Should we throw an exception instead?
>> >
>> > This could occur for a number of reasons. It could occur if the
>> key_name is
>> > wrong (i.e., doesn't exist or misspelled), if the key_type is wrong
>> (user
>> > instead of system or vice versa), or if the caller doesn't have
>> sufficient
>> > privileges. I think we should leave this method as is. I think we should
>> return
>> > None instead of raising. Returning a handle if successful and None or
>> zero
>>
> on
>
>> > failure is behavior that is more inline with the Windows APIs used for
>> > manipulating the registry.
>>
>
>  I don't think it's too important that we try to follow the windows API. I
>>
> think
>
>> trying to access a nonexistent or key that you don't have privileges for
>>
> should
>
>> count as an error, and it would better to just have the exception be
>> raised
>>
> from
>
>> _winreg. I would just remove this function and use _winreg directly. The
>> only
>> thing it really does for us is add a 0 as the third parameter.
>>
>
>
> http://codereview.chromium.**org/10384104/diff/49001/**
>
install_test/chrome_installer.**py#newcode148<http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_installer.py#newcode148>
>
>> install_test/chrome_installer.**py:148: """Returns Chrome's registry key
>> name
>> based on installation type."""
>> On 2012/07/25 23:39:21, nkang wrote:
>> > On 2012/07/23 16:46:23, kkania wrote:
>> > > The replace arg here is a bit strange. Can you find a cleaner way to
>> do
>> this?
>> >
>> >
>> > Changed the replace arg with a 'value' arg. If the value specified is
>> 'pv',
>> > which stands for Product Version and represents the current Chrome
>> version,
>> then
>> > ClientState in key name is replaced with Clients (since 'pv' is in
>> >
>>
>
> HKEY_CURRENT_USER\Software\**Google\Update\Clients\{**
> 8A69D345-D564-463c-AFF1-**A69D9E530F96}).
>
>> > If not, which would be the case for 'UninstallArguments' or
>>
> 'UninstallString',
>
>> > it returns the key name as is. The key name itself is determined by the
>> type
>> of
>> > installation. If user, it uses self._HKEY_USER, and for system level
>> > installation, it uses self._HKEY_LOCAL.
>>
>
>  I see. I think this function is still confusing. See if you can rework it
>> somehow to make it more understandable. Also, why do you have to pass in
>> the
>> install type? Can we just use self._type?
>>
>
> Do you have an update on this yet?
>
>
http://codereview.chromium.**org/10384104/<http://codereview.chromium.org/103...
>

nkang

Addressed all the comments below. Plus, created a new class called ChromeRegistryKeys that will be ...

8 years, 4 months ago (2012-08-02 21:05:42 UTC) #26

Addressed all the comments below. Plus, created a new class called
ChromeRegistryKeys that will be used for accessing and manipulating Chrome
related registry keys. All registry related methods from ChromeInstallation were
moved to this class. In addition, several new methods were created for accessing
and manipulating the registry. To identify Chrome's various different registry
values - such as 'pv', 'UninstallArguments', 'UninstallString', etc. - a new
enumerator class was created. The enumerator class contains only three members,
each uses a numerical code to identify the aforementioned Chrome registry
values.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:23: _PLATFORM = platform.system().lower()
On 2012/07/25 23:59:38, kkania wrote:
> On 2012/07/25 23:39:21, nkang wrote:
> > On 2012/07/23 16:46:23, kkania wrote:
> > > I see there's a few checks in this file that check if we're on windows. I
> > > thought these scripts were only for windows. If so, these checks seem a
bit
> > > unnecessary.
> > 
> > I added this check because I was told that ultimately we will support other
> > platforms, as well. You certainly don't want to import ctypes or windll when
> on
> > other platforms, as it will invoke an error. But since this script will only
> run
> > on Windows for the time being, I will go ahead and remove the check. We can
> > always add it later on, if need be. 
> 
> There's a few other places you need to update that assert or check this.
Remove
> this var when you're done.

Got rid of the _PLATFORM variable and removed all references to it. The script
will now run under the assumption that the platform its running on is Windows,
since all the platform checks have now been removed.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:73: options = _RemoveDuplicates(options)
On 2012/07/25 23:59:38, kkania wrote:
> Try running the installer/uninstaller manually and see if duplicate args cause
> problems. If the installer uses the same command line parser as chrome (which
it
> probably does), this shouldn't be an issue. I'd go ahead and remove the
> duplicate check.

Checked with Prudhvi regarding duplicate options, since he does a lot of
installer testing. According to him, it makes no difference if there are
duplicates in the command line options. All command line options passed to the
installer are saved in the registry under the 'UninstallArguments' key. If those
options contain duplicates, they are filtered before the options are entered
into the registry. In other words, no duplicates are saved in the registry. So
it seems that the duplicate removal task that chrome_installer was doing, is
already being done by Chrome. Went ahead and removed the _RemoveDuplicates
method and any references to it.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:116: return None
On 2012/07/25 23:59:38, kkania wrote:
> On 2012/07/25 23:39:21, nkang wrote:
> > On 2012/07/23 16:46:23, kkania wrote:
> > > when could this occur? Should we throw an exception instead?
> > 
> > This could occur for a number of reasons. It could occur if the key_name is
> > wrong (i.e., doesn't exist or misspelled), if the key_type is wrong (user
> > instead of system or vice versa), or if the caller doesn't have sufficient
> > privileges. I think we should leave this method as is. I think we should
> return
> > None instead of raising. Returning a handle if successful and None or zero
on
> > failure is behavior that is more inline with the Windows APIs used for
> > manipulating the registry.
> 
> I don't think it's too important that we try to follow the windows API. I
think
> trying to access a nonexistent or key that you don't have privileges for
should
> count as an error, and it would better to just have the exception be raised
from
> _winreg. I would just remove this function and use _winreg directly. The only
> thing it really does for us is add a 0 as the third parameter.

The reason I wrote this method was because I wanted to handle the
_winreg.error/OSError exception that occurs when the key cannot be opened for
whatever reason. But I didn't want every method that called _winreg.OpenKey to
have to have its own try/except block, which would've been redundant. Plus,
there's one method called 'GetType', which determines the Chrome installation
type, that absolutely needs to handle this exception. 'GetType' first tries to
check the user level installation by opening the Chrome key in HKCU. If that
fails, it catches the exception, and in the except block, it tries to open the
HKLM key. If that raises an exception as well, it handles the exception and
return None as the installation type, which indicates that no Chrome versions
are present on this machine. That is why I wrote this method, but if you want
_winreg to just raise the exception then there's really no need for this method.
So I am going to remove ChromeInstallation._OpenKey. Wherever it is called,
we'll now use _winreg.OpenKey instead. As a result, I will also have to
drastically change the 'GetType' method.

http://codereview.chromium.org/10384104/diff/49001/install_test/chrome_instal...
install_test/chrome_installer.py:148: """Returns Chrome's registry key name
based on installation type."""
On 2012/07/25 23:59:38, kkania wrote:
> On 2012/07/25 23:39:21, nkang wrote:
> > On 2012/07/23 16:46:23, kkania wrote:
> > > The replace arg here is a bit strange. Can you find a cleaner way to do
> this?
> > 
> > 
> > Changed the replace arg with a 'value' arg. If the value specified is 'pv',
> > which stands for Product Version and represents the current Chrome version,
> then
> > ClientState in key name is replaced with Clients (since 'pv' is in
> >
>
HKEY_CURRENT_USER\Software\Google\Update\Clients\{8A69D345-D564-463c-AFF1-A69D9E530F96}).
> > If not, which would be the case for 'UninstallArguments' or
'UninstallString',
> > it returns the key name as is. The key name itself is determined by the type
> of
> > installation. If user, it uses self._HKEY_USER, and for system level
> > installation, it uses self._HKEY_LOCAL.
> 
> I see. I think this function is still confusing. See if you can rework it
> somehow to make it more understandable. Also, why do you have to pass in the
> install type? Can we just use self._type?

Got rid of this method altogether. Per our conversation, created a new class
called ChromeRegistryKeys that will be used for accessing and manipulating
Chrome related registry keys. All registry related methods from
ChromeInstallation were moved to this class. In addition, several new methods
were created that, among other things, generate the appropriate registry key
name based on the installation type and the value the user wishes to look up. To
identify Chrome's various different registry values - such as 'pv',
'UninstallArguments', 'UninstallString', etc. - a new enumerator class was
created. The enumerator class contains only three members, each uses a numerical
code to identify the aforementioned Chrome registry values.

kkania

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_installer.py File install_test/chrome_installer.py (right): http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_installer.py#newcode29 install_test/chrome_installer.py:29: PRODUCT_VERSION = 0 you can just change these values ...

8 years, 4 months ago (2012-08-03 18:41:08 UTC) #27

nkang

Made several changes to chrome_installer.py. Some of the changes are: - Wrote a new class ...

8 years, 4 months ago (2012-08-06 23:57:13 UTC) #28

Made several changes to chrome_installer.py. Some of the changes are:
- Wrote a new class called ChromeRegistryKeys for accessing and manipulating
Chrome registry keys.
- Wrote a new enumerator class for identifying various Chrome registry values.
- All registry related methods were moved from ChromeInstallation to
ChromeRegistryKeys.
- Several methods were renamed.
- GetType, which is now called GetCurrent, was changed to a static method and
now returns a ChromeInstallation object if Chrome is present and None if it its
not.
- Updated InstallTest to reflect these changes.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:29: PRODUCT_VERSION = 0
On 2012/08/03 18:41:08, kkania wrote:
> you can just change these values to the string equivalents; that should make
it
> a bit simpler

Changed the three values from numerical to string. This will save some code in
ChromeRegistryKeys, as we no longer have to convert the numerical values to
string when looking up their respective values in the registry.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:45: A ChromeInstallation object if successful,
otherwise None.
On 2012/08/03 18:41:08, kkania wrote:
> this comment needs to be updated

Changed the comment. It now says, 'Returns: A ChromeInstallation object'.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:54: install_type = ('system-level' in options
and InstallationType.SYSTEM
On 2012/08/03 18:41:08, kkania wrote:
> this statement no longer works. that is why I typically don't like this trick.
> fix.

Good find! Changed this statement to the following:
install_type = (InstallationType.SYSTEM if 'system-level' in options else
                InstallationType.USER)

Logically speaking, there's no difference between this statement and the
original one, but for some reason, the latter works as expected but the former
does not. Amazing find.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:89: _chrome_ustring =
r'ClientState\{8A69D345-D564-463C-AFF1-A69D9E530F96}'
On 2012/08/03 18:41:08, kkania wrote:
> what's diff between this and above var? merge them

There's no difference; they are the same. I just created two separate variables
for clarity, so each value ('pv', 'UninstallString', and 'UninstallArguments')
has its own key name. But since you asked for them to be merged, I changed the
variables. Instead of two separate variables with the same registry key, there's
now just one, _chrome_args, which can be used for both values.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:112: return ''
On 2012/08/03 18:41:08, kkania wrote:
> throw instead

Instead of returning an empty string, the method now raises a RuntimeError
exception if the value specified is invalid.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:127: return None
On 2012/08/03 18:41:08, kkania wrote:
> throw instead

Instead of returning None, the method raises a RuntimeError exception if an
invalid installation type is passed as an argument.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:143: return b_exists
On 2012/08/03 18:41:08, kkania wrote:
> throw instead

Got rid of this statement altogether. We can't throw here because this method is
needed to figure out the type of Chrome installation. When trying to figure out
the installation type, we first check in HKEY_CURRENT_USER. If a Chrome
installation is found there, InstallationType.USER is returned. If, however, no
Chrome installation is found in HKEY_CURRENT_USER, it will result in a
_winreg.error exception. We need to handle that exception, so we can continue on
and check in HKEY_LOCAL_MACHINE. So the logical choice here would be to just get
rid of the 'if' statement, since both '_GetRegistryType' and '_GeyKeyName' throw
if an invalid argument is passed.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:161: A string representing the subkey value if
successful, otherwise None.
On 2012/08/03 18:41:08, kkania wrote:
> don't return none in this func in any circumstance

Got rid of the None return value if a _winreg.error exception is raised. The
method no longer handles this exception, so if an error occurs, the exception
will just be raised. Updated the docstring accordingly.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:170: if not key or not key_name:
On 2012/08/03 18:41:08, kkania wrote:
> don't check here, let the other funcs throw

Got rid of the 'if' statement, altogether. If key is not valid or if key_name is
not valid, we're just going to let the other methods throw.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:173: hkey = _winreg.OpenKey(key, key_name)
On 2012/08/03 18:41:08, kkania wrote:
> don't catch exceptions here, let them be raised

Got rid of the try/except block. We will no longer catch _winreg.error
exceptions. The exceptions will be raised, if an error occurs.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:190: root = key_name[: key_name.rfind('\\')]
On 2012/08/03 18:41:08, kkania wrote:
> no space between :

Fixed.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:191: child = key_name[key_name.rfind('\\') + 1
:]
On 2012/08/03 18:41:08, kkania wrote:
> same

Fixed.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:196: except _winreg.error, err:
On 2012/08/03 18:41:08, kkania wrote:
> why bother catching if you're just going to raise?

Previously 'DeleteRegistryEntries' did not raise at all. It used to handle the
_winreg.error exception and would return a zero or non-zero value based on
success or failure of the deletion process. The reasoning behind that was, we
didn't want the test to stop prematurely because we couldn't delete the registry
entries. Deleting registry entries is not crucial. In fact, up until now, it was
optional. Previously, the user would pass the --clean option, which was False by
default, and based on that we would determine whether or not to even delete the
registry entries. Furthermore, deleting registry entries from HKEY_LOCAL_MACHINE
is not allowed, as it invokes an 'Access denied' error, unless, of course,
you're running the command prompt as an admin. So we didn't want to raise an
exception, and thus cause the test to stop prematurely for something that, up
until now, was deemed inconsequential. However, after you reviewed the code and
recommended that we raise instead of returning, the simplest way to do that was
to just raise the exception that we were already catching. But since you want me
to get rid of the try/except block altogether, I went ahead and removed it. The
exception will not be caught or re raised anymore. If an error occurs during
deletion, we'll just let _winreg raise an exception.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:246: def _LaunchInstaller(self, installer_path,
options):
On 2012/08/03 18:41:08, kkania wrote:
> remove this separate func and put subproces.popen in Uninstall; and don't
catch
> the exception

Got rid of the '_LaunchInstaller' method. The whole reason I wrote this method
was so it could handle any exceptions that might be raised while launching a
child process, which in this case means the mini_installer. But if we're not
catching the exception, then there's really no reason to have this method. Got
rid of the method and the exception handling code, which left only one line of
code, which now gets called in the 'Uninstall' method.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:259: def GetPath(self):
On 2012/08/03 18:41:08, kkania wrote:
> GetExePath

Changed method name to GetExePath.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:270: def GetType():
On 2012/08/03 18:41:08, kkania wrote:
> change this to GetCurrent, which returns None or an instance of this class

Changed method name to GetCurrent. Also changed the return type. Instead of
returning the installation type (or None if no Chrome version is found), the
method now returns a ChromeInstallation object instantiated with the current
type if a Chrome version is found on the system, otherwise it returns None.

http://codereview.chromium.org/10384104/diff/74001/install_test/chrome_instal...
install_test/chrome_installer.py:287: install_type =
ChromeInstallation.GetType()
On 2012/08/03 18:41:08, kkania wrote:
> don't do this

Fine, I won't :)

Nirnimesh

This is looking much better than when I had looked last. http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): ...

8 years, 4 months ago (2012-08-09 21:32:48 UTC) #29

nkang1

That's music to my ears :) On Thu, Aug 9, 2012 at 2:32 PM, <nirnimesh@chromium.org> ...

8 years, 4 months ago (2012-08-09 21:54:24 UTC) #30

That's music to my ears :)

On Thu, Aug 9, 2012 at 2:32 PM, <nirnimesh@chromium.org> wrote:

> This is looking much better than when I had looked last.
>
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py>
> File install_test/chrome_checkout.**py (right):
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode9<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode9>
> install_test/chrome_checkout.**py:9: used to obtain the revision number,
> which is then used to checkout files from
> describe how "The version number is used to obtain the revision number"
>
> describe about the DEPS file
>
> Also declare all the other files it needs from the internet
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode34<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode34>
> install_test/chrome_checkout.**py:34: def
> _GetContentAndReturnResponse(**server, path):
> why not just use urllib?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode44<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode44>
> install_test/chrome_checkout.**py:44: try:
> all this try/catch seems unncessary to me. Remove it. If it fails, the
> right trace will be thrown anyway
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode101<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode101>
> install_test/chrome_checkout.**py:101: match = re.search("'src':[\n\r
> ]+'(.*?)'", deps)
> use ' for the outer quotes
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode103<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode103>
> install_test/chrome_checkout.**py:103: match = re.search(r"@(\d+)",
> match.group(1))
> use '
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode107<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode107>
> install_test/chrome_checkout.**py:107: match =
> re.search("""['"]src['"].*?:.***?['"]/branches/(.*?)/.*?,""",
> this is very confusing. give an example of what it's trying to match
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode116<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode116>
> install_test/chrome_checkout.**py:116: def _GetRevision(deps,
> rev_type='selenium'):
> do not provide default arg
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode126<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode126>
> install_test/chrome_checkout.**py:126: assert(rev_type == 'selenium' or
> rev_type == 'pyftpdlib')
> remove parens
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode128<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode128>
> install_test/chrome_checkout.**py:128: m =
> re.search(r'http://selenium\.**googlecode\.com/svn/trunk/py@(**\d+)',
> m -> match
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode146<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode146>
> install_test/chrome_checkout.**py:146: cmd = 'svn co'
> make cmd a list, and then remove shell=True from line 153
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode153<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode153>
> install_test/chrome_checkout.**py:153: assert(subprocess.Popen(cmd,
> shell=True).wait() == 0)
> remove outer parens
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode153<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode153>
> install_test/chrome_checkout.**py:153: assert(subprocess.Popen(cmd,
> shell=True).wait() == 0)
> remove assert. use subprocess.check_call instead
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode166<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode166>
> install_test/chrome_checkout.**py:166: return
> re.findall('\d+\.\d+\.\d+\.\d+**', version) != []
> use re.match()
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode190<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode190>
> install_test/chrome_checkout.**py:190:
> _SvnCo('%s/src/chrome/test/**functional' % svn_url_base,
> This seems too fragile. if pyauto requires additional dependencies, no
> one is going to remember to add them here. Why not use functional.DEPS?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_checkout.**py#newcode191<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode191>
> install_test/chrome_checkout.**py:191: rev_info['revision'],
> os.path.join(dest, 'src', 'chrome', 'test',
> how do you plan to handle internal deps?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py>
> File install_test/chrome_installer.**py (right):
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode6<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode6>
> install_test/chrome_installer.**py:6: """Provides an interface for
> installing Chrome."""
> Is this supposed to be for win only? If so, mention it here
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode8<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode8>
> install_test/chrome_installer.**py:8: import _winreg
> will fail on non-win
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode21<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode21>
> install_test/chrome_installer.**py:21: class InstallationType:
> inherit from object
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode27<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode27>
> install_test/chrome_installer.**py:27: class ChromeRegistryValues:
> inherit from object
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode44<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode44>
> install_test/chrome_installer.**py:44: A ChromeInstallation object.
> an instance of ChromeInstallation.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode46<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode46>
> install_test/chrome_installer.**py:46: def DoPreliminaryChecks(regedit):
> prefix _
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode47<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode47>
> install_test/chrome_installer.**py:47: """Validates the test parameters
> and Chrome version.
> Be more specific. Call out exactly what it does.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode52<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode52>
> install_test/chrome_installer.**py:52:
> assert(os.path.isfile(**installer_path))
> why?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode61<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode61>
> install_test/chrome_installer.**py:61: if current_type != None:
> remove "!= None"
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode71<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode71>
> install_test/chrome_installer.**py:71: raise RuntimeError('Please specify
> a version higher than the one '
> Remove "Please"
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode78<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode78>
> install_test/chrome_installer.**py:78: cmd = '%s %s' % (installer_path,
> options)
> either use + to join strings (line 76), or use this format. Not both
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode79<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode79>
> install_test/chrome_installer.**py:79: ret = subprocess.Popen(cmd,
> shell=True).wait()
> make cmd a list. remove shell=True
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode80<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode80>
> install_test/chrome_installer.**py:80: if ret == 0:
> Invert the logic. Raise first if ret != 0
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode90<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode90>
> install_test/chrome_installer.**py:90: _HKEY_USER =
> _HKEY_LOCAL.replace('\\**Wow6432Node', '')
> either use r'..' or \\ thoughout.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode95<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode95>
> install_test/chrome_installer.**py:95: """Generates the registry key name
> for the specified value.
> Generate -> Get
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode142<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode142>
> install_test/chrome_installer.**py:142: b_exists = False
> remove "b_"
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode149<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode149>
> install_test/chrome_installer.**py:149: hkey.Close()
> If you just return True from here, you don't even need the boolean var
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/chrome_installer.**py#newcode241<http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode241>
> install_test/chrome_installer.**py:241: chrome_path =
> os.path.join(self._**GetWinLocalFolder(folder_id), 'Google',
> What about Chromium?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py>
> File install_test/install_test.py (right):
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode4<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode4>
> install_test/install_test.py:**4: # found in the LICENSE file.
> docstring.
>
> Give an example test. Look at the top of pyauto.py
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode29<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode29>
> install_test/install_test.py:**29: _OPTIONS = None
> Do not use globals. What you're doing can easily be done without it.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode46<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode46>
> install_test/install_test.py:**46: _dir_prefix = '__CHRBLD__'
> why the mystique name?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode49<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode49>
> install_test/install_test.py:**49: _installer_name = 'mini_installer.exe'
> So all this is win only? Where did you declare/check it?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode62<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode62>
> install_test/install_test.py:**62: if not self._DownloadDeps(build):
> Do you want the downloading to be done separately for every test?
>
> Remember that unittest module initializes all tests in one go, and then
> runs their setup/test/tearDown one by one.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode88<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode88>
> install_test/install_test.py:**88: self.failIf(self._pyauto == None)
> call parent's setup
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode93<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode93>
> install_test/install_test.py:**93: self._DeleteBuild()
> call parent's tearDown
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode101<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode101>
> install_test/install_test.py:**101: def _Refresh(self):
> Refresh? It's not clear from the method name that it does clearing. Use
> better name
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode104<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode104>
> install_test/install_test.py:**104: del(self._pyauto)
> remove parens
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode133<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode133>
> install_test/install_test.py:**133: self._pyauto.suite_holder =
> pyauto.PyUITestSuite(['test.**py'])
> [] would do I think
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode261<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode261>
> install_test/install_test.py:**261: def _OnError(self, func, path,
> exc_info):
> I've seen this elsewhere. Move this and line 252, 280.. to a helper
> method.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode286<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode286>
> install_test/install_test.py:**286: def _Download(self, dfile, url,
> dest=None):
> I've seen similar function elsewhere. Reuse code.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode344<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode344>
> install_test/install_test.py:**344: def _GetUnitTests(self):
> remove 'Unit'
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode360<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode360>
> install_test/install_test.py:**360: else:
> remove the else part. it's redundant
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode363<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode363>
> install_test/install_test.py:**363: def _ParseArgs(self):
> Move _ParseArgs before _Run
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode372<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode372>
> install_test/install_test.py:**372: help='Specifies the chrome-master2
> url, without the build number.')
> do not refer internal URLs/hostname.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode380<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode380>
> install_test/install_test.py:**380: _OPTIONS = opts
> verify the sanity of the args here, esp --builds
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
install_test/install_test.py#**newcode380<http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py#newcode380>
> install_test/install_test.py:**380: _OPTIONS = opts
> Before firing off the install tests, it'd be nice to determine if the
> individual chrome builds are sane by themselves and can run pyauto test.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
pyautolib/fetch_prebuilt_**pyauto.py<http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_pyauto.py>
> File pyautolib/fetch_prebuilt_**pyauto.py (right):
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
pyautolib/fetch_prebuilt_**pyauto.py#newcode130<http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_pyauto.py#newcode130>
> pyautolib/fetch_prebuilt_**pyauto.py:130: def OnError(func, path,
> exc_info):
> _OnError
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
pyautolib/fetch_prebuilt_**pyauto.py#newcode144<http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_pyauto.py#newcode144>
> pyautolib/fetch_prebuilt_**pyauto.py:144: except (OSError, IOError):
> Since you're just re-raising the exception, the try block is a no-op.
> Remove the try/except block.
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
pyautolib/fetch_prebuilt_**pyauto.py#newcode240<http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_pyauto.py#newcode240>
> pyautolib/fetch_prebuilt_**pyauto.py:240: def _ParseArgs(self):
> I think it's better to move _ParseArgs to the above class too. then main
> will just call Run() on it.
>
>
http://codereview.chromium.**org/10384104/diff/73002/**pyautolib/pyauto.py<ht...
> File pyautolib/pyauto.py (right):
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
pyautolib/pyauto.py#newcode121<http://codereview.chromium.org/10384104/diff/73002/pyautolib/pyauto.py#newcode121>
> pyautolib/pyauto.py:121: def __init__(self, methodName='runTest',
> **kwargs):
> why not add browser_path arg here?
>
> http://codereview.chromium.**org/10384104/diff/73002/**
>
pyautolib/pyauto.py#newcode138<http://codereview.chromium.org/10384104/diff/73002/pyautolib/pyauto.py#newcode138>
> pyautolib/pyauto.py:138: browser_path = kwargs.get('browser_path', None)
> So BrowserPath() method will be out of sync?
>
>
http://codereview.chromium.**org/10384104/<http://codereview.chromium.org/103...
>

nkang

Addressed all the comments made by Nirnimesh. http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py File install_test/chrome_checkout.py (right): http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checkout.py#newcode9 install_test/chrome_checkout.py:9: used to ...

8 years, 4 months ago (2012-08-16 23:46:23 UTC) #31

Addressed all the comments made by Nirnimesh.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
File install_test/chrome_checkout.py (right):

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:9: used to obtain the revision number, which is
then used to checkout files from
On 2012/08/09 21:32:48, Nirnimesh wrote:
> describe how "The version number is used to obtain the revision number"
> 
> describe about the DEPS file
> 
> Also declare all the other files it needs from the internet

Changed the text so its more descriptive.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:34: def _GetContentAndReturnResponse(server,
path):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> why not just use urllib?

I thought about that, as well. But part of this script was written by Anthony
LaForge, including this method. Since he had already written it and it worked, I
decided not to mess with it. But I went ahead and changed it. However, I used
urllib2, since urllib.urlopen has been deprecated as of Python version 2.6.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:44: try:
On 2012/08/09 21:32:48, Nirnimesh wrote:
> all this try/catch seems unncessary to me. Remove it. If it fails, the right
> trace will be thrown anyway

Got rid of the try/except block. Also, this method has been completely reworked.
I changed the code so we use urllib2 instead of httplib, so most of the code
below is no longer in existence.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:101: match = re.search("'src':[\n\r ]+'(.*?)'",
deps)
On 2012/08/09 21:32:48, Nirnimesh wrote:
> use ' for the outer quotes

The reason we (Anthony LaForge originally added them when he wrote this method,
and I continued to use them) use double quotes on the outside is because the
word 'src' has single quotes around it in the DEPS file. If we switched the
double quotes with the single quotes, it wouldn't match. To use single quotes
inside single quotes, we'd have to use escape characters. So I changed it to
following: 
re.search('\'src\':[\n\r ]+\'(.*?)\'', deps)

The other option, of course, is to use triple quotes, but IMHO, we should use
the escape characters instead.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:103: match = re.search(r"@(\d+)",
match.group(1))
On 2012/08/09 21:32:48, Nirnimesh wrote:
> use '

Changed to single quotes.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:107: match =
re.search("""['"]src['"].*?:.*?['"]/branches/(.*?)/.*?,""",
On 2012/08/09 21:32:48, Nirnimesh wrote:
> this is very confusing. give an example of what it's trying to match

Branch releases have the branch number listed in the DEPS file, for example:
'/branches/1230/src@150565'. So, this line of code is trying to match the branch
number listed in DEPS. If no match is found, that means its a trunk.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:116: def _GetRevision(deps,
rev_type='selenium'):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> do not provide default arg

Got rid of the default argument.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:126: assert(rev_type == 'selenium' or rev_type
== 'pyftpdlib')
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove parens

Got rid of the parenthesis.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:128: m =
re.search(r'http://selenium\.googlecode\.com/svn/trunk/py@(\d+)',
On 2012/08/09 21:32:48, Nirnimesh wrote:
> m -> match

d -> done!

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:146: cmd = 'svn co'
On 2012/08/09 21:32:48, Nirnimesh wrote:
> make cmd a list, and then remove shell=True from line 153

Changed cmd to a list. However, removing shell=True results in the following
error: 

WindowsError: [Error 2] The system cannot find the file specified

I checked the documentation for the subprocess module, and it states that 'args'
can be passed as a string or as a sequence of program arguments. Documentation
further states that 'shell=True' is required if using a string as 'args', unless
the string specifies the name of a program without any arguments. I'm guessing
that's why you told me to convert 'cmd' to a list. However, even after I changed
it to a list, I kept getting the aforementioned error. The only way it works is
if I pass 'shell=True'. I did some more digging and discovered that if
shell=False is used, python uses the CreateProcess API on Windows to launch
another process, and CreateProcess adds a '.exe' to the process name if it
doesn't have an extension, which would explain why we get the 'file not found'
error. Also, I found a similar thread that someone had posted on the Python-Dev
site. It states that if you need to run something that's not a Windows
executable, such as a batch file, you should use the command interpreter (i.e.
shell=True). Since svn is also launched using a batch file (svn.bat), I guess it
makes sense why it doesn't work with shell=False. This also explains why Chrome
works when launched without shell=True but not svn. 

So, for the time being, I've left shell=True in there. I did get rid of the
assert and changed subprocess.Popen to subprocess.check_call.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:153: assert(subprocess.Popen(cmd,
shell=True).wait() == 0)
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove assert. use subprocess.check_call instead

Got rid of the assert and replaced it with subprocess.check_call.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:153: assert(subprocess.Popen(cmd,
shell=True).wait() == 0)
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove outer parens

Got rid of the assert, so this no longer applies.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:166: return re.findall('\d+\.\d+\.\d+\.\d+',
version) != []
On 2012/08/09 21:32:48, Nirnimesh wrote:
> use re.match()

Changed re.findall to re.match.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:190: _SvnCo('%s/src/chrome/test/functional' %
svn_url_base,
On 2012/08/09 21:32:48, Nirnimesh wrote:
> This seems too fragile. if pyauto requires additional dependencies, no one is
> going to remember to add them here. Why not use functional.DEPS?

Checked with Ken about this and he was adamant that we should not use
functional.DEPS. He said that functional.DEPS contains items we do not need,
particularly the 'data' folder, which is humongous (over 600MB approx). He
further stated that using functional.DEPS would mean that we would 'have to
substitute the branch numbers'. He recommended that we either leave it like this
or create a new DEPS file for these dependencies. Personally I feel that, given
the amount of time that's already been put into this framework, we should just
continue on like this for the time being. Down the line we can create a new DEPS
file. The rest is up to you guys. I am okay with whatever we decide.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_checko...
install_test/chrome_checkout.py:191: rev_info['revision'], os.path.join(dest,
'src', 'chrome', 'test',
On 2012/08/09 21:32:48, Nirnimesh wrote:
> how do you plan to handle internal deps?

Checked with Anantha about this, and she told me to check with Ken. Checked with
Ken regarding this, and he said that we will not be handling internal deps. He
said that maybe down the line we will but not at the moment.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:6: """Provides an interface for installing
Chrome."""
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Is this supposed to be for win only? If so, mention it here

Added a line that mentions that currently the only platform supported is
Windows.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:8: import _winreg
On 2012/08/09 21:32:48, Nirnimesh wrote:
> will fail on non-win

Previously I had a check in here that only imported certain modules like
_winreg, wintypes and windll, etc., if the platform was Windows. However, Ken
requested that I remove that check. He stated that since this will only be run
on Windows, there's no need for that check. So, at his recommendation, I removed
the check. Let me know if you would like me to add that check again.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:21: class InstallationType:
On 2012/08/09 21:32:48, Nirnimesh wrote:
> inherit from object

Done.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:27: class ChromeRegistryValues:
On 2012/08/09 21:32:48, Nirnimesh wrote:
> inherit from object

Done.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:44: A ChromeInstallation object.
On 2012/08/09 21:32:48, Nirnimesh wrote:
> an instance of ChromeInstallation.

Done.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:46: def DoPreliminaryChecks(regedit):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> prefix _

Done.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:47: """Validates the test parameters and Chrome
version.
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Be more specific. Call out exactly what it does.

Updated the docstring, so its more descriptive.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:52: assert(os.path.isfile(installer_path))
On 2012/08/09 21:32:48, Nirnimesh wrote:
> why?

The installer was initially downloaded by a class in chrome_installer called
ChromeInstaller, which downloaded the installer and ran it to install Chrome.
Later on, it was decided that ChromeInstaller shouldn't download the installer;
it should simply run the installer to install Chrome. A lot of the code from
ChromeInstaller was moved to InstallTest, and ChromeInstaller itself was
removed, altogether. This check is just a remnant of the previous code from
ChromeInstaller. Now, since InstallTest downloads the installer, it does its own
check to make sure that the downloaded installer file exists. If not, it raises
an exception, which make this check redundant. So I went ahead and removed it.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:61: if current_type != None:
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove "!= None"

I explicitly added '!= None' because if a Chrome version already exists on the
system, an InstallationType value will be assigned to current_type. Depending on
the type of installation, it could either be InstallationType.SYSTEM or
InstallationType.USER (i.e. 0 or 1). So if I simply say 'if current_type:', it
will evaluate to False even when InstallationType is SYSTEM (i.e. zero). If you
want, I can change the InstallationType values so they are 1 and 2 instead of 0
and 1. If we do that, I can then get rid of the != None. Let me know if you
would like me to do that.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:71: raise RuntimeError('Please specify a
version higher than the one '
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Remove "Please"

Sorry, I was just trying to be courteous. But, rest assure, next time not only
will I not say please, I'll demand that they specify a version higher than the
installed one :)

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:78: cmd = '%s %s' % (installer_path, options)
On 2012/08/09 21:32:48, Nirnimesh wrote:
> either use + to join strings (line 76), or use this format. Not both

Got rid of the +=. I now use string formatting to concatenate the two strings.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:79: ret = subprocess.Popen(cmd,
shell=True).wait()
On 2012/08/09 21:32:48, Nirnimesh wrote:
> make cmd a list. remove shell=True

Made cmd a list and got rid of shell=True.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:80: if ret == 0:
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Invert the logic. Raise first if ret != 0

Inverted the logic. An exception is now raised if ret != 0.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:90: _HKEY_USER =
_HKEY_LOCAL.replace('\\Wow6432Node', '')
On 2012/08/09 21:32:48, Nirnimesh wrote:
> either use r'..' or \\ thoughout.

Changed to r'..'.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:95: """Generates the registry key name for the
specified value.
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Generate -> Get

Done.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:142: b_exists = False
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove "b_"

Got rid of the boolean var, so this no longer applies.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:149: hkey.Close()
On 2012/08/09 21:32:48, Nirnimesh wrote:
> If you just return True from here, you don't even need the boolean var

Done. Got rid of the boolean var.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:241: chrome_path =
os.path.join(self._GetWinLocalFolder(folder_id), 'Google',
On 2012/08/09 21:32:48, Nirnimesh wrote:
> What about Chromium?

Per Anantha, we will not be using Chromium.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py
File install_test/install_test.py (right):

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:4: # found in the LICENSE file.
On 2012/08/09 21:32:48, Nirnimesh wrote:
> docstring.
> 
> Give an example test. Look at the top of pyauto.py

Added a docstring and included instructions on how updater tests can utilize
this framework. As for the example test, I included it in the docstring of
InstallTest, similar to how PyAuto.py provides an example in the PyUITest class.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:29: _OPTIONS = None
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Do not use globals. What you're doing can easily be done without it.

I actually did it this way because I was trying to emulate PyAuto. This is
exactly how PyAuto does it. But if you still want me to change it, let me know
and I'll go ahead and change it.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:46: _dir_prefix = '__CHRBLD__'
On 2012/08/09 21:32:48, Nirnimesh wrote:
> why the mystique name?

We have a particular naming scheme for folders where all the dependencies are
downloaded. It is, _dir_prefix + version_number, so, for example, for version
22.0.1230.0, the folder name will be '__CHRBLD__22.0.1230.0'. The dependencies
are downloaded only once, before firing off the first test. We created this
naming scheme, so subsequent tests can use it to locate the corresponding
dependencies folder in the temp directory. Before we were using tempfile to get
random folder names, but those names had to be kept in a global var, so they
wouldn't be lost when a new instance of InstallTest is created by the next test.
This eliminates the need for global variables.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:49: _installer_name = 'mini_installer.exe'
On 2012/08/09 21:32:48, Nirnimesh wrote:
> So all this is win only? Where did you declare/check it?

Updated the docstring so it states that only Windows is supported. As for
checking it, Ken suggested to remove checks that verify if the current platform
is Windows because, per Ken, these scripts will only be run on Windows.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:62: if not self._DownloadDeps(build):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Do you want the downloading to be done separately for every test?
> 
> Remember that unittest module initializes all tests in one go, and then runs
> their setup/test/tearDown one by one.

No, the downloading will occur only once. This is why we created the mystique
name :)

Each specified build has a corresponding folder in the temp directory that gets
created only once. These folders have a unique name, which ties them to a
particular build. So, for example, let's say you want to test with builds
22.0.1230.0 and 22.0.1231.0. Before the first test can begin, two folders will
get created in the temp directory, the first named __CHRBLD__22.0.1230.0 and the
second named __CHRBLD__22.0.1231.0. One by one, dependencies for each build will
be downloaded into the corresponding folders. The method that downloads all the
dependencies checks if a folder that corresponds with the current build exists
in the temp directory and only downloads if it does not. This will only be the
case for the first testcase. For every subsequent test, the method will check in
the temp directory, determine that the deps folder already exists, and simply do
nothing. We also took into account the fact that something could go wrong during
the
download process. To eliminate the possibility of an incomplete deps folder, we
first download everything into a temp folder with a randomly generated name. If
all download/checkouts are successful, that folder is then renamed using our
naming scheme.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:88: self.failIf(self._pyauto == None)
On 2012/08/09 21:32:48, Nirnimesh wrote:
> call parent's setup

Sorry, I didn't fully understand this comment. The parent class of InstallTest
is unittest.TestCase, and the default implementations of setUp and tearDown do
not do anything. So I didn't quite understand why we should call parent's setup
method here.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:93: self._DeleteBuild()
On 2012/08/09 21:32:48, Nirnimesh wrote:
> call parent's tearDown

The parent class of InstallTest is unittest.TestCase, and the default
implementations of setUp and tearDown do not do anything.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:101: def _Refresh(self):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Refresh? It's not clear from the method name that it does clearing. Use better
> name

Renamed to _ClearModulesRegistry.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:104: del(self._pyauto)
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove parens

Done.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:133: self._pyauto.suite_holder =
pyauto.PyUITestSuite(['test.py'])
On 2012/08/09 21:32:48, Nirnimesh wrote:
> [] would do I think

Yes, it did. Got rid of the arbitrary file name.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:261: def _OnError(self, func, path, exc_info):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> I've seen this elsewhere. Move this and line 252, 280.. to a helper method.

Put line no. 252 in a helper method, and made the callback a nested method
inside the helper method, _DeleteDir.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:286: def _Download(self, dfile, url, dest=None):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> I've seen similar function elsewhere. Reuse code.

The only other method that's somewhat similar is
chrome_checkout._GetContentsAndReturnResponse. I could probably use that, but
the only problem is, in that method, we don't actually download anything; it all
happens in memory. In order to download the file, we'd still have to write
additional code that calls _GetContentsAndReturnResponse and reads the contents
of the installer into a buffer and then writes that to a file. 

I think you probably meant _DoesURLExist because a similar method also exists in
fetch_prebuilt_pyauto. After checking with Nirnimesh, moved DoesUrlExist from
this class and FetchPrebuilt to pyauto_utils.py.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:344: def _GetUnitTests(self):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove 'Unit'

Renamed to _GetTests.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:360: else:
On 2012/08/09 21:32:48, Nirnimesh wrote:
> remove the else part. it's redundant

Removed else.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:363: def _ParseArgs(self):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Move _ParseArgs before _Run

Done.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:372: help='Specifies the chrome-master2 url,
without the build number.')
On 2012/08/09 21:32:48, Nirnimesh wrote:
> do not refer internal URLs/hostname.

Removed the part that referred to an internal URL.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:380: _OPTIONS = opts
On 2012/08/09 21:32:48, Nirnimesh wrote:
> verify the sanity of the args here, esp --builds

Wrote a new method called ValidateArgs, which confirms that all specified builds
have a valid version number and valid urls.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:380: _OPTIONS = opts
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Before firing off the install tests, it'd be nice to determine if the
individual
> chrome builds are sane by themselves and can run pyauto test.

I am not entirely sure how we can confirm that the builds can run pyauto tests.
I checked with Ken and he recommended that one solution would be to install the
build and run a simple pyauto test with it (maybe test_basic). I don't know if
that's what you had in mind? If not, I'd welcome any suggestions.

http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_p...
File pyautolib/fetch_prebuilt_pyauto.py (right):

http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:130: def OnError(func, path, exc_info):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> _OnError

Done.

http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:144: except (OSError, IOError):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> Since you're just re-raising the exception, the try block is a no-op.
> Remove the try/except block.

Got rid of the try/except block.

http://codereview.chromium.org/10384104/diff/73002/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:240: def _ParseArgs(self):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> I think it's better to move _ParseArgs to the above class too. then main will
> just call Run() on it.

I moved the Args here at Ken's recommendation. The reason is, we use the
FetchPrebuilt class to fetch the builds specified by the user. We don't run this
script; we instantiate FetchPrebuilt and use the object to download the builds.
FetchPrebuilt constructor was also updated so it accepts arguments. If we're
just using a FetchPrebuilt object to do the download, there's no reason to parse
the arguments because the arguments are passed directly to the class
constructor. This is why we separated the command parsing into a separate class,
so this script can do both. You can instantiate FetchPrebuilt or run it as a
standalone script.

http://codereview.chromium.org/10384104/diff/73002/pyautolib/pyauto.py
File pyautolib/pyauto.py (right):

http://codereview.chromium.org/10384104/diff/73002/pyautolib/pyauto.py#newcod...
pyautolib/pyauto.py:121: def __init__(self, methodName='runTest', **kwargs):
On 2012/08/09 21:32:48, Nirnimesh wrote:
> why not add browser_path arg here?

I included it in kwargs at Ken's recommendation. Adding a separate broswer_path
argument might confuse users. Up until now, users didn't have to worry about
providing the browser path, as initialization was always done by calling
BrowserPath. We added a broser_path variable so we could provide a path for the
Chrome binary if need be, which would be the case for the updater tests where
the Chrome binary and pyautolib files are in different locations (see comment
below for why they are in different places). At the same time, we didn't want to
change things for other tests that 'fetch' a chrome build and run pyauto tests
using the fetched build. So kwargs seemed like a natural choice. If you feel
that we should not include it in kwargs, let me know and I'll change it.

http://codereview.chromium.org/10384104/diff/73002/pyautolib/pyauto.py#newcod...
pyautolib/pyauto.py:138: browser_path = kwargs.get('browser_path', None)
On 2012/08/09 21:32:48, Nirnimesh wrote:
> So BrowserPath() method will be out of sync?

BrowserPath returns the same path as the pyautolib file, which makes sense if
you're fetching a build because then the binary and pyautolib are in the same
location. But we wanted to run the tests using the installed version of Chrome,
which is either in User\AppData\Local\Google\Chrome\Application <or> Program
Files(x86)\Google\Chrome\Application, depending on the type of installation. At
first we decided to just download the pyautolib files in the same directory as
the Chrome binary, but that didn't work. The initial installation would work,
but upgrade would fail because the pyautolib files, due to being in use, would
cause an access violation error, causing the upgrade to fail. So ultimately we
decided to download pyautolib and all other dependencies in the Temp folder.
That's why we created the browser_path variable, which, if set, would provide
the location for the Chrome binary. If browser_path is not set, BrowserPath
would be used for initialization. So, to answer you question, BrowserPath would
be out of sync if it were used, but it wouldn't even be used in this case. For
tests that don't need to specify browser_path, they can simply ignore the
browser_path arg. In which case, broser_path will be None and BrowserPath() will
be used for initialization.

Nirnimesh

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py File install_test/chrome_installer.py (right): http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode8 install_test/chrome_installer.py:8: import _winreg On 2012/08/16 23:46:24, nkang wrote: > On ...

8 years, 4 months ago (2012-08-22 07:06:34 UTC) #32

nkang

Address all comments made by Nirnimesh. http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py File install_test/chrome_installer.py (right): http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_installer.py#newcode61 install_test/chrome_installer.py:61: if current_type != ...

8 years, 4 months ago (2012-08-24 22:45:26 UTC) #33

Address all comments made by Nirnimesh.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:61: if current_type != None:
On 2012/08/22 07:06:34, Nirnimesh wrote:
> On 2012/08/16 23:46:24, nkang wrote:
> > On 2012/08/09 21:32:48, Nirnimesh wrote:
> > > remove "!= None"
> > 
> > I explicitly added '!= None' because if a Chrome version already exists on
the
> > system, an InstallationType value will be assigned to current_type.
Depending
> on
> > the type of installation, it could either be InstallationType.SYSTEM or
> > InstallationType.USER (i.e. 0 or 1). So if I simply say 'if current_type:',
it
> > will evaluate to False even when InstallationType is SYSTEM (i.e. zero). If
> you
> > want, I can change the InstallationType values so they are 1 and 2 instead
of
> 0
> > and 1. If we do that, I can then get rid of the != None. Let me know if you
> > would like me to do that.
> 
> That sounds bizarre. Why not add another installation type NOT_INSTALLED = 0?

Added a third variable called NOT_INSTALLED and set its value to 0. SYSTEM and
USER will be 1 and 2, respectively.

http://codereview.chromium.org/10384104/diff/73002/install_test/chrome_instal...
install_test/chrome_installer.py:241: chrome_path =
os.path.join(self._GetWinLocalFolder(folder_id), 'Google',
On 2012/08/22 07:06:34, Nirnimesh wrote:
> On 2012/08/16 23:46:24, nkang wrote:
> > On 2012/08/09 21:32:48, Nirnimesh wrote:
> > > What about Chromium?
> > 
> > Per Anantha, we will not be using Chromium.
> 
> Do you declare this somewhere in the docs?


I checked with Ken on this, and he advised to declare it in the docstring of
this method. So I went ahead and updated the docstring and mentioned that it
only works for Chrome binary.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test.py
File install_test/install_test.py (right):

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:29: _OPTIONS = None
On 2012/08/22 07:06:34, Nirnimesh wrote:
> On 2012/08/16 23:46:24, nkang wrote:
> > On 2012/08/09 21:32:48, Nirnimesh wrote:
> > > Do not use globals. What you're doing can easily be done without it.
> > 
> > 
> > I actually did it this way because I was trying to emulate PyAuto. This is
> > exactly how PyAuto does it. But if you still want me to change it, let me
know
> > and I'll go ahead and change it.
> 
> globals should not be used unless absolutely necessary.
> pyauto did not have that luxury, but this class might be able to do without
it.


Per our conversation, I created a 'staticmethod' in InstallTest called
SetOptions. Main, which parses the command line args, calls SetOptions to set
the test options for InstallTest. This will eliminate the need for a global
variable. After implementing this change, I got rid of the global _OPTIONS
variable.

http://codereview.chromium.org/10384104/diff/73002/install_test/install_test....
install_test/install_test.py:380: _OPTIONS = opts
On 2012/08/22 07:06:34, Nirnimesh wrote:
> On 2012/08/16 23:46:24, nkang wrote:
> > On 2012/08/09 21:32:48, Nirnimesh wrote:
> > > Before firing off the install tests, it'd be nice to determine if the
> > individual
> > > chrome builds are sane by themselves and can run pyauto test.
> > 
> > I am not entirely sure how we can confirm that the builds can run pyauto
> tests.
> > I checked with Ken and he recommended that one solution would be to install
> the
> > build and run a simple pyauto test with it (maybe test_basic). I don't know
if
> > that's what you had in mind? If not, I'd welcome any suggestions.
> 
> Yes, running a test from test_basic would be fine.

After taking everything into consideration, I think this would require a lot of
work. First of all, this would require changes to the checkout script, as we
would need to download the 'functional' folder, which contains test_basic.
Second, since we're launching a PyAuto test, we need to come up with a way to
tell PyAuto where the Chrome binary is. InstallTest overcomes this problem by
creating a PyUITest object and passing the browser path to the class constructor
as a parameter. However, in the case of test_basic, since we're just firing off
a PyAuto test, we don't have that luxury. So we would need to figure out how to
tell PyAuto where the Chrome binary is, as, by default, it would expect it to be
in the same place as the PyAuto binaries. I talked to Ken and Anantha about
this, and they suggested that maybe we should just punt this for now. We can
create another CL for this down the line. Also, Anantha is considering using
ChromeDriver instead of PyAuto, so this may no longer even apply, if we decide
to go with ChromeDriver. I think we should hold off on this, at least until
Anantha makes her decision. What do you think?

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
File install_test/chrome_checkout.py (right):

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:30: def _SetConfiguration():
On 2012/08/22 07:06:35, Nirnimesh wrote:
> rename -> _SetupLogging

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:49: buff = url_opener.read()
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Don't use cryptic var names.
> 
> buff -> data

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:61:
'http://src.chromium.org/viewvc/chrome/releases/%s/DEPS' % version)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> shouldn't part of this URL also be declared as a constant at the top like line
> 25?

Created a new constant called _CHROME_URL and updated this method accordingly.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:93: match = re.search('\'src\':[\n\r
]+\'(.*?)\'', deps)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Give an example of the pattern you're trying to match in a comment

Added a comment that specifies what this statement is trying to match.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:99: match =
re.search("""['"]src['"].*?:.*?['"]/branches/(.*?)/.*?,""",
On 2012/08/22 07:06:35, Nirnimesh wrote:
> ditto

Added a comment that specifies what this statement is trying to match.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:109: """Gets selenium/pyftpdlib rev. number by
parsing contents of DEPS file.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> rev. -> revision

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:130: def _SvnCo(path, revision=None, dest=None):
On 2012/08/22 07:06:35, Nirnimesh wrote:
> _SvnCheckout

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:138: cmd = 'svn co'
On 2012/08/22 07:06:35, Nirnimesh wrote:
> co -> checkout

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:141: cmd += ' %s' % path
On 2012/08/22 07:06:35, Nirnimesh wrote:
> remove %s. It's redundant

We need to add a space between the strings we're concatenating, both here and in
the comment below. I am guessing this is why Anthony did it this way. If you
want I can do something like:

cmd += ' ' + path

Or I can add a space above (cmd += ' --revision %d ' % revision). That way we
can eliminate the %s here, but we'd still need to either add %s to the comment
below or concatenate the two string using: cmd += ' ' + dest

Let me know if you would like me to change this.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:143: cmd += ' %s' % dest
On 2012/08/22 07:06:35, Nirnimesh wrote:
> ditto

See response above.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:157: if type(version) == str:
On 2012/08/22 07:06:35, Nirnimesh wrote:
> isinstance(version, basestring)

Rewrote the method as:
return isinstance(...) and re.match(...)

So this no longer applies.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:158: match = re.match('\d+\.\d+\.\d+\.\d+',
version)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> remove the match var. and move the if on this line

Rewrote the method as:
return isinstance(...) and re.match(...)

So this no longer applies.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:161: return False
On 2012/08/22 07:06:35, Nirnimesh wrote:
> This whole function can be written as:
> 
> return isinstance(...) and re.match(...)

Rewrote the method as:
return isinstance(...) and re.match(...)

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:178: # If its a patch, check out the branch.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> its -> it's

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:178: # If its a patch, check out the branch.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> check out -> checkout

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_checko...
install_test/chrome_checkout.py:182: else:
On 2012/08/22 07:06:35, Nirnimesh wrote:
> I don't like this. But whatever.

Maybe we can change this in the next CL.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
File install_test/chrome_installer.py (right):

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:8: At present the only platform it supports is
Windows.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> rename the file to append _win to the name. You'll not remember it 1 month
> later.

Changed name to chrome_installer_win.py. Updated other scripts that used this
installer accordingly.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:26: SYSTEM = 0
On 2012/08/22 07:06:35, Nirnimesh wrote:
> If you're going to do if calls on this, make it non-zero

Done. Added a third variable called NOT_INSTALLED. Zero was assigned to
NOT_INSTALLED. SYSTEM and USER are assigned values 1 and 2, respectively.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:44: options: Any additional installation
options.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> make it a list instead of having to split by ' ' at line 84

Changed options to a list. For this I also had to update InstallTest. Since
InstallTest parses the command args and assigns the Chrome options specified to
a string. I had to convert that string to a list as well.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:44: options: Any additional installation
options.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> remove 'Any'

Removed 'Any'.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:69: if current_type != None:
On 2012/08/22 07:06:35, Nirnimesh wrote:
> remove "!= None"

Changed the InstallationType class and created a third type called
NOT_INSTALLED, which is represented by zero. SYSTEM is now 1, and USER is 2. So
I was able to get rid of '!= None'. Now if Chrome is not installed, current_type
will be assigned InstallationType.NOT_INSTALLED , which will evaluate to False.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:71: if (current_type == InstallationType.SYSTEM
and install_type ==
On 2012/08/22 07:06:35, Nirnimesh wrote:
> move install_type == to the next line

Moved 'install_type' to the next line.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:84: options = '%s %s' % (options, '--install
--do-not-launch-chrome')
On 2012/08/22 07:06:35, Nirnimesh wrote:
> remove the last %s and move the strings there directly

Got rid of the last %s and moved the strings there directly.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:88: ret = subprocess.Popen(args).wait()
On 2012/08/22 07:06:35, Nirnimesh wrote:
> use a better varname.
> It looks like you don't even need one here. Move the if to this line instead

Got rid of this arg. Moved the 'if' statement to that line instead.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:101: _chrome_args =
r'ClientState\{8A69D345-D564-463C-AFF1-A69D9E530F96}'
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Why is this lower case, whereas line 98,99 are in upper case?

It's just because the names HKEY_USER and HKEY_LOCAL are more inline with the
Windows registry naming convention. But I can see that this is causing some
confusion, so I went ahead and changed them to lower case, as well.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:154: hkey = _winreg.OpenKey(key, key_name)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> only this line needs to be in the try block right?
> Move the rest out

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:273: uninstall_str = self._GetUninstallString()
On 2012/08/22 07:06:35, Nirnimesh wrote:
> does this not contain .exe?

The UninstallString contains the path to the installer, which is an executable.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:281: subprocess.Popen(cmd, shell=True).wait()
On 2012/08/22 07:06:35, Nirnimesh wrote:
> subprocess.call()

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/chrome_instal...
install_test/chrome_installer.py:283: logging.log(logging.INFO, 'Chrome was
uninstalled successfully...')
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Instead of using logging.log(logging.INFO, ...), use logging.info(..)
> Change everywhere

Updated everywhere logging.log was used in chrome_installer.py,
chrome_checkout.py, and install_test.py.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test.py
File install_test/install_test.py (right):

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:9: allows users to run pyauto tests using the
installed version. User and system
On 2012/08/22 07:06:35, Nirnimesh wrote:
> pyauto -. PyAuto

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:11: pyauto tests. Currently the only platform
that's supported is Windows.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> ditto

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:68:
self.assertFalse(self._pyauto.GetProtectorState()['showing_change'])
On 2012/08/22 07:06:35, Nirnimesh wrote:
> include the __name__ == '__main__' portion as well.
> 
> Give an example command line to use where you demonstrate the options

I had initially included the __name__=='__main__' portion in docstring above (at
module level). But since you want me to add it here, I moved it from there to
here. Also added and example command line.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:71: _build_iterator = None
On 2012/08/22 07:06:35, Nirnimesh wrote:
> This is a long list of member vars. At least some of them merit comments.

Yes they do, and now they have comments.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:116: self.failIf(self._pyauto == None)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> use self.assertTrue(self._pyauto)

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:123: def _GetPlatform(self):
On 2012/08/22 07:06:35, Nirnimesh wrote:
> @staticmethod

Changed it to staticmethod and changed the name from _GetPlatform to
GetPlatform.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:129: def _ClearModulesRegistry(self):
On 2012/08/22 07:06:35, Nirnimesh wrote:
> what do you mean by Registry?
> Rename to _UnloadPyAutoModules

I meant the modules registry (sys.modules), which is a dictionary that contains
all the loaded modules. But I'll go ahead and rename it.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:130: """Deletes the PyUITest object and clears the
modules registry."""
On 2012/08/22 07:06:35, Nirnimesh wrote:
> and update this line

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:137: def _RemovePaths(self):
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Remove -> Restore to match reality

Done. 'To match reality', I like that ;)

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:157: import pyauto
On 2012/08/22 07:06:35, Nirnimesh wrote:
> only this line should be in the try block

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:164: logging.log(logging.ERROR, err)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> logging.error()

Changed here and elsewhere.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:169: sys.path.insert(0, self._current_location)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Why does it need to be in the front?

Just an extra precaution to make sure the correct version of pyauto and
pyautolib are imported.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:188: """Installs the second Chrome build specified
in the command args."""
On 2012/08/22 07:06:35, Nirnimesh wrote:
> command -> command line

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:195: """Checks if specified files/folders exist at
specified 'root' folder.
On 2012/08/22 07:06:35, Nirnimesh wrote:
> 'root' folder -> location

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:204: return all(map(lambda p: os.path.exists(p) and
True or False,
On 2012/08/22 07:06:35, Nirnimesh wrote:
> os.path.exists(p) already returns a boolean. Rewrite as:
> 
> return all([os.path.exists(os.path.join(root, x)) for x in items]) 

Done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:235: else:
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Remove else:
> and left-indent the next line.

Done and done.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:264: location = os.path.join('%s', '%s%s') %
(tempfile.gettempdir(),
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Either use string formattting to join (like previous line) or use os.path.join
> (like this line). Don't mix.

I mixed it because the last two strings (self._dir_prefix and build) need to be
concatenated before I call os.path.join to join it with tempfile.gettempdir().
Since you don't want me to mix the two, I created a new variable called
dir_name. I concatenate the last two strings and assign them to dir_name. Then I
call os.path.join(tempdir.gettempdir(), dir_name). This way we don't have to mix
the two.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:330: d = urllib.urlretrieve(file_url, filename)
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Don't use one-letter varnames.

Changed var name to dfile (as in downloaded file), since file is a keyword.

http://codereview.chromium.org/10384104/diff/76002/install_test/install_test....
install_test/install_test.py:393: print >>sys.stderr, ('Tests can be disabled by
editing %s. Ref: %s'
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Really? I don't see any PYAUTO_TESTS file in this CL.

We haven't made any changes to PYAUTO_TESTS. That's why I didn't see a point in
including it in this CL. No big deal, though, I'll go ahead and change the
comment.

http://codereview.chromium.org/10384104/diff/76002/pyautolib/fetch_prebuilt_p...
File pyautolib/fetch_prebuilt_pyauto.py (right):

http://codereview.chromium.org/10384104/diff/76002/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:213: class Main:
On 2012/08/22 07:06:35, Nirnimesh wrote:
> derive from object

Done.

http://codereview.chromium.org/10384104/diff/76002/pyautolib/fetch_prebuilt_p...
pyautolib/fetch_prebuilt_pyauto.py:257: sys.exit(Main())
On 2012/08/22 07:06:35, Nirnimesh wrote:
> Please ensure that this script continues to work for the current use case.

Yes it does. We just separated the command parsing from the rest of the
functionality because we wanted to create a FetchPrebuilt object in InstallTest
and use that object to 'fetch' the build, rather than using this script as a
standalone script. So by separating the command parsing we can do both. Now this
script can be run as standalone, just as it is done for the current use case, or
you can instantiate FetchPrebuilt and use the instance to fetch the build.

Nirnimesh

http://codereview.chromium.org/10384104/diff/85002/install_test/protector_updater.py File install_test/protector_updater.py (right): http://codereview.chromium.org/10384104/diff/85002/install_test/protector_updater.py#newcode1 install_test/protector_updater.py:1: #!/usr/bin/env python Anantha told me yesterday that she doesn't ...

8 years, 4 months ago (2012-08-24 22:51:34 UTC) #34

nkang

Addressed the recent comment by Nirnimesh. http://codereview.chromium.org/10384104/diff/85002/install_test/protector_updater.py File install_test/protector_updater.py (right): http://codereview.chromium.org/10384104/diff/85002/install_test/protector_updater.py#newcode1 install_test/protector_updater.py:1: #!/usr/bin/env python On ...

8 years, 4 months ago (2012-08-24 23:00:42 UTC) #35

Nirnimesh

On 2012/08/24 23:00:42, nkang wrote: > Addressed the recent comment by Nirnimesh. > > http://codereview.chromium.org/10384104/diff/85002/install_test/protector_updater.py ...

8 years, 4 months ago (2012-08-24 23:03:52 UTC) #36

nkang

Updated the framework so it uses ChromeDriver instead of PyAuto. Modified install_test.py to reflect this ...

8 years, 2 months ago (2012-09-26 21:39:38 UTC) #37

kkania

https://chromiumcodereview.appspot.com/10384104/diff/99001/install_test/chrome_installer_win.py File install_test/chrome_installer_win.py (right): https://chromiumcodereview.appspot.com/10384104/diff/99001/install_test/chrome_installer_win.py#newcode30 install_test/chrome_installer_win.py:30: def Install(installer_path, install_type, build, options): change build->version or build_no ...

8 years, 2 months ago (2012-10-02 17:05:16 UTC) #38

nkang

Addressed all comments made by Ken. https://chromiumcodereview.appspot.com/10384104/diff/99001/install_test/chrome_installer_win.py File install_test/chrome_installer_win.py (right): https://chromiumcodereview.appspot.com/10384104/diff/99001/install_test/chrome_installer_win.py#newcode30 install_test/chrome_installer_win.py:30: def Install(installer_path, install_type, ...

8 years, 2 months ago (2012-10-03 22:12:01 UTC) #39

nkang

Adding sky for owner's review for the addition of a new folder called 'install_test'. Adding ...

8 years, 2 months ago (2012-10-04 20:56:03 UTC) #41

sky

I'm swapping myself with John as a reviewer. John is more knowledgeable about watching what's ...

8 years, 2 months ago (2012-10-04 21:27:49 UTC) #42

dennis_jeffrey

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py File pyautolib/pyauto_utils.py (right): http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py#newcode201 pyautolib/pyauto_utils.py:201: def DoesUrlExist(url): Something similar already exists in other pyauto ...

8 years, 2 months ago (2012-10-04 21:54:46 UTC) #43

nkang

Addressed Dennis' comments regarding pyautolib. http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py File pyautolib/pyauto_utils.py (right): http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py#newcode201 pyautolib/pyauto_utils.py:201: def DoesUrlExist(url): On 2012/10/04 ...

8 years, 2 months ago (2012-10-04 22:25:57 UTC) #44

Addressed Dennis' comments regarding pyautolib.

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py
File pyautolib/pyauto_utils.py (right):

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py...
pyautolib/pyauto_utils.py:201: def DoesUrlExist(url):
On 2012/10/04 21:54:46, dennis_jeffrey wrote:
> Something similar already exists in other pyauto code here:
> 
>
http://src.chromium.org/svn/trunk/src/chrome/test/pyautolib/fetch_prebuilt_py...
> 
> Can we make sure this utility function only gets implemented once in pyauto?

Originally FetchPrebuiltPyauto was part of this CL, and that's part of the
reason I moved this method to pyautolib so it wouldn't be repeated.  Then, after
we switched from PyAuto to ChromeDriver, we dropped FetchPrebuiltPyauto from the
CL because it was no longer needed. I can remove this method from
FetchPrebuiltPyauto, update it, and include it in this CL. Let me know if you're
okay with that.

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py...
pyautolib/pyauto_utils.py:225: class
_GTestTextTestResult(unittest._TextTestResult):
On 2012/10/04 21:54:46, dennis_jeffrey wrote:
> This code looks like it already exists for webdriver here:
> 
>
http://src.chromium.org/svn/trunk/src/chrome/test/webdriver/test/py_unittest_...
> 
> Can we find a single place to put it so that we don't have two copies of this
> floating around in the codebase?

This code also exists in PyAuto.py. PyAuto was originally part of this CL, as
well, but was later dropped in favor of ChromeDriver. I had removed this code
from PyAuto and put it here. I had also updated PyAuto so it imported this class
from pyautolib, but since PyAuto was dropped, we decided to remove it from the
CL. If you want, I can update PyAuto and remove this code from it. I can also
update Webdriver, so it imports from pyautolib. I think that would be better
than having separate copies of this code in PyAuto and Webdriver. Let me know if
you would like me to do that. If you have another place in mind, I'd be open to
that as well.

dennis_jeffrey

Thanks for giving the back-story about how things got to the current state. http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py File ...

8 years, 2 months ago (2012-10-04 22:37:14 UTC) #45

Thanks for giving the back-story about how things got to the current state.

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py
File pyautolib/pyauto_utils.py (right):

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py...
pyautolib/pyauto_utils.py:201: def DoesUrlExist(url):
On 2012/10/04 22:25:58, nkang wrote:
> On 2012/10/04 21:54:46, dennis_jeffrey wrote:
> > Something similar already exists in other pyauto code here:
> > 
> >
>
http://src.chromium.org/svn/trunk/src/chrome/test/pyautolib/fetch_prebuilt_py...
> > 
> > Can we make sure this utility function only gets implemented once in pyauto?
> 
> Originally FetchPrebuiltPyauto was part of this CL, and that's part of the
> reason I moved this method to pyautolib so it wouldn't be repeated.  Then,
after
> we switched from PyAuto to ChromeDriver, we dropped FetchPrebuiltPyauto from
the
> CL because it was no longer needed. I can remove this method from
> FetchPrebuiltPyauto, update it, and include it in this CL. Let me know if
you're
> okay with that.

Yeah, I think it would be good to remove the method from fetch_prebuilt_pyauto
and have it use the one here instead.  Is the one here identical to the one in
fetch_prebuilt_pyauto?

fetch_prebuilt_pyauto already imports the current file so we'd might as well
have it use this one rather than having its own copy ;-)

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py...
pyautolib/pyauto_utils.py:225: class
_GTestTextTestResult(unittest._TextTestResult):
On 2012/10/04 22:25:58, nkang wrote:
> On 2012/10/04 21:54:46, dennis_jeffrey wrote:
> > This code looks like it already exists for webdriver here:
> > 
> >
>
http://src.chromium.org/svn/trunk/src/chrome/test/webdriver/test/py_unittest_...
> > 
> > Can we find a single place to put it so that we don't have two copies of
this
> > floating around in the codebase?
> 
> This code also exists in PyAuto.py. PyAuto was originally part of this CL, as
> well, but was later dropped in favor of ChromeDriver. I had removed this code
> from PyAuto and put it here. I had also updated PyAuto so it imported this
class
> from pyautolib, but since PyAuto was dropped, we decided to remove it from the
> CL. If you want, I can update PyAuto and remove this code from it. I can also
> update Webdriver, so it imports from pyautolib. I think that would be better
> than having separate copies of this code in PyAuto and Webdriver. Let me know
if
> you would like me to do that. If you have another place in mind, I'd be open
to
> that as well.

I would like to see only 1 copy of this code in the chrome codebase, but it does
complicate matters that pyauto needs it and ChromeDriver needs it.  Not sure if
it's best to have WebDriver depend on pyautolib though.  It seems like it would
be better to have a more general test utility module that can be imported and
used by both chromedriver and pyauto.  Maybe Ken could comment on that one since
he knows more about the connections between pyauto and chromedriver.

kkania

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py File pyautolib/pyauto_utils.py (right): http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py#newcode225 pyautolib/pyauto_utils.py:225: class _GTestTextTestResult(unittest._TextTestResult): On 2012/10/04 22:37:14, dennis_jeffrey wrote: > On ...

8 years, 2 months ago (2012-10-05 15:17:41 UTC) #46

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py
File pyautolib/pyauto_utils.py (right):

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py...
pyautolib/pyauto_utils.py:225: class
_GTestTextTestResult(unittest._TextTestResult):
On 2012/10/04 22:37:14, dennis_jeffrey wrote:
> On 2012/10/04 22:25:58, nkang wrote:
> > On 2012/10/04 21:54:46, dennis_jeffrey wrote:
> > > This code looks like it already exists for webdriver here:
> > > 
> > >
> >
>
http://src.chromium.org/svn/trunk/src/chrome/test/webdriver/test/py_unittest_...
> > > 
> > > Can we find a single place to put it so that we don't have two copies of
> this
> > > floating around in the codebase?
> > 
> > This code also exists in PyAuto.py. PyAuto was originally part of this CL,
as
> > well, but was later dropped in favor of ChromeDriver. I had removed this
code
> > from PyAuto and put it here. I had also updated PyAuto so it imported this
> class
> > from pyautolib, but since PyAuto was dropped, we decided to remove it from
the
> > CL. If you want, I can update PyAuto and remove this code from it. I can
also
> > update Webdriver, so it imports from pyautolib. I think that would be better
> > than having separate copies of this code in PyAuto and Webdriver. Let me
know
> if
> > you would like me to do that. If you have another place in mind, I'd be open
> to
> > that as well.
> 
> I would like to see only 1 copy of this code in the chrome codebase, but it
does
> complicate matters that pyauto needs it and ChromeDriver needs it.  Not sure
if
> it's best to have WebDriver depend on pyautolib though.  It seems like it
would
> be better to have a more general test utility module that can be imported and
> used by both chromedriver and pyauto.  Maybe Ken could comment on that one
since
> he knows more about the connections between pyauto and chromedriver.

It would be nice to just have one copy. I'll take this though in a separate CL,
since it's really my problem, not Navdeep's.

jam

thanks for switching to chromedriver, lgtm (didn't review in detail) please add an owners file ...

8 years, 2 months ago (2012-10-05 17:22:30 UTC) #47

dennis_jeffrey

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py File pyautolib/pyauto_utils.py (right): http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py#newcode201 pyautolib/pyauto_utils.py:201: def DoesUrlExist(url): On 2012/10/04 22:37:14, dennis_jeffrey wrote: > On ...

8 years, 2 months ago (2012-10-05 17:57:23 UTC) #48

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py
File pyautolib/pyauto_utils.py (right):

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py...
pyautolib/pyauto_utils.py:201: def DoesUrlExist(url):
On 2012/10/04 22:37:14, dennis_jeffrey wrote:
> On 2012/10/04 22:25:58, nkang wrote:
> > On 2012/10/04 21:54:46, dennis_jeffrey wrote:
> > > Something similar already exists in other pyauto code here:
> > > 
> > >
> >
>
http://src.chromium.org/svn/trunk/src/chrome/test/pyautolib/fetch_prebuilt_py...
> > > 
> > > Can we make sure this utility function only gets implemented once in
pyauto?
> > 
> > Originally FetchPrebuiltPyauto was part of this CL, and that's part of the
> > reason I moved this method to pyautolib so it wouldn't be repeated.  Then,
> after
> > we switched from PyAuto to ChromeDriver, we dropped FetchPrebuiltPyauto from
> the
> > CL because it was no longer needed. I can remove this method from
> > FetchPrebuiltPyauto, update it, and include it in this CL. Let me know if
> you're
> > okay with that.
> 
> Yeah, I think it would be good to remove the method from fetch_prebuilt_pyauto
> and have it use the one here instead.  Is the one here identical to the one in
> fetch_prebuilt_pyauto?
> 
> fetch_prebuilt_pyauto already imports the current file so we'd might as well
> have it use this one rather than having its own copy ;-)

Please address this comment and then it'll be good to go from my end.

http://codereview.chromium.org/10384104/diff/106001/pyautolib/pyauto_utils.py...
pyautolib/pyauto_utils.py:225: class
_GTestTextTestResult(unittest._TextTestResult):
On 2012/10/05 15:17:41, kkania wrote:
> On 2012/10/04 22:37:14, dennis_jeffrey wrote:
> > On 2012/10/04 22:25:58, nkang wrote:
> > > On 2012/10/04 21:54:46, dennis_jeffrey wrote:
> > > > This code looks like it already exists for webdriver here:
> > > > 
> > > >
> > >
> >
>
http://src.chromium.org/svn/trunk/src/chrome/test/webdriver/test/py_unittest_...
> > > > 
> > > > Can we find a single place to put it so that we don't have two copies of
> > this
> > > > floating around in the codebase?
> > > 
> > > This code also exists in PyAuto.py. PyAuto was originally part of this CL,
> as
> > > well, but was later dropped in favor of ChromeDriver. I had removed this
> code
> > > from PyAuto and put it here. I had also updated PyAuto so it imported this
> > class
> > > from pyautolib, but since PyAuto was dropped, we decided to remove it from
> the
> > > CL. If you want, I can update PyAuto and remove this code from it. I can
> also
> > > update Webdriver, so it imports from pyautolib. I think that would be
better
> > > than having separate copies of this code in PyAuto and Webdriver. Let me
> know
> > if
> > > you would like me to do that. If you have another place in mind, I'd be
open
> > to
> > > that as well.
> > 
> > I would like to see only 1 copy of this code in the chrome codebase, but it
> does
> > complicate matters that pyauto needs it and ChromeDriver needs it.  Not sure
> if
> > it's best to have WebDriver depend on pyautolib though.  It seems like it
> would
> > be better to have a more general test utility module that can be imported
and
> > used by both chromedriver and pyauto.  Maybe Ken could comment on that one
> since
> > he knows more about the connections between pyauto and chromedriver.
> 
> It would be nice to just have one copy. I'll take this though in a separate
CL,
> since it's really my problem, not Navdeep's.

Sounds good - thanks!

nkang

On 2012/10/05 17:22:30, John Abd-El-Malek wrote: > thanks for switching to chromedriver, lgtm (didn't review ...

8 years, 2 months ago (2012-10-05 18:57:12 UTC) #49

nkang

Addressed the comment made by Dennis. Removed the duplicate code from fetch_prebuilt_pyauto.py and updated it ...

8 years, 2 months ago (2012-10-05 18:58:02 UTC) #50