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

scripts/slave/recipe_api.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import copy
 import functools
 import imp
 import inspect
 import os
 import sys
 import tempfile
 
 
 class Placeholder(object):
   """Base class for json placeholders. Do not use directly."""
@@ skipping 26 matching lines @@
       return [self.input_file]
 
   def step_finished(self, presentation, step_result, test_data):
     if test_data is None:  # pragma: no cover
       os.unlink(self.input_file)
 
 
 class ModuleInjectionSite(object):
   pass
 
+class RecipeTestApi(object):
+  def __init__(self, module=None):
+    """Note: Injected dependencies are NOT available in __init__()."""
+    # If we're the 'root' api, inject directly into 'self'.
+    # Otherwise inject into 'self.m'
+    self.m = self if module is None else ModuleInjectionSite()
 
 class RecipeApi(object):
   """
   Framework class for handling recipe_modules.
 
   Inherit from this in your recipe_modules/<name>/api.py . This class provides
   wiring for your config context (in self.c and methods, and for dependency
   injection (in self.m).
 
   Dependency injection takes place in load_recipe_modules() below.
   """
   def __init__(self, module=None, mock=None, **_kwargs):
     """Note: Injected dependencies are NOT available in __init__()."""
     self.c = None
     self._module = module
     self._mock = mock
 
     # If we're the 'root' api, inject directly into 'self'.
     # Otherwise inject into 'self.m'
     self.m = self if module is None else ModuleInjectionSite()
 
+    # If our module has a test api, it gets injected here.
+    self.test_api = None
+
+
   def _sanitize_config_vars(self, config_name, CONFIG_VARS):
     params = self.get_config_defaults(config_name)
     params.update(CONFIG_VARS)
     return params
 
   def get_config_defaults(self, _config_name):  # pylint: disable=R0201
     """
     Allows your api to dynamically determine static default values for configs.
     """
     return {}
@@ skipping 32 matching lines @@
 
   def apply_config(self, config_name, config_object=None):
     """Apply a named configuration to the provided config object or self."""
     self._module.CONFIG_CTX.CONFIG_ITEMS[config_name](config_object or self.c)
 
 
 def load_recipe_modules(mod_dirs):
   def patchup_module(submod):
     submod.CONFIG_CTX = getattr(submod, 'CONFIG_CTX', None)
     submod.API = getattr(submod, 'API', None)
+    submod.TEST_API = getattr(submod, 'TEST_API', None)
     submod.DEPS = frozenset(getattr(submod, 'DEPS', ()))
 
     if hasattr(submod, 'config'):
       for v in submod.config.__dict__.itervalues():
         if hasattr(v, 'I_AM_A_CONFIG_CTX'):
           assert not submod.CONFIG_CTX, (
             'More than one configuration context: %s' % (submod.config))
           submod.CONFIG_CTX = v
       assert submod.CONFIG_CTX, 'Config file, but no config context?'
 
     for v in submod.api.__dict__.itervalues():

[agable, 2013/09/16 20:29:25]

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.

       if inspect.isclass(v) and issubclass(v, RecipeApi):
         assert not submod.API, (
           'More than one Api subclass: %s' % submod.api)
         submod.API = v
 
     assert submod.API, 'Submodule has no api? %s' % (submod)
 
+    if hasattr(submod, 'test_api'):
+      for v in submod.test_api.__dict__.itervalues():
+        if inspect.isclass(v) and issubclass(v, RecipeTestApi):
+          assert not submod.TEST_API, (
+            'More than one TestApi subclass: %s' % submod.api)
+          submod.TEST_API = v
+      assert submod.API, (
+        'Submodule has test_api.py but no TestApi subclass? %s'
+        % (submod)
+      )
+
   RM = 'RECIPE_MODULES'
   def find_and_load(fullname, modname, path):
     if fullname not in sys.modules or fullname == RM:
       try:
         fil, pathname, descr = imp.find_module(modname,
                                                [os.path.dirname(path)])
         imp.load_module(fullname, fil, pathname, descr)
       finally:
         if fil:
           fil.close()
@@ skipping 48 matching lines @@
 
       # Then import all the config extenders.
       for root in mod_dirs:
         if os.path.isdir(root):
           recursive_import(root)
     return sys.modules[RM]
   finally:
     imp.release_lock()
 
 
-def CreateRecipeApi(names, mod_dirs, mocks=None, **kwargs):
+def CreateApi(mod_dirs, names, mocks=None, required=None,

[agable, 2013/09/16 20:29:25]

If 'required' and 'optional' are not antitheses of each other, their names
shouldn't be.

+              optional=None, kwargs=None):
   """
   Given a list of module names, return an instance of RecipeApi which contains

[agable, 2013/09/16 20:29:25]

Update docstring to reflect new argument ordering.

   those modules as direct members.
 
   So, if you pass ['foobar'], you'll get an instance back which contains a
   'foobar' attribute which itself is a RecipeApi instance from the 'foobar'
   module.
 
   Args:
     names (list): A list of module names to include in the returned RecipeApi.
     mod_dirs (list): A list of paths to directories which contain modules.
     mocks (dict): An optional dict of {<modname>: <mock data>}. Each module
         expects its own mock data.
-    **kwargs: Data passed to each module api. Usually this will contain:
+    kwargs: Data passed to each module api. Usually this will contain:
         properties (dict): the properties dictionary (used by the properties
             module)
         step_history (OrderedDict): the step history object (used by the
             step_history module!)
   """
-
+  kwargs = kwargs or {}
   recipe_modules = load_recipe_modules(mod_dirs)
 
-  inst_map = {None: RecipeApi()}
+  inst_maps = {}
+  if required:

[agable, 2013/09/16 20:29:25]

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.

+    inst_maps[required[0]] = { None: required[1]() }
+  if optional:
+    inst_maps[optional[0]] = { None: optional[1]() }
+
   dep_map = {None: set(names)}
   def create_maps(name):
     if name not in dep_map:
       module = getattr(recipe_modules, name)
 
       dep_map[name] = set(module.DEPS)
       map(create_maps, dep_map[name])
 
       mock = None if mocks is None else mocks.get(name, {})
-      inst_map[name] = module.API(module=module, mock=mock, **kwargs)
+      if required:
+        api = getattr(module, required[0])
+        inst_maps[required[0]][name] = api(module=module, mock=mock, **kwargs)
+      if optional:
+        api = getattr(module, optional[0], None) or optional[1]
+        inst_maps[optional[0]][name] = api(module=module)

[agable, 2013/09/16 20:29:25]

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".

+
   map(create_maps, names)
 
+  if required:
+    MapDependencies(dep_map, inst_maps[required[0]])
+  if optional:
+    MapDependencies(dep_map, inst_maps[optional[0]])
+    if required:
+      for name, module in inst_maps[required[0]].iteritems():
+        module.test_api = inst_maps[optional[0]][name]
+
+  return inst_maps[(required or optional)[0]][None]
+
+
+def MapDependencies(dep_map, inst_map):

[agable, 2013/09/16 20:29:25]

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.

   # NOTE: this is 'inefficient', but correct and compact.
-  did_something = True
+  dep_map = copy.deepcopy(dep_map)
   while dep_map:
     did_something = False
     to_pop = []
     for api_name, deps in dep_map.iteritems():
       to_remove = []
       for dep in [d for d in deps if d not in dep_map]:
         # Grab the injection site
         obj = inst_map[api_name].m
         assert not hasattr(obj, dep)
         setattr(obj, dep, inst_map[dep])
         to_remove.append(dep)
         did_something = True
       map(deps.remove, to_remove)
       if not deps:
         to_pop.append(api_name)
         did_something = True
     map(dep_map.pop, to_pop)
     assert did_something, 'Did nothing on this loop. %s' % dep_map
 
-  return inst_map[None]
+
+def CreateRecipeApi(mod_dirs, names, mocks=None, **kwargs):
+  return CreateApi(mod_dirs, names, mocks=mocks, kwargs=kwargs,
+                   required=('API', RecipeApi),
+                   optional=('TEST_API', RecipeTestApi))
+
+
+def CreateTestApi(mod_dirs, names):
+  return CreateApi(mod_dirs, names, optional=('TEST_API', RecipeTestApi))
 
 
 def wrap_followup(kwargs, pre=False):
   """
   Decorator for a new followup_fn.
 
   Will pop the existing fn out of kwargs (if any), and return a decorator for
   the new folloup_fn.
 
   Args:
     kwargs - dictionary possibly containing folloup_fn
     pre - If true, the old folloup_fn is called before the wrapped function.
           Otherwise, the old followup_fn is called after the wrapped function.
   """
   null_fn = lambda _: None
   old_followup = kwargs.pop('followup_fn', null_fn)
   def decorator(f):
     @functools.wraps(f)
     def _inner(step_result):
       if pre:
         old_followup(step_result)
         f(step_result)
       else:
         f(step_result)
         old_followup(step_result)
     if old_followup is not null_fn:
       _inner.__name__ += '[%s]' % old_followup.__name__
     return _inner
   return decorator