First cut of testing infrastructure for recipes.

First cut of testing infrastructure for recipes.

R=agable@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200078

[iannucci, 2013-05-10 09:27:06]

So there's a catch. Right now the tests require you to `pip install coverage`.
I'd like to include coverage in third_party, but it has a non-trivial C
extension component, so I'm not sure if it's even reasonable to distribute in
that manner (though I could use distutils to autobuid it...).

Other than that, it's pretty cool. PTAL.

[iannucci, 2013-05-10 09:27:06]

So there's a catch. Right now the tests require you to `pip install coverage`.
I'd like to include coverage in third_party, but it has a non-trivial C
extension component, so I'm not sure if it's even reasonable to distribute in
that manner (though I could use distutils to autobuid it...).

Other than that, it's pretty cool. PTAL.

[iannucci, 2013-05-10 09:30:30]

On 2013/05/10 09:27:06, iannucci wrote:
> So there's a catch. Right now the tests require you to `pip install coverage`.
> I'd like to include coverage in third_party, but it has a non-trivial C
> extension component, so I'm not sure if it's even reasonable to distribute in
> that manner (though I could use distutils to autobuid it...).
> 
> Other than that, it's pretty cool. PTAL.

It's also missing a means to capture the annotated_checkout commands that could
have run (perhaps we should generate the 'initial checkout' + 'clean' + 'update'
commands even though they wouldn't all run at the same time).

Also, please feel free to nitpick. I just got it working and haven't cleaned it
up at all.

[Vadim Shtayura, 2013-05-11 00:33:32]

There's more and more magic in recipes framework...

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/annotated_ch...
File scripts/slave/annotated_checkout.py (right):

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/annotated_ch...
scripts/slave/annotated_checkout.py:182: def setup(self, *args, **kwargs):
Why 'args' and 'kwargs' are here considering they are not used and
Checkout.setup doesn't expect them?

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/annotated_ch...
scripts/slave/annotated_checkout.py:224: def run(checkout_type, checkout_spec,
test_mode=False):
Update 'Args' section of the doc string to include 'test_mode'.

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/annotated_ru...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/annotated_ru...
scripts/slave/annotated_run.py:111: if isinstance(ret, int):
Maybe make 'make_steps' to return a tuple (status code, script, steps) where
each element is not None when it makes sense? It'll make code that calls
make_steps more readable IMO.

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/annotated_ru...
scripts/slave/annotated_run.py:126: factory_properties = factory_properties
Hmm...

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/unittests/re...
File scripts/slave/unittests/recipes_test.py (right):

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/unittests/re...
scripts/slave/unittests/recipes_test.py:34: @contextlib.contextmanager
Add newline above this line.

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/unittests/re...
scripts/slave/unittests/recipes_test.py:97:
annotator.StructuredAnnotationStream(stream=DevNull()), bp, fp, True)
Open real /dev/null? :)

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/unittests/re...
scripts/slave/unittests/recipes_test.py:137: def add_tests(_base_dir_type,
recipe_path, loader, suite):
For readability reasons this stuff probably can be converted to some metaclass's
__init__ method. That function is basically a metaclass. Explicitly stating it
as such will make this code more readable IMO. But I'm not sure :)

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/unittests/re...
scripts/slave/unittests/recipes_test.py:158: def loop_over_recipes(func, *args,
**kwargs):
Maybe convert to a generator? 'had_errors' is used only with 'train_from_tests'
so far and so can be implemented right there (generators can't have return
values).

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/unittests/re...
scripts/slave/unittests/recipes_test.py:167: execfile(recipe_path, {})
Do we care about an error in a single recipe blocking tests of other recipes? If
we are, then we need to wrap 'execfile' in exception blocks everywhere.

https://codereview.chromium.org/14988009/diff/4002/scripts/slave/unittests/re...
scripts/slave/unittests/recipes_test.py:181: if '--help' in argv or '-h' in
argv:
Use ArgumentParser or OptionParser?

[iannucci, 2013-05-11 04:12:27]

PTAL

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/annot...
File scripts/slave/annotated_checkout.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/annot...
scripts/slave/annotated_checkout.py:182: def setup(self, *args, **kwargs):
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Why 'args' and 'kwargs' are here considering they are not used and
> Checkout.setup doesn't expect them?

Done. Not sure how that got there...

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/annot...
scripts/slave/annotated_checkout.py:224: def run(checkout_type, checkout_spec,
test_mode=False):
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Update 'Args' section of the doc string to include 'test_mode'.

Done.

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/annot...
File scripts/slave/annotated_run.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/annot...
scripts/slave/annotated_run.py:111: if isinstance(ret, int):
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Maybe make 'make_steps' to return a tuple (status code, script, steps) where
> each element is not None when it makes sense? It'll make code that calls
> make_steps more readable IMO.

Done. (Actually made it a namedtuple)

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/annot...
scripts/slave/annotated_run.py:126: factory_properties = factory_properties
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Hmm...

Oops. Find/replace :D

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/unitt...
File scripts/slave/unittests/recipes_test.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:34: @contextlib.contextmanager
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Add newline above this line.

Done.

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:97:
annotator.StructuredAnnotationStream(stream=DevNull()), bp, fp, True)
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Open real /dev/null? :)

No bueno on windows :/

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:137: def add_tests(_base_dir_type,
recipe_path, loader, suite):
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> For readability reasons this stuff probably can be converted to some
metaclass's
> __init__ method. That function is basically a metaclass. Explicitly stating it
> as such will make this code more readable IMO. But I'm not sure :)

Hm... I'll take a whack at this, but I'm not sure it'll actually be better. I'll
still have to dynamically create the test methods. It may allow me to contain
the magic in one spot more though.

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:158: def loop_over_recipes(func, *args,
**kwargs):
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Maybe convert to a generator? 'had_errors' is used only with
'train_from_tests'
> so far and so can be implemented right there (generators can't have return
> values).

Done.

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:167: execfile(recipe_path, {})
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Do we care about an error in a single recipe blocking tests of other recipes?
If
> we are, then we need to wrap 'execfile' in exception blocks everywhere.

I think the answer is 'maybe'. I'm OK with having the exception bubble up to the
top. If the recipe is bailing out with an exception, we pretty much need to know
immediately. If it's bailing on presubmit, then it's basically unsubmittable
code anyhow. Unless you feel strongly here, I'm inclined to leave it as is.

https://chromiumcodereview.appspot.com/14988009/diff/4002/scripts/slave/unitt...
scripts/slave/unittests/recipes_test.py:181: if '--help' in argv or '-h' in
argv:
On 2013/05/11 00:33:32, Vadim Shtayura wrote:
> Use ArgumentParser or OptionParser?

Unfortunately we can't because of the way that unittest.main works :/. unittest
will barf if it sees the extra arg (so we have to delete from argv), and
optparse doesn't like to let not exit after printing help (so you have to catch
SystemExit). It turns out to be 2x the lines.

This is the least-bad way I've found to add arguments to a unittest, but I
agree, it sucks.

[Isaac (away), 2013-05-12 09:24:56]

Drive by comments, hope that's OK.

https://codereview.chromium.org/14988009/diff/15001/scripts/slave/annotated_c...
File scripts/slave/annotated_checkout.py (right):

https://codereview.chromium.org/14988009/diff/15001/scripts/slave/annotated_c...
scripts/slave/annotated_checkout.py:241: stream = StringIO.StringIO()
Would open(os.devnull, 'w') work?

https://codereview.chromium.org/14988009/diff/15001/scripts/slave/annotated_r...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/14988009/diff/15001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:244: proc.communicate('')
I wonder if 244/245 could be:

proc.stdin.close()
ret = proc.wait()

Could we add a comment to their purpose?

https://codereview.chromium.org/14988009/diff/15001/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14988009/diff/15001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:72: path_func_name = (name+"_path")
I'd suggest running gpylint on these files and fixing all the reasonable
suggestions.  For example: spaces around + sign and extraneous parens.

[iannucci, 2013-05-14 04:31:35]

PTAL. I refactored the magic somewhat. Should be much more readable now :)

https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
File scripts/slave/annotated_checkout.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
scripts/slave/annotated_checkout.py:241: stream = StringIO.StringIO()
On 2013/05/12 09:24:56, Isaac wrote:
> Would open(os.devnull, 'w') work?

Actually yes, that would work. For some reason I forgot that os.devnull is 'nul'
on windows. There's another instance of this in the other script, too.

https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
File scripts/slave/annotated_run.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
scripts/slave/annotated_run.py:244: proc.communicate('')
On 2013/05/12 09:24:56, Isaac wrote:
> I wonder if 244/245 could be:
> 
> proc.stdin.close()
> ret = proc.wait()
> 
> Could we add a comment to their purpose?

It could be... Is there some advantage to doing that? Subprocess docs imply that
communicate is superior in all ways...

Also, what do you mean by comment to their purpose?

[Isaac (away), 2013-05-14 10:49:20]

https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
File scripts/slave/annotated_run.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
scripts/slave/annotated_run.py:244: proc.communicate('')
On 2013/05/14 04:31:35, iannucci wrote:
> On 2013/05/12 09:24:56, Isaac wrote:
> > I wonder if 244/245 could be:
> > 
> > proc.stdin.close()
> > ret = proc.wait()
> > 
> > Could we add a comment to their purpose?
> 
> It could be... Is there some advantage to doing that? Subprocess docs imply
that
> communicate is superior in all ways...
> 
> Also, what do you mean by comment to their purpose?

I usually don't pipe to stdin when I don't have information to send. 
crrev.com/12567022 added it.

[Mike Stip (use stip instead), 2013-05-14 18:01:32]

few comments, other than that lg2m. do you still need to pip install, or are the
includes here valid?

https://codereview.chromium.org/14988009/diff/40001/scripts/slave/annotated_r...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/14988009/diff/40001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:112: assert ret.script is None, "Unexpectedly got
script from make_steps?"
can you put a comment here saying that script is meant for use in
recipes_test.py? otherwise to the casual observer it looks like the script
argument can never be used

https://codereview.chromium.org/14988009/diff/40001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:125: MakeStepsRetval =
namedtuple('MakeStepsRetval', 'status_code script steps')
didn't know about namedtuple. cool!

[iannucci, 2013-05-14 18:48:20]

On 2013/05/14 10:49:20, Isaac wrote:
>
https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
> File scripts/slave/annotated_run.py (right):
> 
>
https://chromiumcodereview.appspot.com/14988009/diff/15001/scripts/slave/anno...
> scripts/slave/annotated_run.py:244: proc.communicate('')
> On 2013/05/14 04:31:35, iannucci wrote:
> > On 2013/05/12 09:24:56, Isaac wrote:
> > > I wonder if 244/245 could be:
> > > 
> > > proc.stdin.close()
> > > ret = proc.wait()
> > > 
> > > Could we add a comment to their purpose?
> > 
> > It could be... Is there some advantage to doing that? Subprocess docs imply
> that
> > communicate is superior in all ways...
> > 
> > Also, what do you mean by comment to their purpose?
> 
> I usually don't pipe to stdin when I don't have information to send. 
> crrev.com/12567022 added it.

I'm really not convinced that this is unclear. You need to set stdin to be a
PIPE in order to close it no matter what. Instead of accessing stdin, it's using
the subprocess recommended way of interacting with the file streams. The entire
thing is wrapped in a 'keep_stdin' boolean condition, which, in case the
variable is unclear, is derived from a documented command line parameter.

What's confusing?

[iannucci, 2013-05-14 19:18:14]

PTAL. I'd really like to land this today so I can land the enhanced recipe stuff
later today.

Thanks,
Rob

https://codereview.chromium.org/14988009/diff/40001/scripts/slave/annotated_r...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/14988009/diff/40001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:112: assert ret.script is None, "Unexpectedly got
script from make_steps?"
On 2013/05/14 18:01:32, Mike Stipicevic wrote:
> can you put a comment here saying that script is meant for use in
> recipes_test.py? otherwise to the casual observer it looks like the script
> argument can never be used

Good point, done.

https://codereview.chromium.org/14988009/diff/40001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:125: MakeStepsRetval =
namedtuple('MakeStepsRetval', 'status_code script steps')
On 2013/05/14 18:01:32, Mike Stipicevic wrote:
> didn't know about namedtuple. cool!

:)

[iannucci, 2013-05-14 19:20:29]

Oh, and this is fully standalone, btw. There's no need to pip install anything.

Once recipes start getting larger, the pure-python coverage module may start
getting slow, in which I can do some detection in test_env to see if the global
version of coverage is new enough and use that instead of the bundled version.
The global install includes a c module which really speeds up the data
collection a lot.

But right now the entire test takes < 1 second.

[agable, 2013-05-14 20:47:23]

LGTM with nitty comments. Go go go!

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/annotated_c...
File scripts/slave/annotated_checkout.py (right):

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/annotated_c...
scripts/slave/annotated_checkout.py:254: return (0, checkout.root())
And sometimes I wish GOTOs were socially acceptable :P

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/annotated_r...
File scripts/slave/annotated_run.py (left):

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:103: 'the build factory')
Thanks for removing these.

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/annotated_r...
File scripts/slave/annotated_run.py (right):

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:275: sys.exit(main())
Doesn't this now need to pass sys.argv?

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/recipe_util.py
File scripts/slave/recipe_util.py (right):

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:62: def mock_paths():
Docstring.

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/recipes_tes...
File scripts/slave/recipes_test/gatekeeper.py (right):

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/recipes_tes...
scripts/slave/recipes_test/gatekeeper.py:8: return {}
Empty test?

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/unittests/r...
File scripts/slave/unittests/recipes_test.py (right):

https://codereview.chromium.org/14988009/diff/52001/scripts/slave/unittests/r...
scripts/slave/unittests/recipes_test.py:61: return os.path.join(root,
'recipes_test', os.path.basename(recipe_path))
This would fail if we ever put recipes in categories within recipes/. But so
will other things, so that can get fixed later.

[iannucci, 2013-05-14 21:32:08]

Landing :)

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/anno...
File scripts/slave/annotated_checkout.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/anno...
scripts/slave/annotated_checkout.py:254: return (0, checkout.root())
On 2013/05/14 20:47:23, Aaron Gable wrote:
> And sometimes I wish GOTOs were socially acceptable :P

Or part of the language...

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/anno...
File scripts/slave/annotated_run.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/anno...
scripts/slave/annotated_run.py:275: sys.exit(main())
On 2013/05/14 20:47:23, Aaron Gable wrote:
> Doesn't this now need to pass sys.argv?

Well, it can, but doesn't technically need to, since argv=None will make
optparse go read sys.argv. I'll add it though to be less confusing :)

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/reci...
File scripts/slave/recipe_util.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/reci...
scripts/slave/recipe_util.py:62: def mock_paths():
On 2013/05/14 20:47:23, Aaron Gable wrote:
> Docstring.

Done. Good catch.

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/reci...
File scripts/slave/recipes_test/gatekeeper.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/reci...
scripts/slave/recipes_test/gatekeeper.py:8: return {}
On 2013/05/14 20:47:23, Aaron Gable wrote:
> Empty test?

Yeah, it takes no factory_properties or build_properties that I can tell. In
order to generate the test output only an empty test is necessary.

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/unit...
File scripts/slave/unittests/recipes_test.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/52001/scripts/slave/unit...
scripts/slave/unittests/recipes_test.py:61: return os.path.join(root,
'recipes_test', os.path.basename(recipe_path))
On 2013/05/14 20:47:23, Aaron Gable wrote:
> This would fail if we ever put recipes in categories within recipes/. But so
> will other things, so that can get fixed later.

Yeah I'm not sure what that would look like.

[commit-bot: I haz the power, 2013-05-14 21:34:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/14988009/21002

[Isaac (away), 2013-05-14 21:34:27]

https://chromiumcodereview.appspot.com/14988009/diff/40001/scripts/slave/reci...
File scripts/slave/recipe_util.py (right):

https://chromiumcodereview.appspot.com/14988009/diff/40001/scripts/slave/reci...
scripts/slave/recipe_util.py:72: path_func_name = (name+"_path")
did pylint really pass on this?  Maybe use gpylint and ignore the excessive
stuff?

[commit-bot: I haz the power, 2013-05-14 21:34:36]

Can't process patch for file
third_party/coverage-3.6/coverage/htmlfiles/keybd_open.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[iannucci, 2013-05-14 21:35:25]

On 2013/05/14 21:34:27, Isaac wrote:
>
https://chromiumcodereview.appspot.com/14988009/diff/40001/scripts/slave/reci...
> File scripts/slave/recipe_util.py (right):
> 
>
https://chromiumcodereview.appspot.com/14988009/diff/40001/scripts/slave/reci...
> scripts/slave/recipe_util.py:72: path_func_name = (name+"_path")
> did pylint really pass on this?  Maybe use gpylint and ignore the excessive
> stuff?

pylint did pass. I have another CL based on this one which gpylint's that file.

[iannucci, 2013-05-14 21:37:40]

Committed patchset #9 manually as r200078 (presubmit successful).