Adding a startup regression test.

Adding a startup regression test.

Adding a test that starts up a chrome build against a corpus of CRXs.
Vet that a module loads and doesn't crash.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2701
TEST=startup_regression
R=ncbray@chromium.org

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8271

[Nick Bray, 2012-04-02 20:31:41]

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
File tests/startup_regression/startup_regression.py (right):

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:20: KNOWN_BAD = set([
This is going to be visible in a publicly viewable repo.  Weren't you concerned
about spewing app IDs?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:51: import download_utils
Move import hackery above KNOWN_BAD - keep it grouped.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:63: env['PATH'] =
'/b/build/scripts/slave' + os.pathsep + env['PATH']
* Lift env creation out of loop - makes intent more obvious.
* Document why you typically need to retry?
* Optional: Add sleep?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:76: sys.exit(1)
Very optional: I am not a huge fan of exiting inside a function.  There's
nothing wrong, pre se, but my opinion is that throwing user-defined 'failiure'
exceptions (which is almost equivalent) is more future proof - how you handle
failure at an outer scope may change, etc.  (See buildbox_standard.py) Not a
very convincing argument, it's mostly instinct.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:79: def
DownloadTotalList(list_filename):
Bad name.  List of what?  Total of what?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:92: def DownloadFile(src_path,
dst_filename):
Bad name.  DownloadFileFromSnapshot?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:102: def Sha1Sum(path):
Sha1Digest?  Sha1FileDigest?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:126: """Converts a path to the
corrisponding chrome app id.
Path, in what context?  On disk?  In the web store?  In the extension directory?

It would be nice to know where you pulled this algorithm from and why you need
it.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:139: def TestAppStartup(options,
crx_path, app_path, profile_path):
Big function.  Is there a clean way to modularize it?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:164: process =
subprocess.Popen(cmd,
For example, this is essentially subprocess.communicate with a time out.  Sounds
like a function to me.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:184: time.sleep(1)
Why sleep?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:190: process_stdout =
stdout_output[0]
Scraping the output will not work on Windows.  We probably will want to run on
Windows, too.  A problem for tomorrow, but you might want to check if someone
tried to run this test on Windows and print a helpful error that explains why
that won't work.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:191: process_stderr =
stderr_output[0]
Scraping for failiures => function (but not printing the failiure)

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:224: manifest_data =
manifest_data.replace(u'\ufeff', '')
What are these characters?  Document.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:228: return {}
This seems a little sketchy.  Why return an empty dict?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:254: def
Sha1FromFilename(filename):
Are there functions here that you should be sharing with the validator script?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:269: def PrimeCache(options,
filename):
Dito.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:294: assert not
os.path.exists(dest)
If you have an assert, have a message to help explain it.  dest, in this case,
would be sufficient.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:300: tpath = os.path.join(dest,
info.filename)
Pwnage: Zipfile is from an untrusted source, filename could be an absolute path,
join discards everything before an absolute path, attacker could cause you to
overwrite an arbitrary file.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:327: if index > 0:
ETA calculation and / or header printing in function?

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:359: default_cache_dir = '/b'
Another bit of code that might do well shared.

[bradn, 2012-04-02 22:43:59]

PTAL

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
File tests/startup_regression/startup_regression.py (right):

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:20: KNOWN_BAD = set([
On 2012/04/02 20:31:41, Nick Bray wrote:
> This is going to be visible in a publicly viewable repo.  Weren't you
concerned
> about spewing app IDs?
So actually mainly I'm worried about app ids.
These are sha1's. If anything I'm thinking of going back to the other and
calling out only sha1s for known bad ones (there were 2 in the other).

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:51: import download_utils
On 2012/04/02 20:31:41, Nick Bray wrote:
> Move import hackery above KNOWN_BAD - keep it grouped.

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:63: env['PATH'] =
'/b/build/scripts/slave' + os.pathsep + env['PATH']
On 2012/04/02 20:31:41, Nick Bray wrote:
> * Lift env creation out of loop - makes intent more obvious.
> * Document why you typically need to retry?
> * Optional: Add sleep?

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:76: sys.exit(1)
On 2012/04/02 20:31:41, Nick Bray wrote:
> Very optional: I am not a huge fan of exiting inside a function.  There's
> nothing wrong, pre se, but my opinion is that throwing user-defined 'failiure'
> exceptions (which is almost equivalent) is more future proof - how you handle
> failure at an outer scope may change, etc.  (See buildbox_standard.py) Not a
> very convincing argument, it's mostly instinct.

Done, due to later unrelated refactor.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:79: def
DownloadTotalList(list_filename):
On 2012/04/02 20:31:41, Nick Bray wrote:
> Bad name.  List of what?  Total of what?

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:92: def DownloadFile(src_path,
dst_filename):
On 2012/04/02 20:31:41, Nick Bray wrote:
> Bad name.  DownloadFileFromSnapshot?

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:102: def Sha1Sum(path):
On 2012/04/02 20:31:41, Nick Bray wrote:
> Sha1Digest?  Sha1FileDigest?

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:126: """Converts a path to the
corrisponding chrome app id.
On 2012/04/02 20:31:41, Nick Bray wrote:
> Path, in what context?  On disk?  In the web store?  In the extension
directory?
> 
> It would be nice to know where you pulled this algorithm from and why you need
> it.

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:139: def TestAppStartup(options,
crx_path, app_path, profile_path):
On 2012/04/02 20:31:41, Nick Bray wrote:
> Big function.  Is there a clean way to modularize it?

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:164: process =
subprocess.Popen(cmd,
On 2012/04/02 20:31:41, Nick Bray wrote:
> For example, this is essentially subprocess.communicate with a time out. 
Sounds
> like a function to me.

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:184: time.sleep(1)
On 2012/04/02 20:31:41, Nick Bray wrote:
> Why sleep?
Oops, switched to wait.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:190: process_stdout =
stdout_output[0]
On 2012/04/02 20:31:41, Nick Bray wrote:
> Scraping the output will not work on Windows.  We probably will want to run on
> Windows, too.  A problem for tomorrow, but you might want to check if someone
> tried to run this test on Windows and print a helpful error that explains why
> that won't work.  

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:191: process_stderr =
stderr_output[0]
On 2012/04/02 20:31:41, Nick Bray wrote:
> Scraping for failiures => function (but not printing the failiure)

Added --verbose flag that does emit it.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:224: manifest_data =
manifest_data.replace(u'\ufeff', '')
On 2012/04/02 20:31:41, Nick Bray wrote:
> What are these characters?  Document.

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:228: return {}
On 2012/04/02 20:31:41, Nick Bray wrote:
> This seems a little sketchy.  Why return an empty dict?

Simplifies the logic of checking for a nested key within it.
ie.
x.get('foo', {}).get('bar', {}).get('baz')

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:254: def
Sha1FromFilename(filename):
On 2012/04/02 20:31:41, Nick Bray wrote:
> Are there functions here that you should be sharing with the validator script?

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:269: def PrimeCache(options,
filename):
On 2012/04/02 20:31:41, Nick Bray wrote:
> Dito.

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:294: assert not
os.path.exists(dest)
On 2012/04/02 20:31:41, Nick Bray wrote:
> If you have an assert, have a message to help explain it.  dest, in this case,
> would be sufficient.

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:300: tpath = os.path.join(dest,
info.filename)
On 2012/04/02 20:31:41, Nick Bray wrote:
> Pwnage: Zipfile is from an untrusted source, filename could be an absolute
path,
> join discards everything before an absolute path, attacker could cause you to
> overwrite an arbitrary file.

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:327: if index > 0:
On 2012/04/02 20:31:41, Nick Bray wrote:
> ETA calculation and / or header printing in function?

Done.

http://codereview.chromium.org/9950041/diff/1/tests/startup_regression/startu...
tests/startup_regression/startup_regression.py:359: default_cache_dir = '/b'
On 2012/04/02 20:31:41, Nick Bray wrote:
> Another bit of code that might do well shared.

Done.

[bradn, 2012-04-03 01:01:28]

Changed per offline.
PTAL

[Nick Bray, 2012-04-03 20:34:29]

Almost there.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
File tests/abi_corpus/corpus_utils.py (right):

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:63: def DownloadCorpusTotalList(list_filename):
Optional: DownloadCorpusCRXList?

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:159: dst.append(fh.read())
Document why you're doing this.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:164: stdout_thread.setDaemon(True)
Why a daemon thread?  Comment.  If you're worried about behavior on exception, a
finally block might be the best bet.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:194: def CachedPath(options, filename):
Passing "options" through is a little sketchy, now that this function is used by
two scripts with different sets of options.  Expand options into explicit
arguments wherever prudent?

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:270: assert not os.path.isabs(info.filename)
and '..' not in info.filename
Factor assert + join into a function.
Change assert into an if/raise.  (Asserts can get compiled out.)

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:274: for info in zf.infolist():
Optional: Instead of two loops, you could skip directories and then create the
directory for each file, if it does not exist.  In this case there'd be no
common code to factor out so you could partially ignore the previous comment.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:373: def SetupOptions(parser):
If you eliminate the "options" arguments as I suggested, you should also inline
this function into each file.  (The code duplication would not be that bad
because argument parsing is "scripty" and not core functionality, in my mind.)

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
File tests/abi_corpus/startup_regression_test.py (right):

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:15: BAD_MANIFEST = r'^BAD
MANIFEST!$'
Gratuitous raw string.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:18: KNOWN_BAD = {
Very optional: you could split this config stuff out as another module.  This
would allow "routine" changes to not touch the main script - the intent of the
change would be immediately obvious.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:79: if expected_result.match('BAD
MANIFEST!'):
This is a little sketchy, but I don't have a good suggestion.  Split the config
up, have explicit sets of hashes for each type of failure that doesn't really
require a regex?  Sorta ugly, too.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:87: '--enable-nacl',
Check out toosl/browser_tester/browsertester/browserlauncher.py for other
command line arguments you might want.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:155: raise Exception('Platform not
yet supported')
Comment: why?

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/validator_r...
File tests/abi_corpus/validator_regression_test.py (right):

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/validator_r...
tests/abi_corpus/validator_regression_test.py:56: print 'Failed on sha1: %s' %
corpus_utils.Sha1FromFilename(path)
Very optional because this is not under review: you might want to explain if
this was a false positive or a false negative.

[bradn, 2012-04-12 17:45:54]

PTAL

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
File tests/abi_corpus/corpus_utils.py (right):

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:63: def DownloadCorpusTotalList(list_filename):
On 2012/04/03 20:34:29, Nick Bray wrote:
> Optional: DownloadCorpusCRXList?

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:159: dst.append(fh.read())
On 2012/04/03 20:34:29, Nick Bray wrote:
> Document why you're doing this.

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:164: stdout_thread.setDaemon(True)
On 2012/04/03 20:34:29, Nick Bray wrote:
> Why a daemon thread?  Comment.  If you're worried about behavior on exception,
a
> finally block might be the best bet.
Cargo cult from the innards of subprocess. Dropped it as its not likely needed
in this case.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:194: def CachedPath(options, filename):
On 2012/04/03 20:34:29, Nick Bray wrote:
> Passing "options" through is a little sketchy, now that this function is used
by
> two scripts with different sets of options.  Expand options into explicit
> arguments wherever prudent?

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:270: assert not os.path.isabs(info.filename)
and '..' not in info.filename
On 2012/04/03 20:34:29, Nick Bray wrote:
> Factor assert + join into a function.
> Change assert into an if/raise.  (Asserts can get compiled out.)

Did the suggestion below instead.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:274: for info in zf.infolist():
On 2012/04/03 20:34:29, Nick Bray wrote:
> Optional: Instead of two loops, you could skip directories and then create the
> directory for each file, if it does not exist.  In this case there'd be no
> common code to factor out so you could partially ignore the previous comment.

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/corpus_util...
tests/abi_corpus/corpus_utils.py:373: def SetupOptions(parser):
On 2012/04/03 20:34:29, Nick Bray wrote:
> If you eliminate the "options" arguments as I suggested, you should also
inline
> this function into each file.  (The code duplication would not be that bad
> because argument parsing is "scripty" and not core functionality, in my mind.)

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
File tests/abi_corpus/startup_regression_test.py (right):

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:15: BAD_MANIFEST = r'^BAD
MANIFEST!$'
On 2012/04/03 20:34:29, Nick Bray wrote:
> Gratuitous raw string.

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:18: KNOWN_BAD = {
On 2012/04/03 20:34:29, Nick Bray wrote:
> Very optional: you could split this config stuff out as another module.  This
> would allow "routine" changes to not touch the main script - the intent of the
> change would be immediately obvious.

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:79: if expected_result.match('BAD
MANIFEST!'):
On 2012/04/03 20:34:29, Nick Bray wrote:
> This is a little sketchy, but I don't have a good suggestion.  Split the
config
> up, have explicit sets of hashes for each type of failure that doesn't really
> require a regex?  Sorta ugly, too.

Added a CrxResult class.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:87: '--enable-nacl',
On 2012/04/03 20:34:29, Nick Bray wrote:
> Check out toosl/browser_tester/browsertester/browserlauncher.py for other
> command line arguments you might want.

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/startup_reg...
tests/abi_corpus/startup_regression_test.py:155: raise Exception('Platform not
yet supported')
On 2012/04/03 20:34:29, Nick Bray wrote:
> Comment: why?

Done.

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/validator_r...
File tests/abi_corpus/validator_regression_test.py (right):

http://codereview.chromium.org/9950041/diff/4008/tests/abi_corpus/validator_r...
tests/abi_corpus/validator_regression_test.py:56: print 'Failed on sha1: %s' %
corpus_utils.Sha1FromFilename(path)
On 2012/04/03 20:34:29, Nick Bray wrote:
> Very optional because this is not under review: you might want to explain if
> this was a false positive or a false negative.

Done.

[bradn, 2012-04-12 18:55:17]

Simplified the result stuff even more.
Further experience with the drift on errors messages suggests cataloging new
ones as they appear.

[Nick Bray (chromium), 2012-04-12 21:33:03]

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_err...
File tests/abi_corpus/corpus_errors.py (right):

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_err...
tests/abi_corpus/corpus_errors.py:18: def __init__(self, id, custom=None,
precidence=1):
custom unused?

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_err...
tests/abi_corpus/corpus_errors.py:32: def Merge(self, other):
This seems a little questionable.  Will there ever be more than one error?

If you keep it, document what you're doing and why.

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
File tests/abi_corpus/corpus_utils.py (right):

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
tests/abi_corpus/corpus_utils.py:87:
GsutilCopySilent('gs://nativeclient-snaps/naclapps.list', filename)
Why not DownloadFileFromCorpus?

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
tests/abi_corpus/corpus_utils.py:94: def DownloadFileFromCorpus(src_path,
dst_filename):
Move up, after GsutilCopySilent?

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
tests/abi_corpus/corpus_utils.py:272: assert not os.path.isabs(info.filename)
and '..' not in info.filename
asserts can get compiled out and this is (potentially) a security check.  Raise?

[bradn, 2012-04-13 00:14:50]

PTAL, will fix the CrxResult thing in a CL soon to come.

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
File tests/abi_corpus/corpus_utils.py (right):

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
tests/abi_corpus/corpus_utils.py:87:
GsutilCopySilent('gs://nativeclient-snaps/naclapps.list', filename)
On 2012/04/12 21:33:03, Nick Bray (chromium) wrote:
> Why not DownloadFileFromCorpus?

Done.

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
tests/abi_corpus/corpus_utils.py:94: def DownloadFileFromCorpus(src_path,
dst_filename):
On 2012/04/12 21:33:03, Nick Bray (chromium) wrote:
> Move up, after GsutilCopySilent?

Done.

http://codereview.chromium.org/9950041/diff/12001/tests/abi_corpus/corpus_uti...
tests/abi_corpus/corpus_utils.py:272: assert not os.path.isabs(info.filename)
and '..' not in info.filename
On 2012/04/12 21:33:03, Nick Bray (chromium) wrote:
> asserts can get compiled out and this is (potentially) a security check. 
Raise?

Done.

[Nick Bray, 2012-04-13 00:30:20]

LGTM assuming followup.