Refactor the way that TestApi works so that it is actually useful.

Refactor the way that TestApi works so that it is actually useful.

I think this is a reasonable compromise beteween a couple of the options we
discussed. It basically boils down to two things:
  * Modules now have a test_api.
    * This is injected into the api's "test_api" member.
    * This is also provided in a module tree to the GenTests api parameter
      exactly the way that the normal module tree is provided. The recipe's DEPS
      variable determines which test api's will be available to the recipe. If
      the module depends on others, the other modules' test_api's will be
      injected into the test_api's "m" member.
  * Test data is now represented by a bunch of add-able TestData objects
  * Better auto-routing of test information from the test_api's to the actual objects which consume it.
  * Steps can now have default_step_data, which should be an instance of StepTestData. This allows modules to provide mock data for themselves.
  * I broke up recipe_api into a bunch of (hopefully) more-logical pieces
  * Expanded coverage to include module examples and test_api's
  * Standardized module/recipe loading (and the path which they use for lookups).

R=agable@chromium.org
BUG=292270, 292298
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=225021

[agable, 2013-09-16 20:29:24]

https://codereview.chromium.org/23889036/diff/6001/scripts/slave/recipe_api.py
File scripts/slave/recipe_api.py (right):

https://codereview.chromium.org/23889036/diff/6001/scripts/slave/recipe_api.p...
scripts/slave/recipe_api.py:146: for v in submod.api.__dict__.itervalues():
Since the API is the most basic part of the module, and the config and test_api
are secondary, either move this above line 138 or move the config block below
the test_api block.

https://codereview.chromium.org/23889036/diff/6001/scripts/slave/recipe_api.p...
scripts/slave/recipe_api.py:233: def CreateApi(mod_dirs, names, mocks=None,
required=None,
If 'required' and 'optional' are not antitheses of each other, their names
shouldn't be.

https://codereview.chromium.org/23889036/diff/6001/scripts/slave/recipe_api.p...
scripts/slave/recipe_api.py:236: Given a list of module names, return an
instance of RecipeApi which contains
Update docstring to reflect new argument ordering.

https://codereview.chromium.org/23889036/diff/6001/scripts/slave/recipe_api.p...
scripts/slave/recipe_api.py:258: if required:
Definitely not a fan of the 'required' and 'optional' names. Better docstring
will help, but something else needs to be fixed here too. Still thinking about
it.

https://codereview.chromium.org/23889036/diff/6001/scripts/slave/recipe_api.p...
scripts/slave/recipe_api.py:277: inst_maps[optional[0]][name] =
api(module=module)
As long as you're treating these as "required" and "optional" rather than "real"
and "test", they should both have mocks. I think we should just be explicit
about the test-helper nature of "optional".

https://codereview.chromium.org/23889036/diff/6001/scripts/slave/recipe_api.p...
scripts/slave/recipe_api.py:292: def MapDependencies(dep_map, inst_map):
While we're here, please rename "to_remove" and "to_pop" to better names: e.g.
"done_deps" and "satisfied_modules" or something like that.

[agable, 2013-09-16 21:45:29]

So, to follow up with what we discussed:

Rather than having a 'mock_fn' attribute on steps, we will use the TestApi
classes to directly provide those mocks. Every time a RecipeApi subclass calls
one of its member methods that has been decorated with an "@auto_add_test_data"
decorator, it will call the method, assert that it returns a single step object,
call the TestApi version of the same method with the same arguments, merge the
resulting test_data into the step object, and return. This way the APIs and
recipes won't have to directly encode mocks, and the TestApis will be more like
true mocks and therefore less confusing to the casual observer.

[iannucci, 2013-09-19 09:16:05]

On 2013/09/16 21:45:29, Aaron Gable wrote:
> So, to follow up with what we discussed:
> 
> Rather than having a 'mock_fn' attribute on steps, we will use the TestApi
> classes to directly provide those mocks. Every time a RecipeApi subclass calls
> one of its member methods that has been decorated with an
"@auto_add_test_data"
> decorator, it will call the method, assert that it returns a single step
object,
> call the TestApi version of the same method with the same arguments, merge the
> resulting test_data into the step object, and return. This way the APIs and
> recipes won't have to directly encode mocks, and the TestApis will be more
like
> true mocks and therefore less confusing to the casual observer.

Ok.... things happened. PTAL.

[iannucci, 2013-09-19 09:45:00]

On 2013/09/19 09:16:05, iannucci wrote:
> On 2013/09/16 21:45:29, Aaron Gable wrote:
> > So, to follow up with what we discussed:
> > 
> > Rather than having a 'mock_fn' attribute on steps, we will use the TestApi
> > classes to directly provide those mocks. Every time a RecipeApi subclass
calls
> > one of its member methods that has been decorated with an
> "@auto_add_test_data"
> > decorator, it will call the method, assert that it returns a single step
> object,
> > call the TestApi version of the same method with the same arguments, merge
the
> > resulting test_data into the step object, and return. This way the APIs and
> > recipes won't have to directly encode mocks, and the TestApis will be more
> like
> > true mocks and therefore less confusing to the casual observer.
> 
> Ok.... things happened. PTAL.

(and yes, I know this CL is huge, sorry about that :()

[iannucci, 2013-09-19 21:28:19]

https://codereview.chromium.org/23889036/diff/15001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/chromium_android/api.py (left):

https://codereview.chromium.org/23889036/diff/15001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium_android/api.py:36: s.custom_vars =
self.c.custom_vars
I took this out because it wasn't working (not sure how it was passing), and
it's also not used for anything.

[agable, 2013-09-21 02:05:31]

Buncha random comments that so far add up to "not quite lg2me yet".

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

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/annotated_r...
scripts/slave/annotated_run.py:264: setattr(o, placeholder_name, lst[0])
Why? Why not just put the whole list in placeholder_name and let people index it
on their own if they really only want the first entry?

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/path/test_api.py (right):

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/path/test_api.py:5: @staticmethod
Reverse the order of these decorators?

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/raw/__init__.py (right):

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/raw/__init__.py:1: DEPS = []
Not sure I agree with the naming of the 'raw' module. The "raw.input" is cute,
but "raw" by itself is incredibly nondescript.

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_test...
File scripts/slave/recipe_test_api.py (right):

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_test...
scripts/slave/recipe_test_api.py:28: self.enabled = True
What's enabled for?

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_test...
scripts/slave/recipe_test_api.py:47: def __add__(self, other):
__add__ doesn't have to be simple concatenation. Would it make more sense to put
the 'append' code here and call this code something else?

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_test...
scripts/slave/recipe_test_api.py:211: def Test(name):
The capitalization in these is disconcerting compared to the conventions we've
adopted in the non-test APIs.

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

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_util...
scripts/slave/recipe_util.py:47: return self._name
_name should always be a function of name_pieces, in case _name_pieces gets
changed after init (which shouldn't happen, but still...)

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipes/v8.py
File scripts/slave/recipes/v8.py (right):

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipes/v8....
scripts/slave/recipes/v8.py:36: api.Properties(build_config=build_config,
target_arch='mips')
The fact that these are capitalized in GenTests but never in GenSteps makes me
sad. Even though the reason is clear -- in Steps we're using the api to access
its methods, and in Tests we're using it to construct a fake -- it looks and
feels weird. The whole point of this was to 'unify' APIs across steps and tests.

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipes/v8....
scripts/slave/recipes/v8.py:36: api.Properties(build_config=build_config,
target_arch='mips')
build_config doesn't have a well-defined value here. This yield is outside all
loops.

[iannucci, 2013-09-21 03:12:33]

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

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/anno...
scripts/slave/annotated_run.py:264: setattr(o, placeholder_name, lst[0])
On 2013/09/21 02:05:31, Aaron Gable wrote:
> Why? Why not just put the whole list in placeholder_name and let people index
it
> on their own if they really only want the first entry?

Because people /mostly always/ want the first entry (but not always always). I
can do this if you really want, but it results in a lot of [0]'s :/.

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
File scripts/slave/recipe_modules/path/test_api.py (right):

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipe_modules/path/test_api.py:5: @staticmethod
On 2013/09/21 02:05:31, Aaron Gable wrote:
> Reverse the order of these decorators?

Nope, staticmethod is weird.

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
File scripts/slave/recipe_modules/raw/__init__.py (right):

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipe_modules/raw/__init__.py:1: DEPS = []
On 2013/09/21 02:05:31, Aaron Gable wrote:
> Not sure I agree with the naming of the 'raw' module. The "raw.input" is cute,
> but "raw" by itself is incredibly nondescript.

well, I was also going to add an output() too, but I didn't yet.

renamed to raw_io

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
File scripts/slave/recipe_test_api.py (right):

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipe_test_api.py:28: self.enabled = True
On 2013/09/21 02:05:31, Aaron Gable wrote:
> What's enabled for?

Any time a TestData-ish object is passed to anything, it's expected to check
thing.enabled. It's true for all the 'live' TestData objects, and False for the
DisabledTestData object. I pulled this into a base class.

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipe_test_api.py:47: def __add__(self, other):
On 2013/09/21 02:05:31, Aaron Gable wrote:
> __add__ doesn't have to be simple concatenation. Would it make more sense to
put
> the 'append' code here and call this code something else?

So for StepTestData, there are two axes of concatenation:

{ ('module', 'name'): [ datum1 ] }.append({ ('module', 'name'): [ datum2 ] } )
->
{ ('module', 'name'): [ datum1, datum2 ] }

and

{ ('module', 'name'): [ datum1 ] } + { ('module', 'othername'): [ datum1 ] } )
->
{ ('module', 'name'): [ datum1 ], ('module', 'othername'): [ datum2 ] }
 
When writing tests, the second form is much more common (so far). I didn't want
it to automatically incorporate the first form behavior because it makes it
tricky to override a step's default data instead of just adding to it. I'll
think about this some more.

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipe_test_api.py:211: def Test(name):
On 2013/09/21 02:05:31, Aaron Gable wrote:
> The capitalization in these is disconcerting compared to the conventions we've
> adopted in the non-test APIs.

yep, you're right. This is vestigial. Fixed (and nuked the Properties method for
good measure)

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

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipe_util.py:47: return self._name
On 2013/09/21 02:05:31, Aaron Gable wrote:
> _name should always be a function of name_pieces, in case _name_pieces gets
> changed after init (which shouldn't happen, but still...)

Yeah, ok, good point. Done.

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
File scripts/slave/recipes/v8.py (right):

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipes/v8.py:36: api.Properties(build_config=build_config,
target_arch='mips')
On 2013/09/21 02:05:31, Aaron Gable wrote:
> The fact that these are capitalized in GenTests but never in GenSteps makes me
> sad. Even though the reason is clear -- in Steps we're using the api to access
> its methods, and in Tests we're using it to construct a fake -- it looks and
> feels weird. The whole point of this was to 'unify' APIs across steps and
tests.

Yeah, Done.

https://chromiumcodereview.appspot.com/23889036/diff/21001/scripts/slave/reci...
scripts/slave/recipes/v8.py:36: api.Properties(build_config=build_config,
target_arch='mips')
On 2013/09/21 02:05:31, Aaron Gable wrote:
> build_config doesn't have a well-defined value here. This yield is outside all
> loops.

Oops. Additional unit test fail. Done

[agable, 2013-09-23 22:57:57]

This now looks good modulo a couple small comments and one big one (re: addition
on StepTestData). Fix commutativity and I'll approve.

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_test...
File scripts/slave/recipe_test_api.py (right):

https://codereview.chromium.org/23889036/diff/21001/scripts/slave/recipe_test...
scripts/slave/recipe_test_api.py:47: def __add__(self, other):
On 2013/09/21 03:12:34, iannucci wrote:
> On 2013/09/21 02:05:31, Aaron Gable wrote:
> > __add__ doesn't have to be simple concatenation. Would it make more sense to
> put
> > the 'append' code here and call this code something else?
> 
> So for StepTestData, there are two axes of concatenation:
> 
> { ('module', 'name'): [ datum1 ] }.append({ ('module', 'name'): [ datum2 ] } )
> ->
> { ('module', 'name'): [ datum1, datum2 ] }
> 
> and
> 
> { ('module', 'name'): [ datum1 ] } + { ('module', 'othername'): [ datum1 ] } )
> ->
> { ('module', 'name'): [ datum1 ], ('module', 'othername'): [ datum2 ] }
>  
> When writing tests, the second form is much more common (so far). I didn't
want
> it to automatically incorporate the first form behavior because it makes it
> tricky to override a step's default data instead of just adding to it. I'll
> think about this some more.

I still don't really like this. If I have { (a): [a1], (b): [b1] } + { (a):
[a2], (c): [c2] } I'm *definitely* going to expect { (a): [a1, a2], (b): [b1],
(c): [c1] }. I'm a firm believer that operator overloads should absolutely
preserve algebraic behavior. The current implementation breaks commutativity.

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/chromium_android/api.py (right):

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/chromium_android/api.py:14: def get_env(self):
Looks like this snuck in from the rest of your android work? Oh it's probably
just rietveld being bad at rebases. Nvm.

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/raw_io/api.py (right):

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/raw_io/api.py:31: class
RawApi(recipe_api.RecipeApi):
a) This class name should match the file name.
b) Either make this a true raw_io class and stub out the other io methods, or
make it an InputApi.

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_test...
File scripts/slave/recipe_test_api.py (right):

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_test...
scripts/slave/recipe_test_api.py:211: class RecipeTestApi(object):
You're going to have to document this *really well*. In the recipe world,
virtually all members of api (api.properties, api.step, etc) are explicitly
declared in DEPS (and those that aren't, e.g. apply_config, I think are
confusing already and would like to move somewhere else, though I don't know
where.). For testing, there are suddenly these two extra test and step_data
methods that magically appear. Document these (and the methods on RecipeApi
itself) really really really really well.

[iannucci, 2013-09-24 02:18:50]

ptal

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_modu...
File scripts/slave/recipe_modules/raw_io/api.py (right):

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_modu...
scripts/slave/recipe_modules/raw_io/api.py:31: class
RawApi(recipe_api.RecipeApi):
On 2013/09/23 22:57:58, Aaron Gable wrote:
> a) This class name should match the file name.
> b) Either make this a true raw_io class and stub out the other io methods, or
> make it an InputApi.

a) yes. Done.
b) implemented output, made json.output use it (ptal).

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_test...
File scripts/slave/recipe_test_api.py (right):

https://codereview.chromium.org/23889036/diff/34001/scripts/slave/recipe_test...
scripts/slave/recipe_test_api.py:211: class RecipeTestApi(object):
On 2013/09/23 22:57:58, Aaron Gable wrote:
> You're going to have to document this *really well*. In the recipe world,
> virtually all members of api (api.properties, api.step, etc) are explicitly
> declared in DEPS (and those that aren't, e.g. apply_config, I think are
> confusing already and would like to move somewhere else, though I don't know
> where.). For testing, there are suddenly these two extra test and step_data
> methods that magically appear. Document these (and the methods on RecipeApi
> itself) really really really really well.

Well... with a data set of 1, all things are unexpected.

I think the basic flow is:
  * GenSteps gets a RecipeApi (which has some methods). Into that object are
also injected all DEPS api's
  * GenTests gets a RecipeTestApi (which has some methods). Into that object are
also injected all test_api's.

They do need docs though.

[agable, 2013-09-24 16:38:32]

lgtm

[commit-bot: I haz the power, 2013-09-24 17:26:06]

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

[commit-bot: I haz the power, 2013-09-24 17:27:26]

Presubmit check for 23889036-114001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  build/scripts/slave/recipe_loader.py \
  build/scripts/slave/recipe_test_api.py \
  build/scripts/slave/recipe_util.py

Presubmit checks took 48.6s to calculate.

[commit-bot: I haz the power, 2013-09-24 17:30:06]

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

[commit-bot: I haz the power, 2013-09-24 17:32:50]

Change committed as 225021