Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721613004:
simulation_test_ng: initial CL (Closed)
|
Created:
3 years, 9 months ago by Paweł Hajdan Jr. Modified:
3 years, 9 months ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Target Ref:
refs/heads/master Project:
recipe_engine Visibility:
Public. |
DescriptionAdd new 'test' command
BUG=699120
Review-Url: https://codereview.chromium.org/2721613004
Committed: https://github.com/luci/recipes-py/commit/fcd30616c93ac4532e5cfa4324ba5cd18c8b9a76
Patch Set 1 #
Total comments: 31
Patch Set 2 : review #
Total comments: 4
Patch Set 3 : less pickle #Patch Set 4 : first tests #
Total comments: 1
Patch Set 5 : tests #
Total comments: 37
Patch Set 6 : list json #Patch Set 7 : review #Patch Set 8 : revised #
Total comments: 2
Messages
Total messages: 61 (43 generated)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
iannucci@chromium.org changed reviewers: + iannucci@chromium.org
some high-level comments, this looks like a good start https://codereview.chromium.org/2721613004/diff/1/recipe_engine/post_process.py File recipe_engine/post_process.py (right): https://codereview.chromium.org/2721613004/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:14: _reEntry = namedtuple('_reEntry', 'at_most at_least fields') can we rename this to _filterRegexEntry now that it's pulled out of the Filter class? https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test_ng.py (right): https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:1: # Copyright 2017 The LUCI Authors. All rights reserved. let's just make this the top level 'test' command: 'recipes.py test list' 'recipes.py test run' # let's make `run` mandatory to prevent the current 'implicit' test weirdness 'recipes.py test train' ? Then when we remove 'simulation_test' it actually is replaced with a sane top-level command. We can also add a `recipes.py debug` at some later point too. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:73: # TODO(phajdan.jr): Consider namedtuple instead. lets do these right away so the start as immutable. The pattern is class Foo(namedtuple('_Foo', [...]): ... https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:97: def expectation_path(self): todo (DEFINITELY not in this CL, and I'd be happy to do this myself): move all expectation files to a parallel directory structure: recipes/ foo.py recipe_modules/ bar/ example.py recipe_test_outputs/ recipes/ foo/ basic.json recipe_modules/ bar/ example/ basic.json Because: * easier to exclude when bundling recipes/fetching recipes from gitiles * easier to review (gerrit/rietveld shouldn't mix expectations with actual code change any more, and `recipe_test_outputs` sorts after `recipe_modules`) * easier to reason about the recipes directories (i.e. `tree recipes/` will be much more useful) If, as you're writing this new version, you can do something to make this migration easier, keep it in mind if that's alright :). It actually looks like it would be pretty easy to implement as this CL is written right now, though. Don't go out of your way for it or anything, just so you know. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:103: # TODO(phajdan.jr): why do we need to re-encode golden data files? because of unicode strings, I think? we might be able to load them better https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:130: def _run_recipe(self): I'd strongly consider extracting this as a top-level function (probably at the top of the file) since it's probably the most important part of this system. Additionally, the smaller and more data-only the Test object is, the better; if Test was pure data it could be serialized a LOT more efficiently than pickle (which is what multiprocessing does under the hood, and it has a bug that tandrii found that makes it very inefficient). In fact, if Test was really a simple tuple of strings, that would be extremely easy for pickle to serialize and I think would avoid the performance bottleneck that we've seen in the other expect tests. Here's the workaround in the other expect_tests: https://chromium.googlesource.com/infra/testing/expect_tests/+/master/expect_... https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:184: def parse_args(args): probably want to move this directly above main https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:190: list_p.set_defaults(command='list') a technique that's useful (dnj recently pointed this out to me) is: list_p.set_defaults(func=lambda opts: run_list()) test_p.set_defaults(func=lambda opts: run_test(opts.jobs)) then after parsing+validating args, just do return opts.func(opts) Then all 'opts' definition and usage is local to the subparser, and it avoids a second command->function lookup table. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:192: test_p = subp.add_parser('test', description='Run the tests') no train? https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:195: '--jobs', metavar='N', type=int, let's make this super-simple and drop this arg for now (unless we have a use case). https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:197: help='run N jobs in parallel (default %(default)s)') missing feature that's probably essential: running a subset of the tests. The only way it would be non-essential would be if running the tests using this new implementation is much faster than the current implementation. The 'subset' logic doesn't need to be as fancy. Could just be `--recipe foo --test_name name`. Because it doesn't use globbing the way that expect_tests did, this could be implemented to be much faster than the current 'generate all tests, but then ignore 99% of them via the glob' way of doing it. filtering the result of 'list' is probably best done with grep anyway, but it might be useful to be able to pass it --recipe as well. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:202: def get_tests(): all of these functions need docstrings at some point https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:227: if (getattr(mod, 'DISABLE_STRICT_COVERAGE', False)): wdyt about just putting this logic in loader so that modules ALWAYS have DISABLE_STRICT_COVERAGE set on them, so we don't need these getattrs outside of loader.py? i.e. do mod.DISABLE_STRICT_COVERAGE = getattr(mod, 'DISABLE_STRICT_COVERAGE', False) When loading the module. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:321: tests, coverage_data, uncovered_modules = get_tests() this can be slow on large repos (like build). Can we at least make this a generator for entries in the pool so that tests can begin executing as we generate them. You can run them in the pool with apply_async. Something like: all_coverage_data = coverage.CoverageData() uncovered_modules = set(_UNIVERSE_VIEW.loop_over_recipe_modules()) result_promises = [] results = [] tests = [] with kill_switch(): for test, coverage_data, covered_modules in generate_tests(): tests.append(test) all_coverage_data.update(coverage_data) uncovered_modules -= covered_modules # now tests can start running immediately in parallel result_promises = pool.apply_async(run_test_worker, test) for r in results: results.append(r.get()) # results has the same order as tests if uncovered_modules: ... (this is still much simpler than expect_tests because the main thread does generation AND collection, instead of just collection). WDYT about having the result of the test actually include that test's coverage data? We might be able to get rid of the intermediate coverage files on the filesystem that way. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:328: results = pool.map(run_test_worker, tests) This will fail (and I suspect is the reason why you moved _reEntry into the global space in the other file): the test_data object is not guaranteed to serialize across process boundaries (it can contain functions so that post_process_hooks work). This is the reason why the other version just pushed the name of the test across the process boundary, and then re-loaded + cached all of the tests in the worker thread (and then used the generated-by-worker-process test data, which can contain anything generated by GenTests).
tandrii@chromium.org changed reviewers: + tandrii@chromium.org
https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test_ng.py (right): https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:73: # TODO(phajdan.jr): Consider namedtuple instead. On 2017/02/28 23:22:59, iannucci wrote: > lets do these right away so the start as immutable. The pattern is > > > class Foo(namedtuple('_Foo', [...]): > ... +1 in general but namedtuples carry inside a lot of crap due to exec call [1] that is dangerous to pickle/unpickle in multiprocess env and often lead to bugs. Thus, I am lately preferring boilerplate instead debugging problems like that, ending up running >>> collections.namedtuple('X', 'y z', verbose=True) then copy-pasting output to my CL and doing s/_tuple/tuple s/_property/property [1] https://hg.python.org/cpython/file/2.7/Lib/collections.py#l386
Description was changed from ========== simulation_test_ng: initial CL BUG=none ========== to ========== simulation_test_ng: initial CL BUG=699120 ==========
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
PTAL. I don't consider this final review state yet. Once we get on the same page on the code structure etc, I'd like to add some tests. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/post_process.py File recipe_engine/post_process.py (right): https://codereview.chromium.org/2721613004/diff/1/recipe_engine/post_process.... recipe_engine/post_process.py:14: _reEntry = namedtuple('_reEntry', 'at_most at_least fields') On 2017/02/28 23:22:59, iannucci wrote: > can we rename this to _filterRegexEntry now that it's pulled out of the Filter > class? Done. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test_ng.py (right): https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:1: # Copyright 2017 The LUCI Authors. All rights reserved. On 2017/02/28 23:22:59, iannucci wrote: > let's just make this the top level 'test' command: > 'recipes.py test list' > 'recipes.py test run' # let's make `run` mandatory to prevent the current > 'implicit' test weirdness > 'recipes.py test train' > > ? > > Then when we remove 'simulation_test' it actually is replaced with a sane > top-level command. > > We can also add a `recipes.py debug` at some later point too. > Done. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:97: def expectation_path(self): On 2017/02/28 23:22:59, iannucci wrote: > todo (DEFINITELY not in this CL, and I'd be happy to do this myself): move all > expectation files to a parallel directory structure: > > recipes/ > foo.py > recipe_modules/ > bar/ > example.py > recipe_test_outputs/ > recipes/ > foo/ > basic.json > recipe_modules/ > bar/ > example/ > basic.json > > Because: > * easier to exclude when bundling recipes/fetching recipes from gitiles > * easier to review (gerrit/rietveld shouldn't mix expectations with actual > code change any more, and `recipe_test_outputs` sorts after `recipe_modules`) > * easier to reason about the recipes directories (i.e. `tree recipes/` will be > much more useful) > > If, as you're writing this new version, you can do something to make this > migration easier, keep it in mind if that's alright :). It actually looks like > it would be pretty easy to implement as this CL is written right now, though. > Don't go out of your way for it or anything, just so you know. Yup, added a TODO. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:103: # TODO(phajdan.jr): why do we need to re-encode golden data files? On 2017/02/28 23:22:59, iannucci wrote: > because of unicode strings, I think? we might be able to load them better I suspect so, and will look into this more when implementing the train command. I'm still planning to keep this a drop-in replacement until everything is migrated. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:130: def _run_recipe(self): On 2017/02/28 23:22:59, iannucci wrote: > I'd strongly consider extracting this as a top-level function (probably at the > top of the file) since it's probably the most important part of this system. I was considering this. Unless/until it's reused by something else, I'm not sure if that makes a big difference. > Here's the workaround in the other expect_tests: > https://chromium.googlesource.com/infra/testing/expect_tests/+/master/expect_... I'd prefer to avoid having wrappers for multiprocess module which change the locking semantics. I'd also prefer to defer optimization to later stages, and only optimize as needed, to avoid premature optimization. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:184: def parse_args(args): On 2017/02/28 23:22:59, iannucci wrote: > probably want to move this directly above main Done. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:190: list_p.set_defaults(command='list') On 2017/02/28 23:22:59, iannucci wrote: > a technique that's useful (dnj recently pointed this out to me) is: > > list_p.set_defaults(func=lambda opts: run_list()) > test_p.set_defaults(func=lambda opts: run_test(opts.jobs)) > > then after parsing+validating args, just do > > return opts.func(opts) > > Then all 'opts' definition and usage is local to the subparser, and it avoids a > second command->function lookup table. Done. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:192: test_p = subp.add_parser('test', description='Run the tests') On 2017/02/28 23:22:59, iannucci wrote: > no train? Not yet. Obviously I'll add it next. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:195: '--jobs', metavar='N', type=int, On 2017/02/28 23:22:59, iannucci wrote: > let's make this super-simple and drop this arg for now (unless we have a use > case). If that's "for now", why do that if I already implemented it anyway? I found it useful, and would prefer to keep the code. I use --jobs=1 to look for any differences between that and true parallel mode. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:197: help='run N jobs in parallel (default %(default)s)') On 2017/02/28 23:22:59, iannucci wrote: > missing feature that's probably essential: running a subset of the tests. The > only way it would be non-essential would be if running the tests using this new > implementation is much faster than the current implementation. > > The 'subset' logic doesn't need to be as fancy. Could just be `--recipe foo > --test_name name`. Because it doesn't use globbing the way that expect_tests > did, this could be implemented to be much faster than the current 'generate all > tests, but then ignore 99% of them via the glob' way of doing it. > > filtering the result of 'list' is probably best done with grep anyway, but it > might be useful to be able to pass it --recipe as well. Yes, I plan to also implement this feature. Added a TODO for now, I think it's fine for the very first CL in this area. The filter would be a simple wrapper around get_tests. I'm confident the current design will support this well. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:202: def get_tests(): On 2017/02/28 23:22:59, iannucci wrote: > all of these functions need docstrings at some point Done. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:227: if (getattr(mod, 'DISABLE_STRICT_COVERAGE', False)): On 2017/02/28 23:22:59, iannucci wrote: > wdyt about just putting this logic in loader so that modules ALWAYS have > DISABLE_STRICT_COVERAGE set on them, so we don't need these getattrs outside of > loader.py? i.e. do > > mod.DISABLE_STRICT_COVERAGE = getattr(mod, 'DISABLE_STRICT_COVERAGE', False) > > When loading the module. Done. https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:321: tests, coverage_data, uncovered_modules = get_tests() On 2017/02/28 23:22:59, iannucci wrote: > this can be slow on large repos (like build). > > Can we at least make this a generator for entries in the pool so that tests can > begin executing as we generate them. I considered this. I'd still prefer to keep the code simple for now and optimize later as needed. The performance difference didn't seem to be very large in my experience. > WDYT about having the result of the test actually include that test's coverage > data? We might be able to get rid of the intermediate coverage files on the > filesystem that way. That's a useful suggestion. I believe the code already does this. Or did you mean something else? One temporary file is needed because I didn't find an API in coverage to load results from memory/CoverageData. Please let me know if you see a cleaner solution to that (I might actually write a patch for coverage, just wanted to have a working CL here first, not blocking on that). https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:328: results = pool.map(run_test_worker, tests) On 2017/02/28 23:22:59, iannucci wrote: > This will fail (and I suspect is the reason why you moved _reEntry into the > global space in the other file). Yes, that's the reason and with that change, it works. There is a tradeoff between re-loading the recipe in the worker, and pickling and sending more data between processes. For now I've optimized for code being most straightfoward, and I'd be fine with changing that in the future. Please let me know what's the actionable feedback for this first CL. I'd be fine with only pushing test name, just wondered it might be premature optimization.
https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... File recipe_engine/simulation_test_ng.py (right): https://codereview.chromium.org/2721613004/diff/1/recipe_engine/simulation_te... recipe_engine/simulation_test_ng.py:130: def _run_recipe(self): On 2017/03/07 19:03:36, Paweł Hajdan Jr. wrote: > On 2017/02/28 23:22:59, iannucci wrote: > > I'd strongly consider extracting this as a top-level function (probably at the > > top of the file) since it's probably the most important part of this system. > > I was considering this. Unless/until it's reused by something else, I'm not sure > if that makes a big difference. > > > Here's the workaround in the other expect_tests: > > > https://chromium.googlesource.com/infra/testing/expect_tests/+/master/expect_... > > I'd prefer to avoid having wrappers for multiprocess module which change the > locking semantics. nit: it doesn't change locking semantics :)
https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py File recipe_engine/test.py (right): https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py#n... recipe_engine/test.py:193: in self.test_data.post_process_hooks: I think you misunderstood me :). This implementation WILL NOT WORK for repos other than recipe_engine, for the reasons I said in my previous comment. Users pass closures via the post process hooks, and it is not possible to pickle closures across processes. The cost of loading the recipes' tests and caching them is small in repos like build, compared to the number of tests that need to run, and fast enough for small repos when cached that it shouldn't matter. https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py#n... recipe_engine/test.py:215: return (result_data, failed_checks, cov.get_data()) pickling the entire result data across processes is one of the things that makes the current implementation slow. I would consider having this function write/compare the result directly.
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
PTAL (still not final review yet) https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py File recipe_engine/test.py (right): https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py#n... recipe_engine/test.py:193: in self.test_data.post_process_hooks: On 2017/03/07 19:13:38, iannucci wrote: > I think you misunderstood me :). This implementation WILL NOT WORK for repos > other than recipe_engine, for the reasons I said in my previous comment. Users > pass closures via the post process hooks, and it is not possible to pickle > closures across processes. Unless I'm severely missing something, this did in fact work with recipe_engine, depot_tools, build, and infra repos. Anyway, I changed the code to only pickle TestDescription so we avoid the controversy. > The cost of loading the recipes' tests and caching them is small in repos like > build, compared to the number of tests that need to run, and fast enough for > small repos when cached that it shouldn't matter. I'd prefer not to worry about premature optimization right now. https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py#n... recipe_engine/test.py:215: return (result_data, failed_checks, cov.get_data()) On 2017/03/07 19:13:38, iannucci wrote: > pickling the entire result data across processes is one of the things that makes > the current implementation slow. I would consider having this function > write/compare the result directly. Again, I'd prefer not to worry about premature optimization right now.
On 2017/03/07 22:53:02, Paweł Hajdan Jr. wrote: > PTAL (still not final review yet) > > https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py > File recipe_engine/test.py (right): > > https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py#n... > recipe_engine/test.py:193: in self.test_data.post_process_hooks: > On 2017/03/07 19:13:38, iannucci wrote: > > I think you misunderstood me :). This implementation WILL NOT WORK for repos > > other than recipe_engine, for the reasons I said in my previous comment. Users > > pass closures via the post process hooks, and it is not possible to pickle > > closures across processes. > > Unless I'm severely missing something, this did in fact work with recipe_engine, > depot_tools, build, and infra repos. > I added a test to recipe engine which demonstrates what I was meaning. > Anyway, I changed the code to only pickle TestDescription so we avoid the > controversy. > > > The cost of loading the recipes' tests and caching them is small in repos like > > build, compared to the number of tests that need to run, and fast enough for > > small repos when cached that it shouldn't matter. > > I'd prefer not to worry about premature optimization right now. > > https://codereview.chromium.org/2721613004/diff/20001/recipe_engine/test.py#n... > recipe_engine/test.py:215: return (result_data, failed_checks, cov.get_data()) > On 2017/03/07 19:13:38, iannucci wrote: > > pickling the entire result data across processes is one of the things that > makes > > the current implementation slow. I would consider having this function > > write/compare the result directly. > > Again, I'd prefer not to worry about premature optimization right now. Maybe you mean something different than I understand by 'premature optimization', but we already have a mature implementation which suffers from these performance issues :)
As requested in chat, I would like to formally indicate that I have no additional comments on this CL at the present time. Please proceed to add tests so that we may commence additional review on this change.
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: Recipes-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/34c7d4748c9af910)
https://codereview.chromium.org/2721613004/diff/60001/unittests/test_test.py File unittests/test_test.py (right): https://codereview.chromium.org/2721613004/diff/60001/unittests/test_test.py#... unittests/test_test.py:82: [{'name': '$result', 'recipe_result': None, 'status_code': 0}]) This invocation is pretty cryptic. Wdyt about r = self.Recipe('foo') # any of these can be omitted r.Deps = [ "foo/bar", ] r.RunStepsBody = """ pass """ r.GenTestsBody = """ yield api.test('basic') """ r.Expectations['basic.json'] = { ... } r.write() I think something like that will make it much clearer what's going on, and will make us much more likely to add more tests in the future :).
(the test cases you have so far look good though, if I understand them correctly)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== simulation_test_ng: initial CL BUG=699120 ========== to ========== Add new 'test' command BUG=699120 ==========
PTAL . I consider this now ready for final review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Recipes-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/34cdab9c72983610)
lgtm w/ comments! https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py File recipe_engine/test.py (right): https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:74: class TestFailure(object): wdyt about putting some of this test code in a subdirectory of recipe_engine? e.g. recipe_engine/test_impl/exceptions.py, etc. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:75: """Generic class for different kinds of test failures.""" Generic or 'Base' class? I assume this is never meant to be raised directly? https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:152: sys.stdout.write('C') are these writes going to happen from multiple threads/processes? This might get confusing if so. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:155: diff = '\n'.join(list(difflib.unified_diff( is the 'list' necessary? afaik, ''.join will work with a generator just fine without buffering the output into an extra copy first https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:186: _GEN_TEST_CACHE[(recipe_name, test_data.name)] = copy.deepcopy(test_data) use cache_key? we had a bug before where we were writing into the cache with key X, but doing a lookup using key Y; as a result it was continually reloading and copying everything :( Actually now that I look at this (and the existing implementation), I would recommend making this a 2-tier cache of CACHE[recipe_name][test_name]. Then the initial lookup will be for the recipe, and then it will either load the full recipe into the cache, or use the existing one, then do a direct lookup for the test name. Otherwise in the case of a bug, we might run a given recipe's GenTests function more than once, and in the case of flake, this might lead to things somehow working some of the time. An example of this might be if the user's recipe generates test names non-deterministically. That SHOULD be a failure, but in this implementation it MIGHT succeed. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:213: result.result.pop('traceback', None) should we replace the traceback with something indicating there WAS a traceback? I wouldn't do that in this CL (it will probably change expectations). maybe TODO? https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:229: # functions in recipe test code. +1 https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:262: coverage_include = os.path.join(_UNIVERSE_VIEW.module_dir, '*', '*.py') nit: this could be moved outside the loop https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:333: omit.append(os.path.join(mod_dir_base, '*', 'resources', '*')) is the final * necessary? because some modules have subdirs under resources too (this is the recursive loading thing). Does this accept **? https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:378: tests, coverage_data, uncovered_modules = get_tests() should this run with the kill switch too? or does ctrl-c work fine to cancel get_tests? https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:451: # Reset the signal to DFL so that double ctrl-C kills us for sure. print "Got Ctrl-C: Stopping. Ctrl-C again to shut down uncleanly" ? https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:473: def re_encode(obj): todo: look into custom decoder class: https://docs.python.org/2/library/json.html#json.JSONDecoder to just do all this on the first pass. We should incorporate it into the 'json' recipe module too. https://codereview.chromium.org/2721613004/diff/80001/recipes.py File recipes.py (right): https://codereview.chromium.org/2721613004/diff/80001/recipes.py#newcode71 recipes.py:71: logging.error( has logging been configured yet? I think you need to do a logging.basicConfig() or some such for this to work. https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py File unittests/test_test.py (right): https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:51: commands(list): list of expected commands I think this is a list of expectation dictionaries? is that correct? https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:164: 'def GenTests(api):', this looks duplicated from RecipeWriter... could a RecipeModuleWriter have-a RecipeWriter instead? https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:226: self.assertNotIn('FATAL: Insufficient coverage', cm.exception.output) maybe TODO: use jsonproto output of 'test' instead of parsing stdout? Internally test could produce its entire result as jsonproto, and then there would be a small, independently tested printer which does proto->human stdout. https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:414: self.assertIn('FATAL: Insufficient coverage', cm.exception.output) I don't see any tests for the post_process hooks? or are those covered elsewhere? edit: I guess that's covered by the stdlib_test.py test above?
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: Recipes-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/34d19aacff3bb310)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py File recipe_engine/test.py (right): https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:74: class TestFailure(object): On 2017/03/10 01:20:09, iannucci wrote: > wdyt about putting some of this test code in a subdirectory of recipe_engine? > e.g. recipe_engine/test_impl/exceptions.py, etc. With slightly over 500 lines, I don't see big benefits of splitting. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:75: """Generic class for different kinds of test failures.""" On 2017/03/10 01:20:08, iannucci wrote: > Generic or 'Base' class? I assume this is never meant to be raised directly? Yeah, changed to base. Obviously it's not meant to be raised directly - format() raises NotImplementedError(). https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:152: sys.stdout.write('C') On 2017/03/10 01:20:09, iannucci wrote: > are these writes going to happen from multiple threads/processes? Yes. > This might get confusing if so. Why? https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:155: diff = '\n'.join(list(difflib.unified_diff( On 2017/03/10 01:20:09, iannucci wrote: > is the 'list' necessary? afaik, ''.join will work with a generator just fine > without buffering the output into an extra copy first Done. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:186: _GEN_TEST_CACHE[(recipe_name, test_data.name)] = copy.deepcopy(test_data) On 2017/03/10 01:20:09, iannucci wrote: > use cache_key? we had a bug before where we were writing into the cache with key > X, but doing a lookup using key Y; as a result it was continually reloading and > copying everything :( We can't use cache_key on this line, because test_name is only equal to test_data.name for _one_ of the loop iterations, not _all_ of them. Obviously cache_key is used on the line right below. > Actually now that I look at this (and the existing implementation), I would > recommend making this a 2-tier cache of CACHE[recipe_name][test_name]. Then the > initial lookup will be for the recipe, and then it will either load the full > recipe into the cache, or use the existing one, then do a direct lookup for the > test name. I don't see an advantage of this. If a recipe is present in the cache, so are all its tests. > Otherwise in the case of a bug, we might run a given recipe's > GenTests function more than once, and in the case of flake, this might lead to > things somehow working some of the time. We're already running GenTests more than once: first in gen_tests, then in each of the worker processes. > An example of this might be if the user's recipe generates test names > non-deterministically. That SHOULD be a failure, but in this implementation it > MIGHT succeed. I think in general the recipe engine should be robust against user recipe code doing weird things. Realistically, there are limits of what we can do, including whether it's worth the complexity. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:213: result.result.pop('traceback', None) On 2017/03/10 01:20:09, iannucci wrote: > should we replace the traceback with something indicating there WAS a traceback? > I wouldn't do that in this CL (it will probably change expectations). maybe > TODO? Added a TODO. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:262: coverage_include = os.path.join(_UNIVERSE_VIEW.module_dir, '*', '*.py') On 2017/03/10 01:20:09, iannucci wrote: > nit: this could be moved outside the loop Done. FWIW, there's a slight tradeoff between "efficiency", and the variable being closer to where it's used. Whatever. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:333: omit.append(os.path.join(mod_dir_base, '*', 'resources', '*')) On 2017/03/10 01:20:09, iannucci wrote: > is the final * necessary? Yes. Otherwise simulation tests inside recipe engine itself fail. Also note this is what the current code does, see cover_omit in recipe_engine/simulation_test.py . > because some modules have subdirs under resources too > (this is the recursive loading thing). Does this accept **? It does, but not sure if it means what you want. Again, this matches what current code does, I tested against all infra repos, and https://codereview.chromium.org/2738053005/ will remove recursive loading. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:378: tests, coverage_data, uncovered_modules = get_tests() On 2017/03/10 01:20:09, iannucci wrote: > should this run with the kill switch too? or does ctrl-c work fine to cancel > get_tests? kill_switch is only needed for multiprocessing. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:451: # Reset the signal to DFL so that double ctrl-C kills us for sure. On 2017/03/10 01:20:09, iannucci wrote: > print "Got Ctrl-C: Stopping. Ctrl-C again to shut down uncleanly" ? This would be non-trivial. If done as-is, it'd print this _after_ bash prompt, possibly more than once. As far as I know, the current code doesn't do this, and I'm not sure what's the specific advantage of printing this message. From user POV, if things hang, there doesn't seem to be a big difference between "clean" and "unclean" shutdown. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:473: def re_encode(obj): On 2017/03/10 01:20:08, iannucci wrote: > todo: look into custom decoder class: > https://docs.python.org/2/library/json.html#json.JSONDecoder to just do all this > on the first pass. > > We should incorporate it into the 'json' recipe module too. Added a TODO. FWIW, this matches what current code does. https://codereview.chromium.org/2721613004/diff/80001/recipes.py File recipes.py (right): https://codereview.chromium.org/2721613004/diff/80001/recipes.py#newcode71 recipes.py:71: logging.error( On 2017/03/10 01:20:09, iannucci wrote: > has logging been configured yet? I think you need to do a logging.basicConfig() > or some such for this to work. This matches what simulation_test does... See right above. https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py File unittests/test_test.py (right): https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:51: commands(list): list of expected commands On 2017/03/10 01:20:09, iannucci wrote: > I think this is a list of expectation dictionaries? is that correct? Done. https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:164: 'def GenTests(api):', On 2017/03/10 01:20:09, iannucci wrote: > this looks duplicated from RecipeWriter... could a RecipeModuleWriter have-a > RecipeWriter instead? I was considering that, but IMO it could make things unnecessarily slightly more complicated. https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:226: self.assertNotIn('FATAL: Insufficient coverage', cm.exception.output) On 2017/03/10 01:20:09, iannucci wrote: > maybe TODO: use jsonproto output of 'test' instead of parsing stdout? Internally > test could produce its entire result as jsonproto, and then there would be a > small, independently tested printer which does proto->human stdout. Yes, I plan to add JSON output support in subsequent CLs. Already done for list command (was necessary to pass presubmit tests on trybots - otherwise there was some warning noise). https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:414: self.assertIn('FATAL: Insufficient coverage', cm.exception.output) On 2017/03/10 01:20:09, iannucci wrote: > I don't see any tests for the post_process hooks? or are those covered > elsewhere? Did you see test_test_check_failure and test_test_check_success? > edit: I guess that's covered by the stdlib_test.py test above? That too.
still lgtm though I would strongly advise re-using the recipewriter class instead of copypasting it, and guarding against multiple GenTests runs per recipe. https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py File recipe_engine/test.py (right): https://codereview.chromium.org/2721613004/diff/80001/recipe_engine/test.py#n... recipe_engine/test.py:186: _GEN_TEST_CACHE[(recipe_name, test_data.name)] = copy.deepcopy(test_data) On 2017/03/10 17:20:30, Paweł Hajdan Jr. wrote: > On 2017/03/10 01:20:09, iannucci wrote: > > use cache_key? we had a bug before where we were writing into the cache with > key > > X, but doing a lookup using key Y; as a result it was continually reloading > and > > copying everything :( > > We can't use cache_key on this line, because test_name is only equal to > test_data.name for _one_ of the loop iterations, not _all_ of them. > Oh, heh, yeah I'm dumb. > Obviously cache_key is used on the line right below. > > > Actually now that I look at this (and the existing implementation), I would > > recommend making this a 2-tier cache of CACHE[recipe_name][test_name]. Then > the > > initial lookup will be for the recipe, and then it will either load the full > > recipe into the cache, or use the existing one, then do a direct lookup for > the > > test name. > > I don't see an advantage of this. If a recipe is present in the cache, so are > all its tests. > Only if GenTests is deterministic. Say that it generates 99% the same test names when you run it. In this model, the main thread will generate test name A, B, C and X, and this worker will generate A, B, C and Z. The worker generates everything when it sees the first test A, and then re-generates everything again for test X (because (recipe, X) isn't in the cache). > > Otherwise in the case of a bug, we might run a given recipe's > > GenTests function more than once, and in the case of flake, this might lead to > > things somehow working some of the time. > > We're already running GenTests more than once: first in gen_tests, then in each > of the worker processes. Yes, and what I'm saying is that we should run them AT MOST 1+N times (one in the main thread, one per worker). Right now there are scenarios where they could run more than that (and not fail where they should). > > > An example of this might be if the user's recipe generates test names > > non-deterministically. That SHOULD be a failure, but in this implementation it > > MIGHT succeed. > > I think in general the recipe engine should be robust against user recipe code > doing weird things. Realistically, there are limits of what we can do, including > whether it's worth the complexity. In general, sure, but a 2D map isn't complex. https://codereview.chromium.org/2721613004/diff/80001/recipes.py File recipes.py (right): https://codereview.chromium.org/2721613004/diff/80001/recipes.py#newcode71 recipes.py:71: logging.error( On 2017/03/10 17:20:30, Paweł Hajdan Jr. wrote: > On 2017/03/10 01:20:09, iannucci wrote: > > has logging been configured yet? I think you need to do a > logging.basicConfig() > > or some such for this to work. > > This matches what simulation_test does... See right above. expect_tests doesn't use logging so it doesn't configure it. you're using logging, you need to configure it. luqui added a logging statement to simulation_test three years ago, but didn't configure logging, so it won't work: https://github.com/luci/recipes-py/blame/master/recipe_engine/simulation_test.... Maybe that's what you were looking at? https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py File unittests/test_test.py (right): https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:164: 'def GenTests(api):', On 2017/03/10 17:20:30, Paweł Hajdan Jr. wrote: > On 2017/03/10 01:20:09, iannucci wrote: > > this looks duplicated from RecipeWriter... could a RecipeModuleWriter have-a > > RecipeWriter instead? > > I was considering that, but IMO it could make things unnecessarily slightly more > complicated. How is it more complex to reuse a well-defined class instead of copy and pasting its implementation in multiple places? https://codereview.chromium.org/2721613004/diff/80001/unittests/test_test.py#... unittests/test_test.py:414: self.assertIn('FATAL: Insufficient coverage', cm.exception.output) On 2017/03/10 17:20:30, Paweł Hajdan Jr. wrote: > On 2017/03/10 01:20:09, iannucci wrote: > > I don't see any tests for the post_process hooks? or are those covered > > elsewhere? > > Did you see test_test_check_failure and test_test_check_success? Yes, but they don't cover the case of a user-provided function, but it's covered here https://github.com/luci/recipes-py/blob/master/recipes/engine_tests/whitelist... so it's fine. > > > edit: I guess that's covered by the stdlib_test.py test above? > > That too.
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2721613004/diff/140001/recipe_engine/test.py File recipe_engine/test.py (right): https://codereview.chromium.org/2721613004/diff/140001/recipe_engine/test.py#... recipe_engine/test.py:309: test_data) Isn't this in a different process? Does this make the multiproccess jump correctly? If so then awesome :)
https://codereview.chromium.org/2721613004/diff/140001/recipe_engine/test.py File recipe_engine/test.py (right): https://codereview.chromium.org/2721613004/diff/140001/recipe_engine/test.py#... recipe_engine/test.py:309: test_data) On 2017/03/10 20:32:30, iannucci wrote: > Isn't this in a different process? Does this make the multiproccess jump > correctly? If so then awesome :) Other process will see it after the fork, just like _UNIVERSE_VIEW and _ENGINE_FLAGS. That's also what the current code does. While I haven't tested on Windows yet, at least on Linux it works just fine.
The CQ bit was checked by phajdan.jr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489178217948830,
"parent_rev": "5f5c9b29d1f6706827f51fb6f7b7d30cdb135d2c", "commit_rev":
"fcd30616c93ac4532e5cfa4324ba5cd18c8b9a76"}
Message was sent while issue was closed.
Description was changed from ========== Add new 'test' command BUG=699120 ========== to ========== Add new 'test' command BUG=699120 Review-Url: https://codereview.chromium.org/2721613004 Committed: https://github.com/luci/recipes-py/commit/fcd30616c93ac4532e5cfa4324ba5cd18c8... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://github.com/luci/recipes-py/commit/fcd30616c93ac4532e5cfa4324ba5cd18c8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
