Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2387763003:
Add initial postprocess unit test thingy. (Closed)
|
Created:
4 years, 2 months ago by iannucci Modified:
4 years, 2 months ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Target Ref:
refs/heads/master Project:
recipe_engine Visibility:
Public. |
DescriptionAdd post_process hooks to recipe testing facilities.
These post_process hooks allow the user to run arbitrary assertions on their expectation outputs, and also allows arbitrary filtering of the expectation data (removing steps/fields, items from lists, etc.), including reduction of the expectations all the way down to an empty dictionary (which will remove the expectation file from disk).
This facility supersedes and replaces the recently-introduced whitelisting capabilities and will hopefully give recipe authors the testing flexibility they need to achieve confidence in the behavior of their recipes.
R=dnj@chromium.org, dpranke@chromium.org, machenbach@chromium.org, martiniss@chromium.org
BUG=459361
Committed: https://github.com/luci/recipes-py/commit/d97fe4474b8ebefb906e594aba840fff58720dae
Patch Set 1 #
Total comments: 101
Patch Set 2 : A bunch more smarts #Patch Set 3 : Fix nits and improve stuff. Tests next #Patch Set 4 : Move added line #Patch Set 5 : fix nit, recipe test #Patch Set 6 : more comments #
Total comments: 9
Patch Set 7 : Implement partial checker_test, rewrite VerifySubset #
Total comments: 16
Patch Set 8 : More tests #Patch Set 9 : Fix recipe example #Patch Set 10 : Add nested statement too #
Total comments: 22
Patch Set 11 : rewrite parser code #
Total comments: 14
Patch Set 12 : Fix bug and nits #Patch Set 13 : Fix nits #Patch Set 14 : Rebase #Messages
Total messages: 37 (7 generated)
This is mostly groundwork/proof of concept (with some light tests for flavor).
So what's here:
* api.whitelist is gone, and is replaced by api.post_process.
* api.post_process takes a callback, as well as any user-supplied args/kwargs
that the callback may want
* the callback is invoked with a `check` function, the current OrderedDict of
steps (what would have been the JSON expectation), and the args/kwargs, if any.
* the function is able to invoke check on any condition it wants to check any
constraints it needs to.
* the function may also return any subset of the OrderedDict it wants to. This
means deleting steps, deleting keys from steps, removing items from lists (like
'cmd'), or items from dicts (like 'env'). Order must be otherwise preserved for
any ordered items. No addition of data is permitted.
* If the function returns [], the expectation file will be omitted entirely.
If it returns None, the expectations will be considered to be unmodified.
* If multiple post_process functions are specified, each will consume the
output of the previous one, in order.
* There are a couple
(not-currently-documented-but-they-will-be-before-this-CL-lands) pre-defined
post processing functions in post_process.py (including a filter that retains
steps you want, as well as 'must run' and 'must not run' asserter functions).
The check function is a little bit magic (but I think it's probably good magic.
Or at least not completely evil). It does a little trick that's inspired by
django's awesome exception handler code. It walks the stack up till its
invocation, parses the statement of its invocation, identifies all local
variables referenced in the invocation, and then includes the values of those in
the output in the event of a failed check. Additionally, since the checks are
all against (nominally) static data (in the form of the expectations), this
implementation captures all of the failed checks simultaneously (so no
fix/test/fix/test cycle; you can see all the failures at once!). I had to modify
expect_tests a bit to have the concept of checks, but it all actually works
pretty well in practice, I think, and produces output like:
"""
➜ recipes-py git:(postprocess) ✗ ./recipes.py --use-bootstrap --package
./infra/config/recipes.cfg simulation_test
Activating environment: 'ENV'
Activating environment
Done creating environment
...........C...........................................................
======================================================================
FAIL CHECK: engine_tests/whitelist_steps.all_steps
(/s/infra/infra/recipes-py/recipes/engine_tests/whitelist_steps.expected/all_steps.json)
----------------------------------------------------------------------
/s/infra/infra/recipes-py/recipe_engine/post_process.py:79 -
`check((step_name in step_names))`:
step_name: 'faekestep'
step_names: ['something unimportant', 'something important', 'another
important', 'fakestep', '$result']
----------------------------------------------------------------------
Ran 72 tests in 0.363s
FAILED (failed checks=1)
"""
The intent of this is to provide as much debugging information as is useful in
the event that an assertion fails. My guess is that you can tell just from the
error report above what went wrong (though I think I should also add a clue that
describes exactly which post_process function this was that produced the error
too).
Anyway, that's about it for now. Let me know what you think.
looking pretty good https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.py File recipe_engine/post_process.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:3: # that can be found in the LICENSE file. Add a file doc string and tests? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:83: def MustRunRE(check, step_odict, step_regex, at_least=1, at_most=None): Maybe have examples of these in other engine tests? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... File recipe_engine/recipe_test_api.py (left): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:553: def whitelist(self, step_name, *fields): Could you keep the whitelist function, and have it do the same thing it does now? So that downstream code doesn't break when you roll this. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... File recipe_engine/recipe_test_api.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:576: be written as the ___ in the sentance 'check that ___.'. Essentially, typo https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:54: continue Why is this here? comment? :) https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:62: self._ignore_set = [self] Don't really understand ignore set https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:67: def _inner(self, hint, exp): Some docs on the magic in this function would be nice :) https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:106: def checkIsSubset(a, b): tests? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:144: iov = iv = 0 not immediately obvious what these variables are for. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:178: checker._ignore(input_odict) why do we always ignore the input dict? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/a... File recipe_engine/third_party/astunparse/README.google (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/a... recipe_engine/third_party/astunparse/README.google:6: Local modifications: Any reason you didn't use https://github.com/luci/recipes-py/blob/master/bootstrap/update_vendoring.py ? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... File recipe_engine/third_party/expect_tests/handle_test.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/handle_test.py:9: import textwrap unused https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/handle_test.py:102: self._emit('C', fc.test, 'womba') womba?? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... File recipe_engine/third_party/expect_tests/type_definitions.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/type_definitions.py:28: def format(self, indent): Maybe put an example of what this will look like when printed? A bit hard to parse the format statement in my head https://codereview.chromium.org/2387763003/diff/1/recipes/engine_tests/whitel... File recipes/engine_tests/whitelist_steps.py (right): https://codereview.chromium.org/2387763003/diff/1/recipes/engine_tests/whitel... recipes/engine_tests/whitelist_steps.py:27: def GenTests(api): No NewFilter with regex.
I may not get to this in a timely manner, so don't block on me if you don't need to.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.py File recipe_engine/post_process.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:24: if not field_set: Can we have an error if both specific and regex match the same step? I think this is an error case. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:24: if not field_set: nit: You're using "is None" below, but "not" here. I prefer "is None". https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:45: check(re_usage_count[regex] < at_most) If I say, "at most 3", I would expect 3 to be valid? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:56: new_re_data[re.compile(step_name_re)] = (at_least, at_most, set(fields)) Let's use a namedtuple here. Simple to create one inside the _filterObject class. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:60: def NewFilter(*steps): docstrings! https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:64: def DoesntRun(check, step_odict, *steps): This looks awkward. How about DoesNotRun? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:70: def DoesntRunRE(check, step_odict, *step_regexes): (same) https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:71: step_regexes = [re.compile(r) for r in step_regexes] Assert that "step_odict" is an OrderedDict? Here and at other public function boundaries. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:84: step_regex = re.compile(step_regex) WDYT about allowing "step_regex" to actually be a compiled regex? And above. _RE_TYPE = typeof(re.compile(r'')) def _get_re(value): if isinstance(value, _RE_TYPE): return value elif isinstance(value, basestring): return re.compile(value) else: raise TypeError("Don't know how to make a regex from %s" % (type(value).__name__,) https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... File recipe_engine/recipe_test_api.py (left): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:553: def whitelist(self, step_name, *fields): On 2016/10/03 19:14:19, martiniss wrote: > Could you keep the whitelist function, and have it do the same thing it does > now? So that downstream code doesn't break when you roll this. I think I'd prefer killing the whitelist function, since it's in its infancy. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... File recipe_engine/recipe_test_api.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:185: PostprocessHook = namedtuple('PostprocessHook', ['func', 'args', 'kwargs']) nit: Use a tuple here, no need for a full list. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:282: self.post_process_hooks.append(PostprocessHook(func, args, kwargs)) Is this worth uniqueifying? Probably not... https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:571: * check is a semi-magical function which you can use to test things. Using nit: check in backticks. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:574: with parens) to produce helpful check messages. Check also has a second nit: "check" https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:576: be written as the ___ in the sentance 'check that ___.'. Essentially, nit: single space before "Essentially". https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:595: Raising an exception will print the exception, but will halt the nit: This is awkwardly worded. Maybe, "... and will halt the ..."? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:601: will remove the expectations from disk altogether. Returning `None` nit: "dict" https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... recipe_engine/recipe_test_api.py:602: (python's implicit default return value) is equivalent to returning the nit: "Python's". https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:34: def getLines(filename): nit: Add underscores to these global functions and variables so it's clear they're not intended to be used outside of this file. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:48: for i in range(lineno-1, 0, -1): nit: xrange https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:67: def _inner(self, hint, exp): On 2016/10/03 19:14:19, martiniss wrote: > Some docs on the magic in this function would be nice :) +176 on docs here. This is probably the most magic code in recipe engine :) https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:67: def _inner(self, hint, exp): Since "_inner" isn't recursive or anything, let's name it something more intuitive. Suggest: "_call_impl" or "_resolve_and_append_check"? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:72: stk = None nit: this shouldn't be needed, since all paths cause it to be defined. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:106: def checkIsSubset(a, b): docs? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:108: if a is b: Could use "==" here to catch duplicates that aren't the exact same object. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:113: raise ValueError( Maybe define a PostProcessError type? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:118: 'post_process removed "name" from step %r' % step_name) Should document that this is not allowed in "post_process". https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:121: if step is orig: == ? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:131: if v is orig_v: == ? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:137: k, step_name, type(orig_v), type(v))) For rendering type names, "type(x).__name__" is a more user-friendly. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:139: if isinstance(v, list): Unless we specifically care about "list" and "dict", consider using collections.Sequence and collections.Mapping respectively. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:155: if subk not in orig_v: 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" https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:202: # This maps from (recipe_name,test_name) -> yielded test_data. It's outside of Can you make this a namedtuple? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:231: props = test_data.properties.copy() deepcopy? I think properties can be nested. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/step_runner.py File recipe_engine/step_runner.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/step_runner.p... recipe_engine/step_runner.py:475: (name, self._rendered_step_to_dict(rs)) Can we make this a namedtuple? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... File recipe_engine/third_party/expect_tests/handle_test.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/handle_test.py:22: class FailChecks(collections.namedtuple('FailChecks', 'test checks')): nit: use a tuple of single strings instead of a space-delimited string. This is my personal preference, so if you really like space-delimited strings better go for it. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... File recipe_engine/third_party/expect_tests/type_definitions.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/type_definitions.py:19: Result = namedtuple('Result', 'data checks') nit: same about namedtuple spaces etc. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/type_definitions.py:27: 'Check', 'name code context filename lineno passed')): nit: (and here WRT namedtuple spaces)
Although I appreciate the reminders to add docs, you may not have seen my initial comment that I intend to add docs and tests before landing :) Thanks for the review, I'll finish this today. R On Tue, Oct 4, 2016, 09:42 <dnj@chromium.org> wrote: > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.py > File recipe_engine/post_process.py (right): > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:24: if not field_set: > Can we have an error if both specific and regex match the same step? I > think this is an error case. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:24: if not field_set: > nit: You're using "is None" below, but "not" here. I prefer "is None". > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:45: check(re_usage_count[regex] < at_most) > If I say, "at most 3", I would expect 3 to be valid? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:56: new_re_data[re.compile(step_name_re)] > = (at_least, at_most, set(fields)) > Let's use a namedtuple here. Simple to create one inside the > _filterObject class. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:60: def NewFilter(*steps): > docstrings! > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:64: def DoesntRun(check, step_odict, > *steps): > This looks awkward. How about DoesNotRun? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:70: def DoesntRunRE(check, step_odict, > *step_regexes): > (same) > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:71: step_regexes = [re.compile(r) for r in > step_regexes] > Assert that "step_odict" is an OrderedDict? Here and at other public > function boundaries. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.... > recipe_engine/post_process.py:84: step_regex = re.compile(step_regex) > WDYT about allowing "step_regex" to actually be a compiled regex? And > above. > > _RE_TYPE = typeof(re.compile(r'')) > > def _get_re(value): > if isinstance(value, _RE_TYPE): > return value > elif isinstance(value, basestring): > return re.compile(value) > else: > raise TypeError("Don't know how to make a regex from %s" % > (type(value).__name__,) > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > File recipe_engine/recipe_test_api.py (left): > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:553: def whitelist(self, step_name, > *fields): > On 2016/10/03 19:14:19, martiniss wrote: > > Could you keep the whitelist function, and have it do the same thing > it does > > now? So that downstream code doesn't break when you roll this. > > I think I'd prefer killing the whitelist function, since it's in its > infancy. > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > File recipe_engine/recipe_test_api.py (right): > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:185: PostprocessHook = > namedtuple('PostprocessHook', ['func', 'args', 'kwargs']) > nit: Use a tuple here, no need for a full list. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:282: > self.post_process_hooks.append(PostprocessHook(func, args, kwargs)) > Is this worth uniqueifying? Probably not... > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:571: * check is a semi-magical function > which you can use to test things. Using > nit: check in backticks. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:574: with parens) to produce helpful > check messages. Check also has a second > nit: "check" > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:576: be written as the ___ in the > sentance 'check that ___.'. Essentially, > nit: single space before "Essentially". > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:595: Raising an exception will print > the exception, but will halt the > nit: This is awkwardly worded. Maybe, "... and will halt the ..."? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:601: will remove the expectations from > disk altogether. Returning `None` > nit: "dict" > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a... > recipe_engine/recipe_test_api.py:602: (python's implicit default return > value) is equivalent to returning the > nit: "Python's". > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > File recipe_engine/simulation_test.py (right): > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:34: def getLines(filename): > nit: Add underscores to these global functions and variables so it's > clear they're not intended to be used outside of this file. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:48: for i in range(lineno-1, 0, -1): > nit: xrange > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:67: def _inner(self, hint, exp): > On 2016/10/03 19:14:19, martiniss wrote: > > Some docs on the magic in this function would be nice :) > > +176 on docs here. This is probably the most magic code in recipe engine > :) > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:67: def _inner(self, hint, exp): > Since "_inner" isn't recursive or anything, let's name it something more > intuitive. Suggest: "_call_impl" or "_resolve_and_append_check"? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:72: stk = None > nit: this shouldn't be needed, since all paths cause it to be defined. > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:106: def checkIsSubset(a, b): > docs? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:108: if a is b: > Could use "==" here to catch duplicates that aren't the exact same > object. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:113: raise ValueError( > Maybe define a PostProcessError type? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:118: 'post_process removed "name" from > step %r' % step_name) > Should document that this is not allowed in "post_process". > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:121: if step is orig: > == ? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:131: if v is orig_v: > == ? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:137: k, step_name, type(orig_v), > type(v))) > For rendering type names, "type(x).__name__" is a more user-friendly. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:139: if isinstance(v, list): > Unless we specifically care about "list" and "dict", consider using > collections.Sequence and collections.Mapping respectively. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:155: if subk not in orig_v: > 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" > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:202: # This maps from > (recipe_name,test_name) -> yielded test_data. It's outside of > Can you make this a namedtuple? > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... > recipe_engine/simulation_test.py:231: props = > test_data.properties.copy() > deepcopy? I think properties can be nested. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/step_runner.py > File recipe_engine/step_runner.py (right): > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/step_runner.p... > recipe_engine/step_runner.py:475: (name, > self._rendered_step_to_dict(rs)) > Can we make this a namedtuple? > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... > File recipe_engine/third_party/expect_tests/handle_test.py (right): > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... > recipe_engine/third_party/expect_tests/handle_test.py:22: class > FailChecks(collections.namedtuple('FailChecks', 'test checks')): > nit: use a tuple of single strings instead of a space-delimited string. > This is my personal preference, so if you really like space-delimited > strings better go for it. > > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... > File recipe_engine/third_party/expect_tests/type_definitions.py (right): > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... > recipe_engine/third_party/expect_tests/type_definitions.py:19: Result = > namedtuple('Result', 'data checks') > nit: same about namedtuple spaces etc. > > > https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... > recipe_engine/third_party/expect_tests/type_definitions.py:27: 'Check', > 'name code context filename lineno passed')): > nit: (and here WRT namedtuple spaces) > > https://codereview.chromium.org/2387763003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
tests next. A failed check now looks like:
'engine_tests/whitelist_steps.all_steps' failed checks!
CHECK(FAIL) - "something was run":
added /s/infra/infra/recipes-py/recipes/engine_tests/whitelist_steps.py:28
MustRun('fakiestep')
/s/infra/infra/recipes-py/recipe_engine/post_process.py:160 - MustRun()
`check("something was run", (step_name in step_odict))`
step_odict.keys(): ['something unimportant', 'something important',
'another important', 'fakestep', '$result']
step_name: 'fakiestep'
What this has:
* The check that failed, along with the (optional) custom message
* The file:line where the post_process hook was added, as well as the name of
the function given and the arguments supplied.
* The stack trace of the actual function execution
* The statement at the location of the failed check
* The relevant local variable data contained in the statement
* ___ in <dict> will print <dict>.keys()
* x[a][b]... will print `x[a]`, `x[a][b]`, etc., but not x (to avoid
printing giant dictionaries, like the actual step expectations)
* compiled regexes will be printed as `re.compile("pattern", FLAG|FLAG)`,
rather than `<_sre.SRE_Pattern object at 0x10d30b4f8>`
* all other non-callable variables will be looked up and printed.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process.py
File recipe_engine/post_process.py (right):
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:24: if not field_set:
On 2016/10/04 16:42:34, dnj wrote:
> nit: You're using "is None" below, but "not" here. I prefer "is None".
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:24: if not field_set:
On 2016/10/04 16:42:33, dnj wrote:
> Can we have an error if both specific and regex match the same step? I think
> this is an error case.
I don't think that's an error case, I've documented the behavior.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:45: check(re_usage_count[regex] < at_most)
On 2016/10/04 16:42:34, dnj wrote:
> If I say, "at most 3", I would expect 3 to be valid?
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:56: new_re_data[re.compile(step_name_re)] =
(at_least, at_most, set(fields))
On 2016/10/04 16:42:34, dnj wrote:
> Let's use a namedtuple here. Simple to create one inside the _filterObject
> class.
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:60: def NewFilter(*steps):
On 2016/10/04 16:42:34, dnj wrote:
> docstrings!
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:64: def DoesntRun(check, step_odict, *steps):
On 2016/10/04 16:42:34, dnj wrote:
> This looks awkward. How about DoesNotRun?
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:70: def DoesntRunRE(check, step_odict,
*step_regexes):
On 2016/10/04 16:42:34, dnj wrote:
> (same)
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:71: step_regexes = [re.compile(r) for r in
step_regexes]
On 2016/10/04 16:42:34, dnj wrote:
> Assert that "step_odict" is an OrderedDict? Here and at other public function
> boundaries.
That would be a terrible abstraction violation :D. post_process is guaranteed to
call this with an OrderedDict. Otherwise it's broken. If I put checks here, it
would mean that users also have to put the same checks in their own assertion
functions.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:83: def MustRunRE(check, step_odict, step_regex,
at_least=1, at_most=None):
On 2016/10/03 19:14:18, martiniss wrote:
> Maybe have examples of these in other engine tests?
there are docs now, I don't think an example of every single function is
necessary.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/post_process....
recipe_engine/post_process.py:84: step_regex = re.compile(step_regex)
On 2016/10/04 16:42:34, dnj wrote:
> WDYT about allowing "step_regex" to actually be a compiled regex? And above.
>
> _RE_TYPE = typeof(re.compile(r''))
>
> def _get_re(value):
> if isinstance(value, _RE_TYPE):
> return value
> elif isinstance(value, basestring):
> return re.compile(value)
> else:
> raise TypeError("Don't know how to make a regex from %s" %
> (type(value).__name__,)
It's already the case.
f = re.compile('foo')
F = re.compile(f)
f == F
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
File recipe_engine/recipe_test_api.py (left):
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:553: def whitelist(self, step_name, *fields):
On 2016/10/04 16:42:34, dnj wrote:
> On 2016/10/03 19:14:19, martiniss wrote:
> > Could you keep the whitelist function, and have it do the same thing it does
> > now? So that downstream code doesn't break when you roll this.
>
> I think I'd prefer killing the whitelist function, since it's in its infancy.
I'm killing it. I'll watch the roll.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
File recipe_engine/recipe_test_api.py (right):
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:185: PostprocessHook =
namedtuple('PostprocessHook', ['func', 'args', 'kwargs'])
On 2016/10/04 16:42:34, dnj wrote:
> nit: Use a tuple here, no need for a full list.
used string
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:282:
self.post_process_hooks.append(PostprocessHook(func, args, kwargs))
On 2016/10/04 16:42:34, dnj wrote:
> Is this worth uniqueifying? Probably not...
yeah I don't think so
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:571: * check is a semi-magical function which
you can use to test things. Using
On 2016/10/04 16:42:34, dnj wrote:
> nit: check in backticks.
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:574: with parens) to produce helpful check
messages. Check also has a second
On 2016/10/04 16:42:34, dnj wrote:
> nit: "check"
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:576: be written as the ___ in the sentance
'check that ___.'. Essentially,
On 2016/10/04 16:42:34, dnj wrote:
> nit: single space before "Essentially".
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:576: be written as the ___ in the sentance
'check that ___.'. Essentially,
On 2016/10/03 19:14:19, martiniss wrote:
> typo
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:595: Raising an exception will print the
exception, but will halt the
On 2016/10/04 16:42:34, dnj wrote:
> nit: This is awkwardly worded. Maybe, "... and will halt the ..."?
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:601: will remove the expectations from disk
altogether. Returning `None`
On 2016/10/04 16:42:34, dnj wrote:
> nit: "dict"
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/recipe_test_a...
recipe_engine/recipe_test_api.py:602: (python's implicit default return value)
is equivalent to returning the
On 2016/10/04 16:42:34, dnj wrote:
> nit: "Python's".
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te...
File recipe_engine/simulation_test.py (right):
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te...
recipe_engine/simulation_test.py:34: def getLines(filename):
On 2016/10/04 16:42:35, dnj wrote:
> nit: Add underscores to these global functions and variables so it's clear
> they're not intended to be used outside of this file.
Done.
https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:48: for i in range(lineno-1, 0, -1): On 2016/10/04 16:42:35, dnj wrote: > nit: xrange (This and other comments in this file).
On 2016/10/06 at 20:28:10, iannucci wrote:
> tests next. A failed check now looks like:
>
> 'engine_tests/whitelist_steps.all_steps' failed checks!
> CHECK(FAIL) - "something was run":
> added
/s/infra/infra/recipes-py/recipes/engine_tests/whitelist_steps.py:28
> MustRun('fakiestep')
> /s/infra/infra/recipes-py/recipe_engine/post_process.py:160 - MustRun()
> `check("something was run", (step_name in step_odict))`
> step_odict.keys(): ['something unimportant', 'something important',
'another important', 'fakestep', '$result']
> step_name: 'fakiestep'
This is a lot of output.
I would personally move the added line below the must run line. IMO people want
to see what assertion failed first, rather than where their test was added.
>
> What this has:
> * The check that failed, along with the (optional) custom message
> * The file:line where the post_process hook was added, as well as the name
of the function given and the arguments supplied.
> * The stack trace of the actual function execution
> * The statement at the location of the failed check
> * The relevant local variable data contained in the statement
> * ___ in <dict> will print <dict>.keys()
> * x[a][b]... will print `x[a]`, `x[a][b]`, etc., but not x (to avoid
printing giant dictionaries, like the actual step expectations)
Maybe just print the size of x, or something?
> * compiled regexes will be printed as `re.compile("pattern", FLAG|FLAG)`,
rather than `<_sre.SRE_Pattern object at 0x10d30b4f8>`
> * all other non-callable variables will be looked up and printed.
>
ok, got the rest of the comments, going to get lunch then do tests. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:48: for i in range(lineno-1, 0, -1): On 2016/10/06 21:20:19, dnj wrote: > On 2016/10/04 16:42:35, dnj wrote: > > nit: xrange > > (This and other comments in this file). oops https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:54: continue On 2016/10/03 19:14:19, martiniss wrote: > Why is this here? comment? :) Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:62: self._ignore_set = [self] On 2016/10/03 19:14:19, martiniss wrote: > Don't really understand ignore set Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:67: def _inner(self, hint, exp): On 2016/10/04 16:42:34, dnj wrote: > Since "_inner" isn't recursive or anything, let's name it something more > intuitive. Suggest: "_call_impl" or "_resolve_and_append_check"? Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:72: stk = None On 2016/10/04 16:42:35, dnj wrote: > nit: this shouldn't be needed, since all paths cause it to be defined. Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:106: def checkIsSubset(a, b): On 2016/10/04 16:42:34, dnj wrote: > docs? Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:108: if a is b: On 2016/10/04 16:42:35, dnj wrote: > Could use "==" here to catch duplicates that aren't the exact same object. I'm not certain how that would ever happen in practice? This is essentially implemented in the loop below anyway. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:113: raise ValueError( On 2016/10/04 16:42:34, dnj wrote: > Maybe define a PostProcessError type? Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:118: 'post_process removed "name" from step %r' % step_name) On 2016/10/04 16:42:35, dnj wrote: > Should document that this is not allowed in "post_process". Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:131: if v is orig_v: On 2016/10/04 16:42:34, dnj wrote: > == ? 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. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:137: k, step_name, type(orig_v), type(v))) On 2016/10/04 16:42:35, dnj wrote: > For rendering type names, "type(x).__name__" is a more user-friendly. Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:139: if isinstance(v, list): On 2016/10/04 16:42:35, dnj wrote: > Unless we specifically care about "list" and "dict", consider using > collections.Sequence and collections.Mapping respectively. We do, expectations will never have something other than primitive json-encodable types. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:144: iov = iv = 0 On 2016/10/03 19:14:19, martiniss wrote: > not immediately obvious what these variables are for. Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:155: if subk not in orig_v: On 2016/10/04 16:42:35, dnj wrote: > 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" Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:178: checker._ignore(input_odict) On 2016/10/03 19:14:19, martiniss wrote: > why do we always ignore the input dict? Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:202: # This maps from (recipe_name,test_name) -> yielded test_data. It's outside of On 2016/10/04 16:42:35, dnj wrote: > Can you make this a namedtuple? I tried, it makes it more verbose for very little benefit. 2 element tuples used exclusively as dictionary keys are perfectly fine imo. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test.py:231: props = test_data.properties.copy() On 2016/10/04 16:42:34, dnj wrote: > deepcopy? I think properties can be nested. they can, but it doesn't matter. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/step_runner.py File recipe_engine/step_runner.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/step_runner.p... recipe_engine/step_runner.py:475: (name, self._rendered_step_to_dict(rs)) On 2016/10/04 16:42:35, dnj wrote: > Can we make this a namedtuple? are you serious? https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/a... File recipe_engine/third_party/astunparse/README.google (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/a... recipe_engine/third_party/astunparse/README.google:6: Local modifications: On 2016/10/03 19:14:19, martiniss wrote: > Any reason you didn't use > https://github.com/luci/recipes-py/blob/master/bootstrap/update_vendoring.py ? b/c I'm a dummy? fixed. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... File recipe_engine/third_party/expect_tests/handle_test.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/handle_test.py:9: import textwrap On 2016/10/03 19:14:19, martiniss wrote: > unused Done. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/handle_test.py:22: class FailChecks(collections.namedtuple('FailChecks', 'test checks')): On 2016/10/04 16:42:35, dnj wrote: > nit: use a tuple of single strings instead of a space-delimited string. This is > my personal preference, so if you really like space-delimited strings better go > for it. I usually do the space delimited string because namedtuple's implementation is horrifying and using anything other than space delimited strings is just fooling yourself. Also it's shorter. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/handle_test.py:102: self._emit('C', fc.test, 'womba') On 2016/10/03 19:14:19, martiniss wrote: > womba?? er, oops. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... File recipe_engine/third_party/expect_tests/type_definitions.py (right): https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/type_definitions.py:19: Result = namedtuple('Result', 'data checks') On 2016/10/04 16:42:35, dnj wrote: > nit: same about namedtuple spaces etc. Acknowledged. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/type_definitions.py:27: 'Check', 'name code context filename lineno passed')): On 2016/10/04 16:42:35, dnj wrote: > nit: (and here WRT namedtuple spaces) Acknowledged. https://codereview.chromium.org/2387763003/diff/1/recipe_engine/third_party/e... recipe_engine/third_party/expect_tests/type_definitions.py:28: def format(self, indent): On 2016/10/03 19:14:19, martiniss wrote: > Maybe put an example of what this will look like when printed? A bit hard to > parse the format statement in my head added https://codereview.chromium.org/2387763003/diff/1/recipes/engine_tests/whitel... File recipes/engine_tests/whitelist_steps.py (right): https://codereview.chromium.org/2387763003/diff/1/recipes/engine_tests/whitel... recipes/engine_tests/whitelist_steps.py:27: def GenTests(api): On 2016/10/03 19:14:19, martiniss wrote: > No NewFilter with regex. yeah I'm going to add real tests for all that.
Thanks a lot for working on this! https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/recipe_t... File recipe_engine/recipe_test_api.py (right): https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/recipe_t... recipe_engine/recipe_test_api.py:601: The function must return either `None`, or it may return a filtered subset Is the original step dict mutable and is it allowed/disallowed to mutate it? https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/recipe_t... recipe_engine/recipe_test_api.py:606: the unmodified step_odict. Your function is never permitted to remove the Will returning an empty dict cause the expectation file to go away entirely? I.e. no file to check in? https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/recipe_t... recipe_engine/recipe_test_api.py:623: + api.post_process(Filter('thing_step')) does Filter work? Maybe add this example verbatim to the recipe engine tests to make sure it has no typos. https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/recipe_t... recipe_engine/recipe_test_api.py:636: yield (api.test('only care one step and the result') nit: care about https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/simulati... File recipe_engine/simulation_test.py (right): https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/simulati... recipe_engine/simulation_test.py:34: subexpressions from a python expression (specificially, from an invocation of nit: s/specificially/specifically
https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/recipe_t... File recipe_engine/recipe_test_api.py (right): https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/recipe_t... recipe_engine/recipe_test_api.py:606: the unmodified step_odict. Your function is never permitted to remove the On 2016/10/07 16:32:04, machenbach (OOO) wrote: > Will returning an empty dict cause the expectation file to go away entirely? > I.e. no file to check in? Yes, looking into the code answered this. And there is a canned method already.
https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... File recipe_engine/post_process.py (right): https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... recipe_engine/post_process.py:95: new_data = self.data.copy() Why not mutate self.data as well (by replacing it with a modified copy)? And then return self? Users might forget to reassign and use: f = NewFilter() f.include(...) Which functional use case do you cover by not permitting permutation here?
https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... File recipe_engine/post_process.py (right): https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... recipe_engine/post_process.py:95: new_data = self.data.copy() On 2016/10/07 16:51:30, machenbach (slow) wrote: > Why not mutate self.data as well (by replacing it with a modified copy)? And > then return self? > > Users might forget to reassign and use: > f = NewFilter() > f.include(...) > > Which functional use case do you cover by not permitting permutation here? More self thinking: Agreed that if we mutate we couldn't use it in the following way: f = NewFilter(...lots of cool initialization) Test + api.post_process(f.include(...more stuff)) Test + api.post_process(f.include(...other stuff)) etc. If users use it that way and f is mutated, the effects might be worse than the case I sketched above. So, maybe just ignore this comment.
https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... File recipe_engine/post_process.py (right): https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... recipe_engine/post_process.py:147: check(not r.match(step_name)) nit: Should these canned checks not get some more human readable hint? Or will the hint be human readable enough?
On 2016/10/07 16:51:30, machenbach (slow) wrote: > https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... > File recipe_engine/post_process.py (right): > > https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... > recipe_engine/post_process.py:95: new_data = self.data.copy() > Why not mutate self.data as well (by replacing it with a modified copy)? And > then return self? > > Users might forget to reassign and use: > f = NewFilter() > f.include(...) > > Which functional use case do you cover by not permitting permutation here? because of the way that these things are yielded, mutating it and passing the mutated thing to multiple post_process calls will result in all of them using the very-last-mutated version, which would be confusing and wrong.
On 2016/10/07 16:56:42, machenbach (slow) wrote: > https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... > File recipe_engine/post_process.py (right): > > https://codereview.chromium.org/2387763003/diff/100001/recipe_engine/post_pro... > recipe_engine/post_process.py:147: check(not r.match(step_name)) > nit: Should these canned checks not get some more human readable hint? Or will > the hint be human readable enough? This one would print out: * the regular expression pattern contained in r * the value of step_name Which I think should be enough to figure out what's up (and why `check` is semi-magical). It would also be possible to add text to the check, if necessary, but I don't think it will be.
Awesome, lgtm from a consumer p-o-v. I didn't check the implementation in much detail, please get another lgtm for that. I'm looking forward to try it out!
On 2016/10/08 at 09:02:01, machenbach wrote: > Awesome, lgtm from a consumer p-o-v. I didn't check the implementation in much detail, please get another lgtm for that. I'm looking forward to try it out! I'll review.
On 2016/10/10 17:05:59, martiniss wrote: > On 2016/10/08 at 09:02:01, machenbach wrote: > > Awesome, lgtm from a consumer p-o-v. I didn't check the implementation in much > detail, please get another lgtm for that. I'm looking forward to try it out! > > I'll review. still missing some tests, but I don't expect the implementation to change (barring bugs found via testing)
Are you done with this CL? More tests coming, right? Looks generally good :) https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.py File recipe_engine/checker.py (right): https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:55: # match `___ in instanceof(dict)` Could you turn this into a doc string? I didn't realize it was describing what the function does. https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:71: # match __[a] Turn into docstring :) https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:80: # match foo Turn into docstring :) https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:151: i = lineno-1 redundant? https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:156: for i in xrange(lineno-1, 0, -1): Couldn't you run into a situation like this: def foo(): check(bar() + 2 == 4) def bar(): return 3 ? This wouldn't be caught by the current logic, since it would never be able to find bar. Might not want to care about this case though.. https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:222: # order it so that innermost frame is at the bottom bottom? it's a list right? https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:252: def VerifySubset(a, b): Is a supposed to be a subset of b? docstring? https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:260: elif len(a) == 1: what happens if len(a) > 1? https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/post_pro... File recipe_engine/post_process.py (right): https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/post_pro... recipe_engine/post_process.py:53: class _filterImpl(Filter): why do we have this class hierarchy? https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/post_pro... recipe_engine/post_process.py:123: """Asserts that the given steps to not run. "to not" -> "don't" https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/recipe_t... File recipe_engine/recipe_test_api.py (right): https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/recipe_t... recipe_engine/recipe_test_api.py:633: + api.post_process(Blacklist('bob_step')) Blacklist no longer exists https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/simulati... File recipe_engine/simulation_test.py (right): https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/simulati... recipe_engine/simulation_test.py:52: failed_checks += checker._failed_checks nit: make a property to access _failed_checks. https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/simulati... recipe_engine/simulation_test.py:56: raise PostProcessError('post_process: steps'+msg) nit: spacing around msg. https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/simulati... recipe_engine/simulation_test.py:85: _GEN_TEST_CACHE = {} Any particular reason you added this?
Ok, PTAL for real. I've actually addressed all comments, added docs and tests, and ended up reworking a bunch of the implementations. Should be good to review for real now. https://chromiumcodereview.appspot.com/2387763003/diff/120001/recipe_engine/s... File recipe_engine/simulation_test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/120001/recipe_engine/s... recipe_engine/simulation_test.py:85: _GEN_TEST_CACHE = {} On 2016/10/10 18:53:12, martiniss wrote: > Any particular reason you added this? You mean besides the comment right above it? :D
Still lgtm from user perspective. Only skimmed through the api and tests and left a few nits. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/c... File recipe_engine/checker.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/c... recipe_engine/checker.py:1: # Copyright 2014 The LUCI Authors. All rights reserved. nit: current year for new files? https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/c... recipe_engine/checker.py:5: """Provides simulator test coverage for individual recipes.""" I'm not fully understanding this description. I understand what it says, but maybe you could state in more detail what's implemented here? https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/p... File recipe_engine/post_process.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/p... recipe_engine/post_process.py:78: fields (str) - The field(s) to include. Omit to include all fields. How do I prune all fields? If I, for example, only want to show the step names and the order in which they are executed... Can I write a field name that does not exist? I assume the step name is not part of these fields, but treated specially? https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... File recipe_engine/unittests/checker_test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... recipe_engine/unittests/checker_test.py:15: import test_env nit: order https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... recipe_engine/unittests/checker_test.py:32: def body(_cfn): nit: Maybe s/_cfn/_ https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... File recipe_engine/unittests/post_process_test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... recipe_engine/unittests/post_process_test.py:11: import copy nit: order https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... File recipes/engine_tests/whitelist_steps.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:16: 'fakeit': Property(kind=bool, default=True), Are you setting this to false anywhere? If not why have it? I assume setting it to false would demonstrate how MustRun below would fail. But I assume we can't do that in a benign way here... https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:31: + api.post_process(Filter('something important')) Maybe also demonstrate chaining of post-processing functions here in one tests, e.g. MustRun + Filter? https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:39: f = f.include('something important', 'env') Maybe change the order of filters to show that it's independent from the order of steps. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:41: yield (api.test('selection') + api.properties() nit: new line for each + https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:41: yield (api.test('selection') + api.properties() Why an empty properties? Did you use it to test changing fakeit?
PTAL https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/c... File recipe_engine/checker.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/c... recipe_engine/checker.py:1: # Copyright 2014 The LUCI Authors. All rights reserved. On 2016/10/13 07:45:08, machenbach (slow) wrote: > nit: current year for new files? Done. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/c... recipe_engine/checker.py:5: """Provides simulator test coverage for individual recipes.""" On 2016/10/13 07:45:08, machenbach (slow) wrote: > I'm not fully understanding this description. I understand what it says, but > maybe you could state in more detail what's implemented here? This is a documentation bug, it got copypasta'd from simulation_test.py https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/p... File recipe_engine/post_process.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/p... recipe_engine/post_process.py:78: fields (str) - The field(s) to include. Omit to include all fields. On 2016/10/13 07:45:08, machenbach (slow) wrote: > How do I prune all fields? If I, for example, only want to show the step names > and the order in which they are executed... Can I write a field name that does > not exist? I assume the step name is not part of these fields, but treated > specially? I changed the function syntax here slightly, but to remove everything from all steps except for name, you would do `api.post_process(Filter().include_re('', ['name']))`, which is "Filter to include all steps matching '', retain the 'name' field". https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... File recipe_engine/unittests/checker_test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... recipe_engine/unittests/checker_test.py:15: import test_env On 2016/10/13 07:45:08, machenbach (slow) wrote: > nit: order nope, order is important. Can't import mock until test_env is imported. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... recipe_engine/unittests/checker_test.py:32: def body(_cfn): On 2016/10/13 07:45:08, machenbach (slow) wrote: > nit: Maybe s/_cfn/_ Done. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... File recipe_engine/unittests/post_process_test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipe_engine/u... recipe_engine/unittests/post_process_test.py:11: import copy On 2016/10/13 07:45:08, machenbach (slow) wrote: > nit: order Done. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... File recipes/engine_tests/whitelist_steps.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:16: 'fakeit': Property(kind=bool, default=True), On 2016/10/13 07:45:08, machenbach (slow) wrote: > Are you setting this to false anywhere? If not why have it? > > I assume setting it to false would demonstrate how MustRun below would fail. But > I assume we can't do that in a benign way here... Oops! I did mean to set this to false in one of the tests to demonstrate DoesNotRun. PTAL. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:31: + api.post_process(Filter('something important')) On 2016/10/13 07:45:08, machenbach (slow) wrote: > Maybe also demonstrate chaining of post-processing functions here in one tests, > e.g. MustRun + Filter? done below https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:39: f = f.include('something important', 'env') On 2016/10/13 07:45:08, machenbach (slow) wrote: > Maybe change the order of filters to show that it's independent from the order > of steps. Done. https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:41: yield (api.test('selection') + api.properties() On 2016/10/13 07:45:08, machenbach (slow) wrote: > Why an empty properties? Did you use it to test changing fakeit? yeah this is where I wanted to change fakeit :) https://chromiumcodereview.appspot.com/2387763003/diff/180001/recipes/engine_... recipes/engine_tests/whitelist_steps.py:41: yield (api.test('selection') + api.properties() On 2016/10/13 07:45:08, machenbach (slow) wrote: > nit: new line for each + That's the reason I missed it :p
Description was changed from ========== Add initial postprocess unit test thingy. R=dnj@chromium.org, dpranke@chromium.org, machenbach@chromium.org, martiniss@chromium.org BUG=459361 ========== to ========== Add post_process hooks to recipe testing facilities. These post_process hooks allow the user to run arbitrary assertions on their expectation outputs, and also allows arbitrary filtering of the expectation data (removing steps/fields, items from lists, etc.), including reduction of the expectations all the way down to an empty dictionary (which will remove the expectation file from disk). This facility supersedes and replaces the recently-introduced whitelisting capabilities and will hopefully give recipe authors the testing flexibility they need to achieve confidence in the behavior of their recipes. R=dnj@chromium.org, dpranke@chromium.org, machenbach@chromium.org, martiniss@chromium.org BUG=459361 ==========
lgtm https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.py File recipe_engine/checker.py (right): https://codereview.chromium.org/2387763003/diff/120001/recipe_engine/checker.... recipe_engine/checker.py:260: elif len(a) == 1: On 2016/10/10 18:53:12, martiniss wrote: > what happens if len(a) > 1? ? https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/checker.py File recipe_engine/checker.py (right): https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/checker.... recipe_engine/checker.py:61: restrict this to Compare ops with a single operator which is `In`. ... which is `In` or `not in`? https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/checker.... recipe_engine/checker.py:115: consts = {'True': True, 'False': False, 'None': None} O_O I didn't know these kinds of constants are identifiers too? https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/checker.... recipe_engine/checker.py:191: to_push = ['body', 'orelse', 'finalbody', 'excepthandler'] Can you document what these are? I'm not sure what body is... The logic here is a bit hard to unravel https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/checker.... recipe_engine/checker.py:326: If a is a vaild subset of b, this returns None. Otherwise this returns typo https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/simulati... File recipe_engine/simulation_test.py (right): https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/simulati... recipe_engine/simulation_test.py:85: _GEN_TEST_CACHE = {} I don't see how this would do anything, is what I meant. Shouldn't this cache be a no-op? Why would we ever run a recipe twice? https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/unittest... File recipe_engine/unittests/checker_test.py (right): https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/unittest... recipe_engine/unittests/checker_test.py:121: self.mk('body', 'check((val is False))', {'val': 'True'})) this (and other) error messages have parens. Can we remove them? https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/unittest... File recipe_engine/unittests/test.py (right): https://codereview.chromium.org/2387763003/diff/200001/recipe_engine/unittest... recipe_engine/unittests/test.py:1: def foo(): What is this file for?
https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/c... File recipe_engine/checker.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/c... recipe_engine/checker.py:61: restrict this to Compare ops with a single operator which is `In`. On 2016/10/13 22:54:12, martiniss wrote: > ... which is `In` or `not in`? done https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/c... recipe_engine/checker.py:115: consts = {'True': True, 'False': False, 'None': None} On 2016/10/13 22:54:12, martiniss wrote: > O_O I didn't know these kinds of constants are identifiers too? Yep https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/c... recipe_engine/checker.py:191: to_push = ['body', 'orelse', 'finalbody', 'excepthandler'] On 2016/10/13 22:54:12, martiniss wrote: > Can you document what these are? I'm not sure what body is... > > The logic here is a bit hard to unravel added a bunch of comments https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/c... recipe_engine/checker.py:326: If a is a vaild subset of b, this returns None. Otherwise this returns On 2016/10/13 22:54:12, martiniss wrote: > typo Done. https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/s... File recipe_engine/simulation_test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/s... recipe_engine/simulation_test.py:85: _GEN_TEST_CACHE = {} On 2016/10/13 22:54:13, martiniss wrote: > I don't see how this would do anything, is what I meant. Shouldn't this cache be > a no-op? Why would we ever run a recipe twice? We always run recipes many many times... one per test case. Maybe I'm not understanding what you mean? https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/u... File recipe_engine/unittests/checker_test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/u... recipe_engine/unittests/checker_test.py:121: self.mk('body', 'check((val is False))', {'val': 'True'})) On 2016/10/13 22:54:13, martiniss wrote: > this (and other) error messages have parens. Can we remove them? Not easily; this code snippet is generated by unparsing the ast node for the statement, and the `astunparse` library doesn't know how to eliminate redundant parentheses in all cases. https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/u... File recipe_engine/unittests/test.py (right): https://chromiumcodereview.appspot.com/2387763003/diff/200001/recipe_engine/u... recipe_engine/unittests/test.py:1: def foo(): On 2016/10/13 22:54:13, martiniss wrote: > What is this file for? uhhh oops. didn't mean to include this.
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org, martiniss@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2387763003/#ps240001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Recipes-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/31d937e022cd1010)
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from martiniss@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2387763003/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add post_process hooks to recipe testing facilities. These post_process hooks allow the user to run arbitrary assertions on their expectation outputs, and also allows arbitrary filtering of the expectation data (removing steps/fields, items from lists, etc.), including reduction of the expectations all the way down to an empty dictionary (which will remove the expectation file from disk). This facility supersedes and replaces the recently-introduced whitelisting capabilities and will hopefully give recipe authors the testing flexibility they need to achieve confidence in the behavior of their recipes. R=dnj@chromium.org, dpranke@chromium.org, machenbach@chromium.org, martiniss@chromium.org BUG=459361 ========== to ========== Add post_process hooks to recipe testing facilities. These post_process hooks allow the user to run arbitrary assertions on their expectation outputs, and also allows arbitrary filtering of the expectation data (removing steps/fields, items from lists, etc.), including reduction of the expectations all the way down to an empty dictionary (which will remove the expectation file from disk). This facility supersedes and replaces the recently-introduced whitelisting capabilities and will hopefully give recipe authors the testing flexibility they need to achieve confidence in the behavior of their recipes. R=dnj@chromium.org, dpranke@chromium.org, machenbach@chromium.org, martiniss@chromium.org BUG=459361 Committed: https://github.com/luci/recipes-py/commit/d97fe4474b8ebefb906e594aba840fff587... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://github.com/luci/recipes-py/commit/d97fe4474b8ebefb906e594aba840fff587... |
