autotest: spawn multiple release full update tests [chromiumos/third_party/autotest : master]

[Gilad Arnold (Code Review)]

Gilad Arnold has uploaded a new change for review.

Change subject: autotest: spawn multiple release full update tests
......................................................................

autotest: spawn multiple release full update tests

This tool (full_release_test) infers and spawns a sequence of AU full
release tests:

* Creates a list of test configurations, based on the given release and
the availability of images and payloads.

* Spawns test instances sequentially via run_remote_tests.sh.

Some auxiliary modules:

* board: reference information about Chrome OS board. Parses
a board_config.ini file in the current directory.

* omaha: functions for querying Omaha.

* release: reference information related to releases and branches.
Parses a release_config.ini file in the current directory.

* test_image: handling of test images/payloads from archive.

BUG=chromium-os:33760
TEST=Infers test configurations on x86-alex with test images; invokes
autotest (autoupdate_FullReleaseTest) for each.

Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
---
A site_utils/autoupdate/board.py
A site_utils/autoupdate/board_config.ini
A site_utils/autoupdate/full_release_test
A site_utils/autoupdate/omaha.py
A site_utils/autoupdate/release.py
A site_utils/autoupdate/release_config.ini
A site_utils/autoupdate/test_image.py
7 files changed, 888 insertions(+), 0 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/autotest refs/changes/33/37433/1
--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................


Patch Set 6:

Please rename full_release_autotest to full_release_autotest.py. If you want the short-hand to execute more readily, check-in a symlink from full_release_autotest to full_release_autotest.py. Otherwise it makes it much harder to review this in gerrit.

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Gilad Arnold <gar...@chromium.org>
Gerrit-Reviewer: Jay Srinivasan <jay...@chromium.org>
Gerrit-Reviewer: Scott Zawalski <sco...@chromium.org>
Gerrit-Reviewer: Tracy Turchetto <tturc...@chromium.org>

[Simran Basi (Code Review)]

Simran Basi has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 7: (37 inline comments)

K so I was Nit-picky and may have gone overboard but a lot of your alignment doesn't match the autotest standards.

In general: Align with the previous line, fit as many args as you can to 80 characters and wrap to the last open paren, etc.

Only when thats not possible then wrap using double indent (which is what you used mostly).

For strings that are too long just end at 80 chars or the last word and then continue on the next line. Python will automatically join the strings. And if they're not in an open paren just put it inside paren's.

I did point out a lot of violations about alignment but I may have missed some. But please take your time to go through and address this. Thanks!

....................................................
File site_utils/autoupdate/board.py
Line 12: os.getcwd(), 'board_config.ini')
Is there a reason for the newline? This looks short enough to be 1 line, and if not you can have one argument be on the first line and the second continue right below the paren.

....................................................
File site_utils/autoupdate/full_release_test.py
Line 42:
2 spaces between methods

Line 44: return 'mp' if self.mp_images else 'test'
Repeat: spacing

Line 47: return 'delta' if self.delta_update else 'full'
Repeat: spacing

Line 51: (self.board, self.name, self.get_image_type(),
move over 1 space align with first element in (

Line 57: return '\n'.join([
alignment

Line 61: ])
Repeat: spacing

Line 65: ['%s%s%s' % (key, assign, val) for key, val in [
align with (

Line 78: # Global reference objects.
can you put these at the top of the file?

Line 95: source_image_uri = test_image.find_image_uri(
alignment

Line 99: return TestConfig(
alignment

Line 137: else:
if not mp_images: and get rid of the previous if

Line 146: (source_release, target_release) = \
don't use the '\' character for newline. wrap it all in parens

Line 153: 'unexpected delta target release: %s != %s (%s)',
align with ( and if its too long break the string up

Line 160: raise TestError(
alignment

Line 209: else:
if not mp_images: and get rid of the previous if

Line 216: logging.warning(
alignment

Line 224: test_config = generate_test_image_test_config(
alignment

Line 248: test_list += generate_npo_nmo_test_list(
alignment

Line 254: test_list += generate_fsi_test_list(
alignment

Line 325: if val is not None:
if val:

Line 326: if val is True:
if val:

Line 328: elif val is False:
elif not val:

....................................................
File site_utils/autoupdate/omaha.py
Line 23: '</o:gupdate>')
2 blank lines

....................................................
File site_utils/autoupdate/release.py
Line 12: _RELEASE_CONFIG_FILE = os.path.join(
alignment

Line 44: branchpoint_list = map(
alignment

Line 51: self._branchpoint_dict[branchpoint] = \
use paren not \

Line 53: _CONF_BRANCH_SECTION,
alignment

Line 58: _CONF_BRANCH_SECTION, _CONF_NEXT_BRANCH_OPT)
alignment

Line 103: i = bisect.bisect_left(
alignment

....................................................
File site_utils/autoupdate/test_image.py
Line 17: pass
4 spaces

Line 29: output = subprocess.Popen(
alignment

Line 63: if payload_uri_list_len == 0:
if not payload_uri_list_len:

Line 66: raise TestImageError(
alignment

Line 91: image_archive_uri_list = gs_ls(
alignment

Line 96: if image_archive_uri_list_len == 0:
if not image_archive_uri_list_len:

Line 99: raise TestImageError(
alignment

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Gilad Arnold <gar...@chromium.org>
Gerrit-Reviewer: Jay Srinivasan <jay...@chromium.org>
Gerrit-Reviewer: Scott Zawalski <sco...@chromium.org>

Gerrit-Reviewer: Simran Basi <sb...@chromium.org>
Gerrit-Reviewer: Tracy Turchetto <tturc...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................


Patch Set 7: (37 inline comments)

Thanks for reviewing. Fixed most of your comments, but not all.

In general, my understanding from previous feedback, Google style guide and PEP-8 (which to the best of my understanding is the Autotest style) is that there is no strict preference between the two continuation line indentation methods (align at paren vs break at paren with double indent). I prefer to stick with the latter when it saves me lines or seems generally more readable. Let me know whether that's a problem.

....................................................
File site_utils/autoupdate/board.py
Line 12: os.getcwd(), 'board_config.ini')

Used to be longer, fixed.

....................................................
File site_utils/autoupdate/full_release_test.py
Line 42:

Done

Line 44: return 'mp' if self.mp_images else 'test'

Done

Line 47: return 'delta' if self.delta_update else 'full'

Done

Line 51: (self.board, self.name, self.get_image_type(),

Not understanding, this looks aligned to me...?

Line 57: return '\n'.join([

Done

Line 61: ])
Done

Line 65: ['%s%s%s' % (key, assign, val) for key, val in [

Not sure I understand. Join this line with the preceding line, aligned subsequent lines with 2x indent

Line 78: # Global reference objects.

Done

Line 95: source_image_uri = test_image.find_image_uri(

I changed it, although this seems to conform with the style guide---?

Line 99: return TestConfig(
Done

Line 137: else:
But I need the TODO item as a reminder. I'll swap the branches.

Line 146: (source_release, target_release) = \

Done

Line 153: 'unexpected delta target release: %s != %s (%s)',

Done. Though appears less readable to me.

Line 160: raise TestError(
Done

Line 209: else:
Swapped branches. Done.

Line 216: logging.warning(
Done

Line 224: test_config = generate_test_image_test_config(

Results in four, mostly empty lines. Leaving as is.

Line 248: test_list += generate_npo_nmo_test_list(

Ditto

Line 254: test_list += generate_fsi_test_list(

Same here

Line 325: if val is not None:

No, I need it to be not None (and not a logical false value)

Line 326: if val is True:

I need val to be the Boolean value True

Line 328: elif val is False:

Same for False

....................................................
File site_utils/autoupdate/omaha.py
Line 23: '</o:gupdate>')

Done

....................................................
File site_utils/autoupdate/release.py
Line 12: _RELEASE_CONFIG_FILE = os.path.join(

Done

Line 44: branchpoint_list = map(

Done

Line 51: self._branchpoint_dict[branchpoint] = \

Done

Line 53: _CONF_BRANCH_SECTION,
Done

Line 58: _CONF_BRANCH_SECTION, _CONF_NEXT_BRANCH_OPT)
Done

Line 103: i = bisect.bisect_left(

Done

....................................................
File site_utils/autoupdate/test_image.py
Line 17: pass

Done

Line 29: output = subprocess.Popen(

Done

Line 63: if payload_uri_list_len == 0:

It's a numeric value, this comparison is clearer and consistent with the subsequent elif.

Line 66: raise TestImageError(
Done

Line 91: image_archive_uri_list = gs_ls(

Done

Line 96: if image_archive_uri_list_len == 0:

Same as above.

Line 99: raise TestImageError(
Done

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 10: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 10

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 7: (1 inline comment)

....................................................
File site_utils/autoupdate/full_release_test.py

Line 95: source_image_uri = test_image.find_image_uri(

What you had was fine. Either way works. Rule is either line up with parens OR double indent starting with first var (which you did here).

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 11: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 11

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 13: Verified

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 13

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 15: I would prefer that you didn't submit this

(19 inline comments)

It's hard to tell if this ready because you keep uploading patches but I reviewed it anyway.

....................................................
File site_utils/autoupdate/board_config.ini
Line 5: appid: {F26D159B-52A3-491A-AE25-B23670A66B32}
Where did you get these appid's from?

....................................................
File site_utils/autoupdate/board.py
Line 56: def get_vendor(self, board):
hrm, seems like a lot of extra code for litte functionality. Why not just expose get_config and perhaps have some constants like BoardInfo.appid = 'appid' to ensure you use the right names.

I don't mind if you keep it just want to understand why you wanted to add so much LOC for it

Line 59: Returns None is vendor was not defined for this board.
s/is/if

....................................................
File site_utils/autoupdate/full_release_test.py
Line 27: class TestError(BaseException):
AUTestError so not to be confused with autotest_lib.common_lib.error.TestError

Line 32: """A single test configuration."""
doc the attributes.

Line 57: def __str__(self):
Docstrings on why you implemented both of these.

Line 108: def generate_npo_nmo_test_list(board, tested_release, mp_images,
Given how big this method is it would be better to separate the mp / non-mp components into separate methods rather than the large conditional block and raise NotImplementedError for _generate_mp_list

Line 110: """Generates N+1/N-1 test configurations.
Also this name is pretty verbose. generate_npo_list should be sufficient.

Line 296: raise TestError('invalid board (%s)' % args.tested_board)
use parser.error()

Line 315: logging.warning('no test configurations generated, nothing to do')
logging.fatal

Line 329: for test in test_list:
use enumerate

i.e.

for i, test in enumerate(test_list):
party()

Line 333: cmd = [
Why are you wasting your first line?

Line 338: '--ssh_connect_timeout=2',
These constants should be variablized somewhere i.e. module constants

Line 343: subprocess.call(cmd)
should be a check_call.

....................................................
File site_utils/autoupdate/omaha.py
Line 74: if not match:
prob also check there's a first group

....................................................
File site_utils/autoupdate/release.py
Line 31: root directory.
doc yo vars

Line 61: except Exception, e:
be more specific.

Line 74: for (branch, release) in self._sorted_branchpoint_list])
This should all be done in an initalize method. init's shouldn't do very much

....................................................
File site_utils/autoupdate/test_image.py
Line 65: elif payload_uri_list_len != 1:
Why is 0 ok but not 2? Seems like !=1 should raise an error regardless.

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 15

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 15: (19 inline comments)

....................................................
File site_utils/autoupdate/board_config.ini
Line 5: appid: {F26D159B-52A3-491A-AE25-B23670A66B32}

I can't remember exactly... probably the thing used by the testing team. I can double check. Why?

....................................................
File site_utils/autoupdate/board.py
Line 56: def get_vendor(self, board):

Seems a tad safer in general; some values may need to be processed rather than returned raw (e.g. FSI releases list). I suspect that savings will be modest anyhow. I massaged it a little bit.

Line 59: Returns None is vendor was not defined for this board.

Done

....................................................
File site_utils/autoupdate/full_release_test.py
Line 27: class TestError(BaseException):

I'll rename it.

Line 32: """A single test configuration."""

Done

Line 57: def __str__(self):
Done

Line 108: def generate_npo_nmo_test_list(board, tested_release, mp_images,

Done

Line 110: """Generates N+1/N-1 test configurations.

Well, it generates both npo and nmo configs, so I'd rather leave both; I'll s/test_list/list/ if that feels better, although that'll be inconsistent with generate_test_list()...

Line 296: raise TestError('invalid board (%s)' % args.tested_board)

Done

Line 315: logging.warning('no test configurations generated, nothing to do')

Done

Line 329: for test in test_list:

Done

Line 333: cmd = [
Done

Line 338: '--ssh_connect_timeout=2',
in fact, they shouldn't be here at all. Fixed.

Line 343: subprocess.call(cmd)
Done

....................................................
File site_utils/autoupdate/omaha.py
Line 74: if not match:

If there's a match then there has to be a non-empty first group, no?

....................................................
File site_utils/autoupdate/release.py
Line 31: root directory.

Done

Line 61: except Exception, e:

Done

Line 74: for (branch, release) in self._sorted_branchpoint_list])

Done

....................................................
File site_utils/autoupdate/test_image.py
Line 65: elif payload_uri_list_len != 1:

This function allows for an empty result in general. Normally, it's [] but for single it's None. Since single is meant to unpack a single-item list, getting strictly more than one is considered an error.

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 15

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 17: Verified

Apologies for multiple revisions, had two separate CLs on the same local branch... next time, a separate branch.

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 17

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 17: I would prefer that you didn't submit this

(21 inline comments)

Closer!

....................................................
File site_utils/autoupdate/full_release_test.py

Line 32: """A single test configuration."""

Generates autotest args for autoupdate_EndToEndTest

Line 80: """Full textual representation w/ image/payload URIs."""
Why do you need both of these? Its rare I see these both defined

Line 105: source_release, target_release, payload_uri):
doc args

Line 128: given tested release and board.
You should doc the structure of what a directory looks like here since you do a lot of safety checks in your for loop i.e. not more than one N+1 or n-1 payload

Line 147: payload_uri_list = test_image.find_payload_uri(
why not check if the length is > 2 raise an exception -- too many items returned

Line 153: (source_release, target_release) = (
useless parens around vars here

Line 177: if test_nmo and source_release != target_release:
elif

Line 186: board, 'nmo', False, True, source_release, tested_release,
use target_release instead of tested_release here given your other comparisons

Line 189: return npo_test_config, nmo_test_config
Should just return a list

Line 212: 'generation of mp-signed test configs not implemented')
one more indent

Line 216: test_nmo, test_npo):
indentation

Line 235: if not (test_nmo or test_npo):
You already check this in the things you call. Check in one place, but not both.

Line 239: npo_test_config, nmo_test_config = (
Please use the if/else. You should only use the inline if/else's when it can all fit in one line otherwise it gets harder to read.

Line 247: test_list = []
simplify your calls to generate_test_*_list to return a list and just return that

Line 271: # we do not have delta payloads from FSIs.
Why is this check done here rather than when using the test conifgs?

Line 276: logging.warning('cannot find full payload for %s, %s; no fsi tests',
I think this should be a logging.error

Line 358: parser = argparse.ArgumentParser(
note this won't work on old machines as ArgumentParser is only in very late versions of Python 2.6 as it was backported from 2.7 . Given you don't use args, you may want to just stick with optionparser

Line 361: parser.add_argument('tested_release', metavar='RELEASE',
why do you want testers to type so much? tested is implied in all of htese as is test. I'd shorten these all by 4/5 letters to release/board/npo/fsi/nmo

Line 383: parser.add_argument('--dev_mode', action='store_true',
This argument doesn't seem like it has a good name. --dev_mode would not make me think that this would hit ctrl+d. Why not jsut always do this if servo is set?

Line 386: parser.add_argument('--verbose', action='store_true',
These seems the same to me (from their names). I'd either use logging-level or just one or the other.

Line 425: for key in ['servo_host', 'servo_port', 'omaha_host', 'dev_mode']:
Why aren't you using these directly i.e. args.servo_host etc. You should not reference its __dict__ like this

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 17

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 17: (21 inline comments)

....................................................
File site_utils/autoupdate/full_release_test.py
Line 32: """A single test configuration."""

Done

Line 80: """Full textual representation w/ image/payload URIs."""

One shorter/partial and one longer/complete. My take on "one readable and one unambiguous." I could kill __repr__ and use an explicit ToStr() method, possibly with some verbosity argument, if you prefer---?

Line 105: source_release, target_release, payload_uri):

Done

Line 128: given tested release and board.

Done

Line 147: payload_uri_list = test_image.find_payload_uri(

It's a necessary but insufficient check. The separate checks for each case inside the loop are sufficient. Why perform an additional check before entering the loop, to cover only part of the cases?

Line 153: (source_release, target_release) = (

Done

Line 177: if test_nmo and source_release != target_release:

Revised, consolidated logic (aka Done)

Line 186: board, 'nmo', False, True, source_release, tested_release,

Yes, revised.

Line 189: return npo_test_config, nmo_test_config

This too

Line 212: 'generation of mp-signed test configs not implemented')

Done

Line 216: test_nmo, test_npo):
Finally nailed down my vim python indentation ;-) Done.

Line 235: if not (test_nmo or test_npo):

This function heavily revised/shortened as well. This was removed.

Line 239: npo_test_config, nmo_test_config = (

Done

Line 247: test_list = []
Done

Line 271: # we do not have delta payloads from FSIs.

A test config would be invalid if missing an update payload. There's no point in returning an incomplete config.

Line 276: logging.warning('cannot find full payload for %s, %s; no fsi tests',

Nope, we don't have the full line of FSIs available (at least not currently) and none for some boards, so I'd say a warning is the right thing.

Line 358: parser = argparse.ArgumentParser(

Alas, switching back to OptionParser... Done

Line 361: parser.add_argument('tested_release', metavar='RELEASE',

Done

Line 383: parser.add_argument('--dev_mode', action='store_true',

Servo is always used. I'll remove this. The test script will infer whether it's dev mode (via servo) and do the right thing.

Line 386: parser.add_argument('--verbose', action='store_true',

Using --log=LEVEL now. Done.

Line 425: for key in ['servo_host', 'servo_port', 'omaha_host', 'dev_mode']:

Well, I want to be able to iterate through (optional) object data attributes, and to be able to parametricly refer to both the name and value. I'll use vars(args)---better?

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 17

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 19: (6 inline comments)

Mostly nits.

Just one overarching question. Any particular reasons you aren't using autotest_lib for your command running? You really should be using that rather than subprocess since this is still in the autotest repos.

....................................................
File site_utils/autoupdate/board.py
Line 5: # Module for obtaining Chrome OS board related info.
docstring

Line 33: except Exception, e:
You should prob catch IOError here

....................................................
File site_utils/autoupdate/full_release_test.py
Line 7: # A tool for inferring and spawning a complete set of autoupdate tests for a
this should be a docstring

....................................................
File site_utils/autoupdate/omaha.py
Line 5: # An API for issuing Omaha requests.
ditto

....................................................
File site_utils/autoupdate/release.py
Line 112: except Exception, e:
Please be more specfic about your exceptions.

....................................................
File site_utils/autoupdate/test_image.py
Line 5: # Module for discovering Chrome OS test images and payloads.
This should be a """docstring"""

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 19

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................


Patch Set 19: (6 inline comments)

Re: subprocess vs wrapper: the wrappers I've seen so far are under client/common_lib. I was assuming these were intended for use by tests only. If this isn't the case then I can definitely switch to using system_output() or something.

....................................................
File site_utils/autoupdate/board.py
Line 5: # Module for obtaining Chrome OS board related info.

Done

Line 33: except Exception, e:

Done

....................................................
File site_utils/autoupdate/full_release_test.py
Line 7: # A tool for inferring and spawning a complete set of autoupdate tests for a

Done

....................................................
File site_utils/autoupdate/omaha.py
Line 5: # An API for issuing Omaha requests.

Done

....................................................
File site_utils/autoupdate/release.py
Line 112: except Exception, e:

Revised this.

....................................................
File site_utils/autoupdate/test_image.py
Line 5: # Module for discovering Chrome OS test images and payloads.

Done

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 19

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 20: Looks good to me, approved

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 20

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 21: Verified; Looks good to me, approved; Ready

Rebased, inheriting +2 from sosa.

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 21

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>

[Gilad Arnold (Code Review)]

Gilad Arnold has posted comments on this change.

Change subject: autotest: spawn a complete set of end-to-end autoupdate tests
......................................................................

Patch Set 23: Verified; Looks good to me, approved; Ready

Rebased, inherited +2 from sosa.

--
To view, visit https://gerrit.chromium.org/gerrit/37433
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b1e6addc925b811f3b844caa7dc3c585f203a22
Gerrit-PatchSet: 23

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <gar...@chromium.org>