autotest: end-to-end autoupdate test [chromiumos/third_party/autotest : master] - Chromium OS reviews

The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Message from discussion autotest: end-to-end autoupdate test [chromiumos/third_party/autote st : master]
Received-SPF: pass (google.com: domain of ger...@chromium.org designates 74.125.248.75 as permitted sender) client-ip=74.125.248.75;
Message-Id: <5099b4c1.03b0340a.505c.2f13SMTPIN_ADDED@mx.google.com>
Date: Tue, 6 Nov 2012 17:09:21 -0800
From: "Chris Masone (Code Review)" <ger...@chromium.org>
To: Gilad Arnold <garn...@chromium.org>
CC: Gerrit <chrome-...@google.com>, Chris Sosa <s...@chromium.org>,
	Tracy Turchetto <tturche...@chromium.org>,
	Don Garrett <dgarr...@chromium.org>,
	Scott Zawalski <sco...@chromium.org>,
	Jay Srinivasan <jay...@chromium.org>
Reply-To: cmas...@chromium.org
Subject: =?UTF-8?Q?autotest:_end-to-end_autoupdate_test_[chromiumos/third=5Fparty/autotest_:_master]=0A?=
Mailing-List: list gerrit-chromiumos-third_party-autot...@gerrit.chromium.org
In-Reply-To: <gerrit.1352191002009.I215e8214d3239f525f062a27a4d97806451c9...@gerrit.chromium.org>
References: <gerrit.1352191002009.I215e8214d3239f525f062a27a4d97806451c9...@gerrit.chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.4.2

Chris Masone has posted comments on this change.

Change subject: autotest: end-to-end autoupdate test
......................................................................


Patch Set 2: (21 inline comments)

I haven't gotten to the event verifying stuff yet, but I think these comments are enough to be going on with.

....................................................
File client/common_lib/cros/dev_server.py
Line 180:             devserver = devservers.pop(hash_index)
Why did you change the semantics here?  We don't want to pop the server off the list unless it's not up.

Line 295:         url_dict['mton_payload'] = ('%s/static/%s/au/%s_%s/update.gz' %
We spent a lot of effort getting "magic" format strings like this out of the code and unified into global_config.ini; see image_url_pattern and package_url_pattern

Line 299:         return url_dict
Why are you creating these strings and returning them here?  You can do this outside this method, and it would be less magical.  There's no reason for this code to understand that "image" has "release" buried in it.

Line 348:                 (self.url(), image_dir, 'chromiumos_test_image.bin'))
This is similarly problematic as the above.

....................................................
File client/common_lib/site_utils.py
Line 142:         result = base_utils.run(gs_cmd).stdout.split('\n')
why aren't you using system_output, here?

Line 145:     return [path.rstrip() for path in result if path]
I think a bare newline in a string evaluates to True, so you're not skipping newlines here.  Also, there is a more elegant way to split and tear out the newlines.

result = base_utils.system_output(gs_cmd).splitlines()

now, bare newlines in the output will be empty strings in result, and your list expression will do the right thing.

....................................................
File server/site_tests/autoupdate_EndToEndTest/autoupdate_EndToEndTest.py
Line 18: def _secs_to_hms(secs):
Python has many utilities for formatting times and such http://docs.python.org/2/library/time.html#module-time

You really shouldn't need to roll your own.

Line 245:     # Timeout periods, given in seconds.
We usually like the constant names to have the units in them.

_WAIT_AFTER_SHUTDOWN_SECONDS, for example.  Improves readability

Line 270:         """Compute a unique IP port to bind a private Omaha/devserver to.
Why not just generate a random number in the right range, try it, and if it fails try another?  Does trying take a long time?

Line 306:         host.servo.set('usb_mux_sel1', 'servo_sees_usbkey')
These constants aren't stored somewhere in the Servo module/class?  Shouldn't they be?

Line 331:         host.servo.power_long_press()
why do you do a long-press?  It seems like a short press (graceful shutdown) would be fine.  Can you explain in a comment?

Line 333:         if intermediate_callback:
This callback mechanism feels overengineered.  Just wrap the 'shutdown' and 'power on' logic into two separate methods and then do

self._shutdown_and_wait(host)
self.<whatever the callback would be>
self._power_on_and_wait(host, is_dev_mode)

It's much less confusing.

Line 342:           # TODO(garnold) implement waiting for MP-signed images; ideas include
Always good to capture a significant TODO in crosbug, and point to it.  This goes for most TODOs in the CL, not just this one

Line 379:         # probably want to use a common library call for that.
Why not do that refactor in an initial CL, then use the refactored code in this CL?

Line 382:         host.servo.install_recovery_image(lorry_image_url)
wait...why does install_revovery_image() behave differently here and above?  How does it know to be different?

Line 420:         updater.trigger_update()
2 lines between functions

Line 437:                             'image.zip')[0].strip('/')
This is confusing to read :-/

All this code makes a lot of assumptions about URL formats. That makes the code brittle; one of the first things we did when paying back the technical debt from the old lab setup was remove these kinds of assumptions from the code base.  I think you need to avoid adding them here.

Line 508:         lorry_devserver = dev_server.ImageServer.resolve(0)
Perhaps take this up with Sosa before trying to submit this CL?

Line 519:         # Launch Omaha/devserver.
I think all this logic around the Omaha/devserver needs to be wrapped up in its own, unittestable class.

....................................................
File server/site_tests/autoupdate_EndToEndTest/control
Line 61: servo_args = hosts.SiteHost.get_servo_arguments(args_dict)
What're you using servo for?

Line 68:     test_conf[key] = args_dict.get(key) or locals().get(key)
You won't be passing in any command line arguments.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I215e8214d3239f525f062a27a4d97806451c9736
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Gilad Arnold <garn...@chromium.org>
Gerrit-Reviewer: Chris Masone <cmas...@chromium.org>
Gerrit-Reviewer: Chris Sosa <s...@chromium.org>
Gerrit-Reviewer: Don Garrett <dgarr...@chromium.org>
Gerrit-Reviewer: Gerrit <chrome-...@google.com>
Gerrit-Reviewer: Gilad Arnold <garn...@chromium.org>
Gerrit-Reviewer: Jay Srinivasan <jay...@chromium.org>
Gerrit-Reviewer: Scott Zawalski <sco...@chromium.org>
Gerrit-Reviewer: Tracy Turchetto <tturche...@chromium.org>
Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
Account Options
