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(-)
Discussion subject changed to "autotest: spawn a complete set of end-to-end autoupdate test... [chromiumos/third_party/autote st : master]" by Chris Sosa (Code Review)
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.
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:
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.
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).
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.
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.
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
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?
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"""
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
