test-fixet: Automated VMWare test of AutoUpdate

test-fixet: Automated VMWare test of AutoUpdate

Fully automated test that uses vmplayer to test updating from
dev-channel -> image-under-test -> rollback to dev-channel.

Also, change to vmware image generation so that vmware images are
generated with two 1-GiB sized root partitions

[adlr, 2010-03-27 03:47:53]

Robert,

I hope you don't mind me giving you a big CL of python, but you seem to know
python, which as you can probably see in my code, I don't. So hopefully you can
provide good feedback.

If you're not good at python feel free to let me know and I'll find someone
else.

Thanks,
-andrew

[rginda, 2010-03-29 02:08:30]

LGTM with a few comments on Pythonisms.  Nothing major though.

http://codereview.chromium.org/1512001/diff/1/2
File src/platform/memento_softwareupdate/autoupdated_vm_test.py (right):

http://codereview.chromium.org/1512001/diff/1/2#newcode1
src/platform/memento_softwareupdate/autoupdated_vm_test.py:1: #!/usr/bin/python
Is this going to be an autotest test?  If so it should probably move into
third_party/autotest/files/client/site-tests/...

If it is going to become part of the autotest portion of the codebase, that
means that PEP8 applies (http://www.python.org/dev/peps/pep-0008/), which calls
for methods to be lowercase_and_underscored rather than WordCapped.  It also
specifies 4-space indents, rather than two.

I've also heard that autotest deviates from PEP8 in that it recommends putting
multiple imports in one line, as in

  import os, re, signal, socket, ...

If it's not moving into autotest/, feel free to ignore all that :)

http://codereview.chromium.org/1512001/diff/1/2#newcode45
src/platform/memento_softwareupdate/autoupdated_vm_test.py:45: class TestState:

http://docs.python.org/reference/datamodel.html#newstyle

Python has 'old style' classes and 'new style'.  In general, stick with
new-style classes, which means inheriting from 'object' or something that
inherits from it.

http://codereview.chromium.org/1512001/diff/1/2#newcode53
src/platform/memento_softwareupdate/autoupdated_vm_test.py:53: range(6)

http://docs.python.org/library/stdtypes.html#xrange-type

Using range here makes python create a list of the numbers 0 through 5 and then
copy that list.  xrange initializes a generator-like object that is O(1) in
space.  In this case the difference is minimal, but xrange is more idiomatic.

http://codereview.chromium.org/1512001/diff/1/2#newcode70
src/platform/memento_softwareupdate/autoupdated_vm_test.py:70: print 'SetState('
+ str(state) + ')'
http://docs.python.org/library/stdtypes.html#string-formatting

The python idiom for this is...

  print 'SetState(%s)' % state

Notice that you don't have to use str(state) here, because Python will convert
the argument for you.  Use a list to format multiple values...

  print 'a: %s, b: %s' % (a, b)

Modulo is a general string formatting operator in python and not specific to the
print statement, so you use it anywhere a string appears.

Also, if this is to become autotesty, then you probably want to use logging.info
or logging.debug instead of print.

http://codereview.chromium.org/1512001/diff/1/2#newcode155
src/platform/memento_softwareupdate/autoupdated_vm_test.py:155: if ret != None:
http://www.corp.google.com/eng/doc/pyguide.xml?showone=True/False_evaluations...

It's more pythonic to use the implicit conversion to boolean in tests, as in 'if
not ret:', although in this case I think you want 'if ret is not None', or swap
the logic and go for 'if ret is None:'

http://codereview.chromium.org/1512001/diff/1/2#newcode175
src/platform/memento_softwareupdate/autoupdated_vm_test.py:175: if self.timer !=
None:
When comparing against objects and None, "is" and "is not" are typically used,
since they skip type coercion (or at least, I think that's why.)

http://codereview.chromium.org/1512001/diff/1/2#newcode189
src/platform/memento_softwareupdate/autoupdated_vm_test.py:189:
HTTPServer.__init__(self, ip_port, klass)
http://docs.python.org/library/functions.html#super

It's considered more matintainable to use super(C, self) than to directly refer
to your superclass' name, so this turns into...

  super(AUTHTTPServer, self).__init__(...)

[adlr, 2010-03-30 03:44:15]

Not checked in yet b/c I have a question below about strings...

please have a quick look.

Thanks,

http://codereview.chromium.org/1512001/diff/1/2
File src/platform/memento_softwareupdate/autoupdated_vm_test.py (right):

http://codereview.chromium.org/1512001/diff/1/2#newcode1
src/platform/memento_softwareupdate/autoupdated_vm_test.py:1: #!/usr/bin/python
On 2010/03/29 02:08:30, rginda wrote:
> Is this going to be an autotest test?  If so it should probably move into
> third_party/autotest/files/client/site-tests/...
> 
> If it is going to become part of the autotest portion of the codebase, that
> means that PEP8 applies (http://www.python.org/dev/peps/pep-0008/), which
calls
> for methods to be lowercase_and_underscored rather than WordCapped.  It also
> specifies 4-space indents, rather than two.
> 
> I've also heard that autotest deviates from PEP8 in that it recommends putting
> multiple imports in one line, as in
> 
>   import os, re, signal, socket, ...
> 
> If it's not moving into autotest/, feel free to ignore all that :)

it's not, nor planning to ever be, an autotest test. I talked to ken mixter and
i just didn't seem like a good fit.

http://codereview.chromium.org/1512001/diff/1/2#newcode45
src/platform/memento_softwareupdate/autoupdated_vm_test.py:45: class TestState:
On 2010/03/29 02:08:30, rginda wrote:
> 
> http://docs.python.org/reference/datamodel.html#newstyle
> 
> Python has 'old style' classes and 'new style'.  In general, stick with
> new-style classes, which means inheriting from 'object' or something that
> inherits from it. 

Done.

http://codereview.chromium.org/1512001/diff/1/2#newcode53
src/platform/memento_softwareupdate/autoupdated_vm_test.py:53: range(6)
On 2010/03/29 02:08:30, rginda wrote:
> 
> http://docs.python.org/library/stdtypes.html#xrange-type
> 
> Using range here makes python create a list of the numbers 0 through 5 and
then
> copy that list.  xrange initializes a generator-like object that is O(1) in
> space.  In this case the difference is minimal, but xrange is more idiomatic.

Done.

http://codereview.chromium.org/1512001/diff/1/2#newcode70
src/platform/memento_softwareupdate/autoupdated_vm_test.py:70: print 'SetState('
+ str(state) + ')'
On 2010/03/29 02:08:30, rginda wrote:
> http://docs.python.org/library/stdtypes.html#string-formatting
> 
> The python idiom for this is...
> 
>   print 'SetState(%s)' % state
> 
> Notice that you don't have to use str(state) here, because Python will convert
> the argument for you.  Use a list to format multiple values...
> 
>   print 'a: %s, b: %s' % (a, b)
> 
> Modulo is a general string formatting operator in python and not specific to
the
> print statement, so you use it anywhere a string appears.
> 
> Also, if this is to become autotesty, then you probably want to use
logging.info
> or logging.debug instead of print.

I changed many, but not all string + calls, to this style.

I wasn't sure for some basic ones like:

foo = constant + '/update.gz'

is it really preferred to say:

foo = '%s/update.gz' % constant

?

if so, i'll happily change them all

http://codereview.chromium.org/1512001/diff/1/2#newcode155
src/platform/memento_softwareupdate/autoupdated_vm_test.py:155: if ret != None:
On 2010/03/29 02:08:30, rginda wrote:
>
http://www.corp.google.com/eng/doc/pyguide.xml?showone=True/False_evaluations...
> 
> It's more pythonic to use the implicit conversion to boolean in tests, as in
'if
> not ret:', although in this case I think you want 'if ret is not None', or
swap
> the logic and go for 'if ret is None:'

Maybe I can come by and ask you a couple questions about python comparisons
later this afternoon?

also, changed.

http://codereview.chromium.org/1512001/diff/1/2#newcode175
src/platform/memento_softwareupdate/autoupdated_vm_test.py:175: if self.timer !=
None:
On 2010/03/29 02:08:30, rginda wrote:
> When comparing against objects and None, "is" and "is not" are typically used,
> since they skip type coercion (or at least, I think that's why.) 

Done.

http://codereview.chromium.org/1512001/diff/1/2#newcode189
src/platform/memento_softwareupdate/autoupdated_vm_test.py:189:
HTTPServer.__init__(self, ip_port, klass)
On 2010/03/29 02:08:30, rginda wrote:
> http://docs.python.org/library/functions.html#super
> 
> It's considered more matintainable to use super(C, self) than to directly
refer
> to your superclass' name, so this turns into...
> 
>   super(AUTHTTPServer, self).__init__(...)
> 

Done.

[rginda, 2010-03-31 16:51:44]

http://codereview.chromium.org/1512001/diff/1/2
File src/platform/memento_softwareupdate/autoupdated_vm_test.py (right):

http://codereview.chromium.org/1512001/diff/1/2#newcode70
src/platform/memento_softwareupdate/autoupdated_vm_test.py:70: print 'SetState('
+ str(state) + ')'
On 2010/03/30 03:44:15, adlr wrote:
> On 2010/03/29 02:08:30, rginda wrote:
> > http://docs.python.org/library/stdtypes.html#string-formatting
> > 
> > The python idiom for this is...
> > 
> >   print 'SetState(%s)' % state
> > 
> > Notice that you don't have to use str(state) here, because Python will
convert
> > the argument for you.  Use a list to format multiple values...
> > 
> >   print 'a: %s, b: %s' % (a, b)
> > 
> > Modulo is a general string formatting operator in python and not specific to
> the
> > print statement, so you use it anywhere a string appears.
> > 
> > Also, if this is to become autotesty, then you probably want to use
> logging.info
> > or logging.debug instead of print.
> 
> I changed many, but not all string + calls, to this style.
> 
> I wasn't sure for some basic ones like:
> 
> foo = constant + '/update.gz'
> 
> is it really preferred to say:
> 
> foo = '%s/update.gz' % constant
> 
> ?
> 
> if so, i'll happily change them all

It's not worth it to change them all, especially if you're working with a
constant that you know a string.  For paths, os.path.join(constant, 'update.gz')
is the typical pattern because it's cross platform, but since we're going to
have much bigger cross platform issues than that, I don't think it matters.

http://codereview.chromium.org/1512001/diff/1/2#newcode155
src/platform/memento_softwareupdate/autoupdated_vm_test.py:155: if ret != None:
On 2010/03/30 03:44:15, adlr wrote:
> On 2010/03/29 02:08:30, rginda wrote:
> >
>
http://www.corp.google.com/eng/doc/pyguide.xml?showone=True/False_evaluations...
> > 
> > It's more pythonic to use the implicit conversion to boolean in tests, as in
> 'if
> > not ret:', although in this case I think you want 'if ret is not None', or
> swap
> > the logic and go for 'if ret is None:'
> 
> Maybe I can come by and ask you a couple questions about python comparisons
> later this afternoon?
> 
> also, changed.

Sorry, I was ooo Monday and Tuesday, but I'm back today.

[adlr, 2010-04-01 20:03:40]

Hey, I made a ton of changes b/c of GPT. Can you take another look?

Thanks

[rginda, 2010-04-01 22:21:29]

LGTM

On 2010/04/01 20:03:40, adlr wrote:
> Hey, I made a ton of changes b/c of GPT. Can you take another look?
> 
> Thanks