Add initial postprocess unit test thingy.

recipe_engine/simulation_test.py

Index: recipe_engine/simulation_test.py
diff --git a/recipe_engine/simulation_test.py b/recipe_engine/simulation_test.py
index 17c1d1e511689cdb38340f1af3a4585b640e46df..96c0d479e92b16e7fd80bb730fa6f301a44900d1 100644
--- a/recipe_engine/simulation_test.py
+++ b/recipe_engine/simulation_test.py
@@ -5,15 +5,22 @@
 """Provides simulator test coverage for individual recipes."""
 
 import StringIO
+import ast
 import contextlib
+import copy
 import json
 import logging
 import os
 import re
 import sys
+import textwrap
+import traceback
+
+from collections import OrderedDict
 
 from . import env
 from . import stream
+import astunparse
 import expect_tests
 
 # This variable must be set in the dynamic scope of the functions in this file.
@@ -22,37 +29,159 @@ import expect_tests
 _UNIVERSE = None
 
 
-def RenderExpectation(test_data, raw_expectations):
-  """Applies the step filters (e.g. whitelists, etc.) to the raw_expectations,
-  if the TestData actually contains any filters.
 
-  Returns the final expect_tests.Result."""
-  if test_data.whitelist_data:
-    whitelist_data  = dict(test_data.whitelist_data)  # copy so we can mutate it
-    def filter_expectation(step):
-      whitelist = whitelist_data.pop(step['name'], None)
-      if whitelist is None:
-        return
-
-      whitelist = set(whitelist)  # copy so we can mutate it
-      if len(whitelist) > 0:
-        whitelist.add('name')
-        step = {k: v for k, v in step.iteritems() if k in whitelist}
-        whitelist.difference_update(step.keys())
-        if whitelist:
-          raise ValueError(
-            "The whitelist includes fields %r in step %r, but those fields"
-            " don't exist."
-            % (whitelist, step['name']))
-      return step
-    raw_expectations = filter(filter_expectation, raw_expectations)
+linecache = {}
+def getLines(filename):

[dnj, 2016/10/04 16:42:35]

nit: Add underscores to these global functions and variables so it's clear
they're not intended to be used outside of this file.

[iannucci, 2016/10/06 20:28:09]

Done.

+  filename = os.path.abspath(filename)
+  if filename in linecache:
+    return linecache[filename]
+
+  with open(filename, 'r') as f:
+    ret = f.readlines()
+    linecache[filename] = ret
+    return ret
+
+
+def getReferencedNamesAndNode(filename, lineno):
+  lines = getLines(filename)
+  i = lineno-1
+  for i in range(lineno-1, 0, -1):

[dnj, 2016/10/04 16:42:35]

nit: xrange

[dnj, 2016/10/06 21:20:19]

(This and other comments in this file).

[iannucci, 2016/10/06 22:47:11]

oops

+    try:
+      to_parse = ''.join(lines[i:lineno]).strip()
+      node = ast.parse(to_parse)
+      break
+    except SyntaxError:
+      continue

[martiniss, 2016/10/03 19:14:19]

Why is this here? comment? :)

[iannucci, 2016/10/06 22:47:11]

Done.

+  names = set(n.id for n in ast.walk(node) if isinstance(n, ast.Name))
+  return (names, node, i)
+
+
+class Checker(object):
+  def __init__(self):
+    self._failed_checks = []
+    self._ignore_set = [self]

[martiniss, 2016/10/03 19:14:19]

Don't really understand ignore set

[iannucci, 2016/10/06 22:47:11]

Done.

+
+  def _ignore(self, obj):
+    self._ignore_set.extend(obj)
+
+  def _inner(self, hint, exp):

[martiniss, 2016/10/03 19:14:19]

Some docs on the magic in this function would be nice :)

[dnj, 2016/10/04 16:42:34]

Since "_inner" isn't recursive or anything, let's name it something more
intuitive. Suggest: "_call_impl" or "_resolve_and_append_check"?

[dnj, 2016/10/04 16:42:35]

+176 on docs here. This is probably the most magic code in recipe engine :)

[iannucci, 2016/10/06 22:47:11]

Done.

+    if exp:
+      # this check passed
+      return
+
+    stk = None

[dnj, 2016/10/04 16:42:35]

nit: this shouldn't be needed, since all paths cause it to be defined.

[iannucci, 2016/10/06 22:47:11]

Done.

+    try:
+      raise ZeroDivisionError
+    except ZeroDivisionError:
+      stk = sys.exc_info()[2].tb_frame.f_back.f_back
+
+    refNames, node, actual_lineno = getReferencedNamesAndNode(
+      stk.f_code.co_filename, stk.f_lineno)
+    local_vars = OrderedDict()
+    for k, v in stk.f_locals.iteritems():
+      if k not in refNames:
+        continue
+      if any(v is itm for itm in self._ignore_set):
+        continue
+      local_vars[k] = v
+    self._failed_checks.append(expect_tests.Check(
+      hint,
+      astunparse.unparse(node),
+      local_vars,
+      stk.f_code.co_filename,
+      actual_lineno,
+      False
+    ))
+
+  def __call__(self, arg1, arg2=None):
+    if arg2 is not None:
+      hint = arg1
+      exp = arg2
+    else:
+      hint = None
+      exp = arg1
+    self._inner(hint, exp)
+
+
+def checkIsSubset(a, b):

[martiniss, 2016/10/03 19:14:19]

tests?

[dnj, 2016/10/04 16:42:34]

docs?

[iannucci, 2016/10/06 22:47:11]

Done.

+  # both a and b should be ordered step lists.
+  if a is b:

[dnj, 2016/10/04 16:42:35]

Could use "==" here to catch duplicates that aren't the exact same object.

[iannucci, 2016/10/06 22:47:11]

I'm not certain how that would ever happen in practice? This is essentially
implemented in the loop below anyway.

+    return
+
+  for step_name, step in a.iteritems():
+    if step_name not in b:
+      raise ValueError(

[dnj, 2016/10/04 16:42:34]

Maybe define a PostProcessError type?

[iannucci, 2016/10/06 22:47:10]

Done.

+        'post_process introduced new step %r' % step_name)
 
-    if whitelist_data:
+    if 'name' not in step:
       raise ValueError(
-        "The step names %r were included in the whitelist, but were never run."
-        % [s['name'] for s in whitelist_data])
+        'post_process removed "name" from step %r' % step_name)

[dnj, 2016/10/04 16:42:35]

Should document that this is not allowed in "post_process".

[iannucci, 2016/10/06 22:47:10]

Done.

 
-  return expect_tests.Result(raw_expectations)
+    orig = b[step_name]
+    if step is orig:

[dnj, 2016/10/04 16:42:34]

== ?

+      continue
+
+    for k, v in step.iteritems():
+      if k not in orig:
+        raise ValueError(
+          'post_process introduced new key %r in step %r' % (
+            k, step_name))
+
+      orig_v = orig[k]
+      if v is orig_v:

[dnj, 2016/10/04 16:42:34]

== ?

[iannucci, 2016/10/06 22:47:11]

Again, not sure how this would happen in practice, and this is implemented
below. The purpose of using 'is' is to avoid looping over the whole data
structure as an optimization. Using == will end up looping over every data
member twice if they're not equal.

+        continue
+
+      if type(v) != type(orig_v):
+        raise ValueError(
+          'post_process changed type of key %r in step %r from %s to %s' % (
+            k, step_name, type(orig_v), type(v)))

[dnj, 2016/10/04 16:42:35]

For rendering type names, "type(x).__name__" is a more user-friendly.

[iannucci, 2016/10/06 22:47:11]

Done.

+
+      if isinstance(v, list):

[dnj, 2016/10/04 16:42:35]

Unless we specifically care about "list" and "dict", consider using
collections.Sequence and collections.Mapping respectively.

[iannucci, 2016/10/06 22:47:10]

We do, expectations will never have something other than primitive
json-encodable types.

+        if len(v) > len(orig_v):
+          raise ValueError(
+            'post_process %r in step %r is longer than the original: %d v %d' %
+            (k, step_name, len(v), len(orig_v)))
+        iov = iv = 0

[martiniss, 2016/10/03 19:14:19]

not immediately obvious what these variables are for.

[iannucci, 2016/10/06 22:47:11]

Done.

+        while iov < len(orig_v) - 1 and iv < len(v) - 1:
+          if v[iv] == orig_v[iov]:
+            iv += 1
+          iov += 1
+        if iv != len(v) - 1:
+          raise ValueError(
+            'post_process added elements to %r in step %r: %r' % (
+              k, step_name, v[iv:]))
+      elif isinstance(v, dict):
+        for subk, subv in v.iteritems():
+          if subk not in orig_v:

[dnj, 2016/10/04 16:42:35]

Can get by with only one lookup:

orig_item = orig_v.get(subk) # Use a sentinel object instead of None if None is
acceptable.
if subk != orig_item:
  if orig_item is None:
    raise "added element"
  else:
    raise "changed element"

[iannucci, 2016/10/06 22:47:10]

Done.

+            raise ValueError(
+              'post_process added element %s[%r] in step %r' % (
+                k, subk, step_name))
+          if subv != orig_v[subk]:
+            raise ValueError(
+              'post_process changed element %s[%r] in step %r: %r to %r' % (
+                k, subk, step_name, subv, orig_v[subk]))
+      elif v != orig_v:
+        raise ValueError(
+          'post_process changed value of key %r in step %r from %s to %s' % (
+            k, step_name, orig_v, v))
+
+
+def RenderExpectation(test_data, step_odict):
+  """Applies the step post_process actions to the step_odict, if the
+  TestData actually contains any.
+
+  Returns the final expect_tests.Result."""
+  checker = Checker()
+
+  for hook, args, kwargs in test_data.post_process_hooks:
+    input_odict = copy.deepcopy(step_odict)
+    checker._ignore(input_odict)

[martiniss, 2016/10/03 19:14:19]

why do we always ignore the input dict?

[iannucci, 2016/10/06 22:47:10]

Done.

+    rslt = hook(checker, input_odict, *args, **kwargs)
+    if rslt is not None:
+      checkIsSubset(rslt, step_odict)
+      step_odict = rslt
+
+  return expect_tests.Result(step_odict.values(), checker._failed_checks)
 
 
 class SimulationAnnotatorStreamEngine(stream.AnnotatorStreamEngine):
@@ -70,13 +199,24 @@ class SimulationAnnotatorStreamEngine(stream.AnnotatorStreamEngine):
                                     self.step_buffer(step_config.name))
 
 
-def RunRecipe(test_data):
+# This maps from (recipe_name,test_name) -> yielded test_data. It's outside of

[dnj, 2016/10/04 16:42:35]

Can you make this a namedtuple?

[iannucci, 2016/10/06 22:47:11]

I tried, it makes it more verbose for very little benefit. 2 element tuples used
exclusively as dictionary keys are perfectly fine imo.

+# RunRecipe so that it can persist between RunRecipe calls in the same process.
+_GEN_TEST_CACHE = {}
+
+def RunRecipe(recipe_name, test_name):
   """Actually runs the recipe given the GenTests-supplied test_data."""
   from . import config_types
   from . import loader
   from . import run
   from . import step_runner
-  from . import stream
+
+  if recipe_name not in _GEN_TEST_CACHE:
+    recipe_script = _UNIVERSE.load_recipe(recipe_name)
+    test_api = loader.create_test_api(recipe_script.LOADED_DEPS, _UNIVERSE)
+    for test_data in recipe_script.GenTests(test_api):
+      _GEN_TEST_CACHE[(recipe_name, test_data.name)] = test_data
+
+  test_data = _GEN_TEST_CACHE[(recipe_name, test_name)]
 
   config_types.ResetTostringFns()
 
@@ -88,15 +228,18 @@ def RunRecipe(test_data):
     step_runner = step_runner.SimulationStepRunner(stream_engine, test_data,
                                                    annotator)
 
-    engine = run.RecipeEngine(step_runner, test_data.properties, _UNIVERSE)
-    recipe_script = _UNIVERSE.load_recipe(test_data.properties['recipe'])
+    props = test_data.properties.copy()

[dnj, 2016/10/04 16:42:34]

deepcopy? I think properties can be nested.

[iannucci, 2016/10/06 22:47:11]

they can, but it doesn't matter.

+    props['recipe'] = recipe_name
+    engine = run.RecipeEngine(step_runner, props, _UNIVERSE)
+    recipe_script = _UNIVERSE.load_recipe(recipe_name)
     api = loader.create_recipe_api(recipe_script.LOADED_DEPS, engine, test_data)
     result = engine.run(recipe_script, api)
 
     # Don't include tracebacks in expectations because they are too sensitive to
     # change.
     result.result.pop('traceback', None)
-    raw_expectations = step_runner.steps_ran + [result.result]
+    raw_expectations = step_runner.steps_ran.copy()
+    raw_expectations[result.result['name']] = result.result
 
     try:
       return RenderExpectation(test_data, raw_expectations)
@@ -130,7 +273,8 @@ def cover_omit():
   return omit
 
 
-class InsufficientTestCoverage(Exception): pass
+class InsufficientTestCoverage(Exception):
+  pass
 
 
 @expect_tests.covers(test_gen_coverage)
@@ -149,27 +293,18 @@ def GenerateTests():
 
       covers = cover_mods + [recipe_path]
 
-      full_expectation_count = 0
       for test_data in recipe.GenTests(test_api):
-        if not test_data.whitelist_data:
-          full_expectation_count += 1
         root, name = os.path.split(recipe_path)
         name = os.path.splitext(name)[0]
         expect_path = os.path.join(root, '%s.expected' % name)
-
-        test_data.properties['recipe'] = recipe_name.replace('\\', '/')
         yield expect_tests.Test(
             '%s.%s' % (recipe_name, test_data.name),
-            expect_tests.FuncCall(RunRecipe, test_data),
+            expect_tests.FuncCall(RunRecipe, recipe_name, test_data.name),
             expect_dir=expect_path,
             expect_base=test_data.name,
             covers=covers,
             break_funcs=(recipe.RunSteps,)
         )
-
-      if full_expectation_count < 1:
-        raise InsufficientTestCoverage(
-          'Must have at least 1 test without a whitelist!')
     except:
       info = sys.exc_info()
       new_exec = Exception('While generating results for %r: %s: %s' % (