Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
220353003:
Replace recipes_test.py with a new parallel expectation-based test runner. (Closed)
|
Created:
6 years, 8 months ago by iannucci Modified:
6 years, 8 months ago CC:
chromium-reviews, kjellander-cc_chromium.org, cmp-cc_chromium.org, ilevy-cc_chromium.org, stip+watch_chromium.org Visibility:
Public. |
DescriptionReplace recipes_test.py with a new parallel expectation-based test runner.
It has Features(tm):
* It's not slow now (2x faster on my macbook air. Parallelizes up to
num_cores correctly)
* list/train/test/debug all tests
* All commands allow you to glob test names
* Debugger breaks at GenSteps() and api.step()
* Coverage summary only shows uncovered lines in report by default.
* Supports YAML expectations (not enabled with this commit)
* Shows real context diff when the test expectation doesn't match.
* Sort-of-compatible output with unittest
* Ctrl-C works.
* Produces identical expectations to the current test
* It's not flakey
R=agable@chromium.org, stip@chromium.org, vadimsh@chromium.org, dpranke@chromium.org
BUG=353884
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=261284
Patch Set 1 #
Total comments: 62
Patch Set 2 : all but docstrings #Patch Set 3 : docstrings #
Total comments: 2
Patch Set 4 : refactor #
Total comments: 18
Patch Set 5 : whitespace #Patch Set 6 : tweak shebang and clear up test_env imoprt #
Total comments: 4
Patch Set 7 : address comments #
Total comments: 11
Patch Set 8 : comments and fix race #
Total comments: 2
Patch Set 9 : fix oops and demote coverage version to allow cq to pass. 3.7.1 isn't needed anyway #Patch Set 10 : fix old pylint #Patch Set 11 : add renamed file... #Patch Set 12 : *sigh* more pylint #Patch Set 13 : fix coverage race #Messages
Total messages: 42 (0 generated)
https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/reci... File scripts/slave/unittests/recipe_simulation_test.py (right): https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/reci... scripts/slave/unittests/recipe_simulation_test.py:55: [os.path.join(x, '*', '*api.py') for x in recipe_util.MODULE_DIRS()] Note that this should really be '*.py', but that actually reveals a couple uncovered lines of code *gasp*, so carrying this over from the previous tests.
https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... File scripts/slave/unittests/expect_tests.py (right): https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:645: f.func_code.co_name)) Need to do this lookup right away since we can't be guaranteed to be able to pickle all funcs in break_funcs, but (path, lineno, funcname) is portable.
https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... File scripts/slave/unittests/expect_tests.py (right): https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:51: elif isinstance(obj, list): + tuple? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:58: import json please, keep imports on top :( https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:65: import yaml on top: try: import yaml except ImportError: yaml = None here: if yaml: ... https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:69: lambda stream: yaml.load(stream, _YAMLSafeLoader), why not just yaml.safe_load? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:101: @staticmethod @classmethod? because, well, it's classier? :) https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:108: class GenLoop(object): Why Loop? It looks more like Stage to me... https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:115: @param test: The generated Test object. That's a very verbose markup for type information :( What tool consumes it? Also it's inconsistently used. Maybe drop it for now? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:176: """list all of the tests instead of running them.""" nit: List with capital L, same below. Lower case them when stuffing in help=''? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:195: class GenLoop(_Handler.GenLoop): nit: new line https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:232: "they contain a diff from the current expectations." starting a string with '...' quotes and ending with "..." :D Why?.. https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:523: test_queue.put_nowait(None) Hm.. Maybe it should be in 'finally' block? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:577: help='See `[mode] --help` for more options.') does '`' have any special meaning here? I thought it's a speck of dust on my monitor... https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:593: default=multiprocessing.cpu_count(), I think it can lie on windows. But we probably don't care? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:597: '--test_list', metavar="FILE", nit: 'FILE' not "FILE" https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:614: with open(opts.test_list, 'rb') as tl: What about '-' handling? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:620: del opts.test_list why? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:631: 'name func args kwargs expectdir expectbase fmt breakpoints') nit: expect_dir, expect_base. https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:634: def __new__(cls, name, func, args=(), kwargs=None, expectdir=None, Doc strings for args. Esp. breakpoints and break_funcs. https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:640: for f in (break_funcs or (func,)): Why 'or (func,)' here? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:641: if hasattr(f, 'im_func'): What's im_func? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:650: def expect_path(self, fmt=None): nit: fmt -> ext? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:659: def main(test_gen, coverage_includes=None, coverage_omits=None, args=None): Doc string. https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:664: signal.signal(signal.SIGINT, lambda *_: kill_switch.set()) what about SIGTERM? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:690: target=_GenLoopProcess, args=test_gen_args)] Maybe always run _GenLoopProcess in current process? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:708: error |= result_handler(result_queue.get(timeout=0.1)) is False 'is False' is scetchy. Maybe 'is Failure' where Failure = object()? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:713: if not procs: nit: if not any(p.is_alive() for p in procs): https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:720: c.combine() How does it work? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/reci... File scripts/slave/unittests/recipe_simulation_test.py (right): https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/reci... scripts/slave/unittests/recipe_simulation_test.py:10: import test_env # pylint: disable=unused-import nit: from . import test_env? And same for expect_tests below. Also, are you importing test_env for side effects? Add a comment about that. https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/reci... scripts/slave/unittests/recipe_simulation_test.py:22: stream = annotator.StructuredAnnotationStream(stream=open(os.devnull, 'w')) Does os.devnull work on windows?
will add missing docstrings @ home https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... File scripts/slave/unittests/expect_tests.py (right): https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:51: elif isinstance(obj, list): On 2014/04/01 03:00:26, Vadim Sh. wrote: > + tuple? I don't think that json.load can generate a tuple https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:58: import json On 2014/04/01 03:00:26, Vadim Sh. wrote: > please, keep imports on top :( Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:65: import yaml On 2014/04/01 03:00:26, Vadim Sh. wrote: > on top: > try: > import yaml > except ImportError: > yaml = None > > here: > if yaml: > ... Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:69: lambda stream: yaml.load(stream, _YAMLSafeLoader), On 2014/04/01 03:00:26, Vadim Sh. wrote: > why not just yaml.safe_load? Because last I checked (~December), it defaults to yaml.SafeLoader (slower) https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:101: @staticmethod On 2014/04/01 03:00:26, Vadim Sh. wrote: > @classmethod? because, well, it's classier? :) But it's not a classmethod... it's a static method in the _Handler class. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:108: class GenLoop(object): On 2014/04/01 03:00:26, Vadim Sh. wrote: > Why Loop? It looks more like Stage to me... Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:115: @param test: The generated Test object. On 2014/04/01 03:00:26, Vadim Sh. wrote: > That's a very verbose markup for type information :( epydoc > What tool consumes it? Also > it's inconsistently used. Maybe drop it for now? YouCompleteMe. I added it where it wasn't automatically determining the type information. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:176: """list all of the tests instead of running them.""" On 2014/04/01 03:00:26, Vadim Sh. wrote: > nit: List with capital L, same below. Lower case them when stuffing in help=''? Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:195: class GenLoop(_Handler.GenLoop): On 2014/04/01 03:00:26, Vadim Sh. wrote: > nit: new line Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:232: "they contain a diff from the current expectations." On 2014/04/01 03:00:26, Vadim Sh. wrote: > starting a string with '...' quotes and ending with "..." :D Why?.. lol, Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:523: test_queue.put_nowait(None) On 2014/04/01 03:00:26, Vadim Sh. wrote: > Hm.. Maybe it should be in 'finally' block? Good point. Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:577: help='See `[mode] --help` for more options.') On 2014/04/01 03:00:26, Vadim Sh. wrote: > does '`' have any special meaning here? I thought it's a speck of dust on my > monitor... No, just implies 'thing to run'. Should I change to " or ' ? https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:593: default=multiprocessing.cpu_count(), On 2014/04/01 03:00:26, Vadim Sh. wrote: > I think it can lie on windows. But we probably don't care? Yeah don't really care. It's probably real cores not hyperthreads. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:597: '--test_list', metavar="FILE", On 2014/04/01 03:00:26, Vadim Sh. wrote: > nit: 'FILE' not "FILE" Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:614: with open(opts.test_list, 'rb') as tl: On 2014/04/01 03:00:26, Vadim Sh. wrote: > What about '-' handling? Erp. good catch. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:620: del opts.test_list On 2014/04/01 03:00:26, Vadim Sh. wrote: > why? Don't really want anyone directly inspecting these. They should never need mode, and test_list should be indistinguishable from test_globs. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:631: 'name func args kwargs expectdir expectbase fmt breakpoints') On 2014/04/01 03:00:26, Vadim Sh. wrote: > nit: expect_dir, expect_base. Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:640: for f in (break_funcs or (func,)): On 2014/04/01 03:00:26, Vadim Sh. wrote: > Why 'or (func,)' here? Want it do default it to breakpointing on the user-supplied test function (user being the person using this test library). In concrete terms, this would be the RunRecipe function in the smoketest file. Otherwise we'll have nothing to break on at all :( https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:641: if hasattr(f, 'im_func'): On 2014/04/01 03:00:26, Vadim Sh. wrote: > What's im_func? I stole this from the pdb implementation, but it's for user defined methods (https://docs.python.org/2/reference/datamodel.html) https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:650: def expect_path(self, fmt=None): On 2014/04/01 03:00:26, Vadim Sh. wrote: > nit: fmt -> ext? Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:664: signal.signal(signal.SIGINT, lambda *_: kill_switch.set()) On 2014/04/01 03:00:26, Vadim Sh. wrote: > what about SIGTERM? Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:690: target=_GenLoopProcess, args=test_gen_args)] On 2014/04/01 03:00:26, Vadim Sh. wrote: > Maybe always run _GenLoopProcess in current process? It could take a while though, and we want to start consuming results as soon as we can. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:708: error |= result_handler(result_queue.get(timeout=0.1)) is False On 2014/04/01 03:00:26, Vadim Sh. wrote: > 'is False' is scetchy. Maybe 'is Failure' where Failure = object()? Unfortunately the result is coming across a pickle, so object-identity only works for False, True and None :/ https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:713: if not procs: On 2014/04/01 03:00:26, Vadim Sh. wrote: > nit: if not any(p.is_alive() for p in procs): Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/expect_tests.py:720: c.combine() On 2014/04/01 03:00:26, Vadim Sh. wrote: > How does it work? magic.... it looks for all files .coverage.hostname.pid.* and combines them together. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... File scripts/slave/unittests/recipe_simulation_test.py (right): https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/recipe_simulation_test.py:10: import test_env # pylint: disable=unused-import On 2014/04/01 03:00:26, Vadim Sh. wrote: > nit: from . import test_env? And same for expect_tests below. Can't when __name__ == __main__ > Also, are you importing test_env for side effects? Add a comment about that. Yeah, we are, that's all it's used for. Done. https://chromiumcodereview.appspot.com/220353003/diff/1/scripts/slave/unittes... scripts/slave/unittests/recipe_simulation_test.py:22: stream = annotator.StructuredAnnotationStream(stream=open(os.devnull, 'w')) On 2014/04/01 03:00:26, Vadim Sh. wrote: > Does os.devnull work on windows? yep. It's NUL
docstrings done ptal https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... File scripts/slave/unittests/expect_tests.py (right): https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:634: def __new__(cls, name, func, args=(), kwargs=None, expectdir=None, On 2014/04/01 03:00:26, Vadim Sh. wrote: > Doc strings for args. Esp. breakpoints and break_funcs. Done. https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:659: def main(test_gen, coverage_includes=None, coverage_omits=None, args=None): On 2014/04/01 03:00:26, Vadim Sh. wrote: > Doc string. Done.
https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... File scripts/slave/unittests/expect_tests.py (right): https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:69: lambda stream: yaml.load(stream, _YAMLSafeLoader), On 2014/04/01 03:45:14, iannucci wrote: > On 2014/04/01 03:00:26, Vadim Sh. wrote: > > why not just yaml.safe_load? > > Because last I checked (~December), it defaults to yaml.SafeLoader (slower) :-/ odd. So C extension is only usable if you are "advanced enough" to know about it? https://codereview.chromium.org/220353003/diff/1/scripts/slave/unittests/expe... scripts/slave/unittests/expect_tests.py:101: @staticmethod On 2014/04/01 03:45:14, iannucci wrote: > On 2014/04/01 03:00:26, Vadim Sh. wrote: > > @classmethod? because, well, it's classier? :) > > But it's not a classmethod... it's a static method in the _Handler class. You "override" this method below in _TrainHandler. In that sense it's more of a class method. That's what I meant. But whatever. https://codereview.chromium.org/220353003/diff/40001/scripts/slave/unittests/... File scripts/slave/unittests/expect_tests.py (right): https://codereview.chromium.org/220353003/diff/40001/scripts/slave/unittests/... scripts/slave/unittests/expect_tests.py:100: SKIP_RUNLOOP = False SKIP_RUNSTAGE? https://codereview.chromium.org/220353003/diff/40001/scripts/slave/unittests/... scripts/slave/unittests/expect_tests.py:109: class GenStage(object): It occurred to me after sending the previous review: why do you need these nested classes at all? Why not something like: class _Handler(...): def gen_stage(self, test, put_result): ... def run_stage(self, test, result, put_result): ... def result_stage(...): ... def handle(...): ... def handle_X(...): ... def finalize(...): ... What's wrong with that? I think it's less typing and less magic overall.
I think I'm missing a lot of the big picture here. I'm kinda surprised by the extent of the code you've had to write (4x the size of the old script?) I can maybe figure it all out from staring at this patch, but perhaps it's better (or faster) to at least go over this in a first pass in person?
On 2014/04/01 18:47:19, Dirk Pranke wrote: > I think I'm missing a lot of the big picture here. I'm kinda surprised by the > extent of the code you've had to write (4x the size of the old script?) > > I can maybe figure it all out from staring at this patch, but perhaps it's > better (or faster) to at least go over this in a first pass in person? It got big because it's replacing unittest entirely. I'd be happy to go over it in person.
PTAL (refactored lotsmany)
https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/cover.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/cover.py:69: def create_subprocess_context(self): @contextlib.contextmanager def create_subprocess_context(self): if not self.enabled: yield else: c = coverage.coverage(**self.opts) ... ? I think it's shorter and cleaner overall. https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/main.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/main.py:36: sp = subparsers.add_parser(k, help=h.__doc__.lower()) lower only first letter? :) https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/pipeline.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/pipeline.py:112: signal.signal(signal.SIGINT, signal.SIG_DFL) why setting to DFL? To die on double Ctrl+C? https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/pipeline.py:127: """It's alive exactly once.""" ugh... https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/serialize.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/serialize.py:94: remove trailing empty line :) https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/types.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:18: class Test(_Test): newline or class Test( namedtuple('Test', '.............')): .... https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:68: Defines 3 nested classes for each stage of the test pipeline. The pipeline Stale doc string? https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:128: error |= handler(obj) is False 'is False' still bugs me. What about: import pickle class _Failure(object): def __eq__(self, other): return isinstance(other, _Failure) Failure = _Failure() assert Failure == pickle.loads(pickle.dumps(Failure)) 'is' won't work in that case though, but == would. Not sure... https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:137: So if |obj| is a Test, this would call self.Test(obj). I'd prefer explicit prefix 'handle_' as it was before. https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/recipe_simulation_test.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/recipe_simulation_test.py:22: def RunRecipe(test_data): Let's use lower_case_with_underscore everywhere in recipe engine? It's messy now. https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... File scripts/slave/unittests/expect_tests/pipeline.py (right): https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:124: if opts.handler.SKIP_RUNLOOP: if opts.handler.run_stage_loop is pipeline.Handler.run_stage_loop: ... Or too silly?
addressed https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/cover.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/cover.py:69: def create_subprocess_context(self): On 2014/04/02 22:37:59, Vadim Sh. wrote: > @contextlib.contextmanager > def create_subprocess_context(self): > if not self.enabled: > yield > else: > c = coverage.coverage(**self.opts) > ... > > ? > > I think it's shorter and cleaner overall. added comment and merged _cover and _nop_cover https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/main.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/main.py:36: sp = subparsers.add_parser(k, help=h.__doc__.lower()) On 2014/04/02 22:37:59, Vadim Sh. wrote: > lower only first letter? :) rrrrrr :p. Done https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/pipeline.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/pipeline.py:112: signal.signal(signal.SIGINT, signal.SIG_DFL) On 2014/04/02 22:37:59, Vadim Sh. wrote: > why setting to DFL? To die on double Ctrl+C? Yeah, added comment. https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/pipeline.py:127: """It's alive exactly once.""" On 2014/04/02 22:37:59, Vadim Sh. wrote: > ugh... Otherwise the generate_objects loop was racy :(. There's probably a refactor, but I can't think of it at the moment. https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... File scripts/slave/unittests/expect_tests/types.py (right): https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:18: class Test(_Test): On 2014/04/02 22:37:59, Vadim Sh. wrote: > newline > > or > class Test( > namedtuple('Test', '.............')): > .... opted for newline... it was too weird. https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:68: Defines 3 nested classes for each stage of the test pipeline. The pipeline On 2014/04/02 22:37:59, Vadim Sh. wrote: > Stale doc string? Done. https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:128: error |= handler(obj) is False On 2014/04/02 22:37:59, Vadim Sh. wrote: > 'is False' still bugs me. > > What about: > > import pickle > > class _Failure(object): > def __eq__(self, other): > return isinstance(other, _Failure) > Failure = _Failure() > > assert Failure == pickle.loads(pickle.dumps(Failure)) > > 'is' won't work in that case though, but == would. > > Not sure... Did a simpler version of this https://chromiumcodereview.appspot.com/220353003/diff/80001/scripts/slave/uni... scripts/slave/unittests/expect_tests/types.py:137: So if |obj| is a Test, this would call self.Test(obj). On 2014/04/02 22:37:59, Vadim Sh. wrote: > I'd prefer explicit prefix 'handle_' as it was before. Ok. Done. https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... File scripts/slave/unittests/expect_tests/pipeline.py (right): https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:124: if opts.handler.SKIP_RUNLOOP: On 2014/04/02 22:37:59, Vadim Sh. wrote: > if opts.handler.run_stage_loop is pipeline.Handler.run_stage_loop: > ... > > Or too silly? Too silly :)
Non-exhaustive comments, mostly nits. https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... File scripts/slave/unittests/expect_tests/cover.py (right): https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... scripts/slave/unittests/expect_tests/cover.py:19: #not hasattr(coverage.collector, 'CTracer') or Remove commented out lines https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... File scripts/slave/unittests/expect_tests/handle_test.py (right): https://chromiumcodereview.appspot.com/220353003/diff/120001/scripts/slave/un... scripts/slave/unittests/expect_tests/handle_test.py:43: def _emit(self, short, test, verbose): This is a really weird input argument order. https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... File scripts/slave/unittests/expect_tests/pipeline.py (right): https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:26: @type match_globs: [str] no longer used https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:28: @type handler: types.Handler no longer used https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... File scripts/slave/unittests/recipe_simulation_test.py (right): https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/recipe_simulation_test.py:6: """Provides simulator test coverage for individual recipes.""" Make sure you update recipes docs (both gdoc design doc and README.recipes) to point at this new script for manual training. https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/recipe_simulation_test.py:49: recipe.GenSteps) Move up a line.
https://codereview.chromium.org/220353003/diff/140001/scripts/slave/unittests... File scripts/slave/unittests/expect_tests/pipeline.py (right): https://codereview.chromium.org/220353003/diff/140001/scripts/slave/unittests... scripts/slave/unittests/expect_tests/pipeline.py:14: def GenLoopProcess(gen, test_queue, result_queue, opts, kill_switch, cover_ctx): gen_loop_process, same for other functions
ptal https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... File scripts/slave/unittests/expect_tests/pipeline.py (right): https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:14: def GenLoopProcess(gen, test_queue, result_queue, opts, kill_switch, cover_ctx): On 2014/04/03 01:00:30, Vadim Sh. wrote: > gen_loop_process, same for other functions Done. https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:26: @type match_globs: [str] On 2014/04/03 00:38:00, agable wrote: > no longer used Done. https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:28: @type handler: types.Handler On 2014/04/03 00:38:00, agable wrote: > no longer used Done. https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/expect_tests/pipeline.py:162: break never mind, this loop is still racy. I'll fix it. Done https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... File scripts/slave/unittests/recipe_simulation_test.py (right): https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/recipe_simulation_test.py:6: """Provides simulator test coverage for individual recipes.""" On 2014/04/03 00:38:00, agable wrote: > Make sure you update recipes docs (both gdoc design doc and README.recipes) to > point at this new script for manual training. Done. https://chromiumcodereview.appspot.com/220353003/diff/140001/scripts/slave/un... scripts/slave/unittests/recipe_simulation_test.py:49: recipe.GenSteps) On 2014/04/03 00:38:00, agable wrote: > Move up a line. Done.
lgtm Does it gets called by PRESUBMIT.py? https://chromiumcodereview.appspot.com/220353003/diff/160001/scripts/slave/RE... File scripts/slave/README.recipes.md (right): https://chromiumcodereview.appspot.com/220353003/diff/160001/scripts/slave/RE... scripts/slave/README.recipes.md:615: To test all the recipes/apis, use `slave/unittests/recipes_test.py`. To set new 'use `slave/unittests/recipes_test.py`' Huh? Is it correct?
The CQ bit was checked by iannucci@chromium.org
https://chromiumcodereview.appspot.com/220353003/diff/160001/scripts/slave/RE... File scripts/slave/README.recipes.md (right): https://chromiumcodereview.appspot.com/220353003/diff/160001/scripts/slave/RE... scripts/slave/README.recipes.md:615: To test all the recipes/apis, use `slave/unittests/recipes_test.py`. To set new On 2014/04/03 02:42:09, Vadim Sh. wrote: > 'use `slave/unittests/recipes_test.py`' Huh? Is it correct? oops :D
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/220353003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 220353003-180001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Pylint (458 files) (46.11s) failed ************* Module /b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/cover.py E0012: 31,0: Bad option value 'protected-access' E0012: 31,0: Bad option value 'protected-access' ************* Module /b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/serialize.py F0401: 11,2: Unable to import 'yaml' ************* Module /b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/types.py W0232: 24,0:Test: Class has no __init__ method E1002: 25,2:Test.__new__: Use of super on an old style class ************* Module /b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/recipe_simulation_test.py E0012: 11,0: Bad option value 'unused-import' E0012: 11,0: Bad option value 'unused-import' W0611: 11,0: Unused import test_env scripts/slave/unittests/recipe_simulation_test.py (9.49s) failed .......................................................................................................................................................................................................................................................................................................................................................................................... ---------------------------------------------------------------------- Ran 378 tests in 4.573s OK Coverage Report Stmts Miss Cover Missing -------------------------------------------------------------------------------------- scripts/slave/recipe_modules/chromium_android/test_api 6 1 83% 13 scripts/slave/recipe_modules/generator_script/test_api 5 2 60% 5-6 scripts/slave/recipe_modules/gpu/test_api 5 1 80% 12 scripts/slave/recipe_modules/json/test_api 41 18 56% 16, 31-52 scripts/slave/recipe_modules/path/test_api 9 3 67% 8-9, 12 scripts/slave/recipe_modules/platform/test_api 12 5 58% 7-8, 13-14, 17 scripts/slave/recipe_modules/properties/test_api 22 15 32% 5-7, 13-23, 30-37, 44-51, 58-70 scripts/slave/recipe_modules/time/test_api 8 2 75% 11, 16 scripts/slave/recipes/android/android_builder 41 11 73% 58-86 scripts/slave/recipes/android/instrumentation_tests 31 18 42% 25-69 scripts/slave/recipes/android_webview_aosp 21 7 67% 59-89 scripts/slave/recipes/blink_trybot 92 27 71% 432, 436-533 scripts/slave/recipes/chromium 129 11 91% 1523, 1527-1553 scripts/slave/recipes/chromium_gn 25 5 80% 67-97 scripts/slave/recipes/chromium_trybot 163 25 85% 385-524 scripts/slave/recipes/codesearch 19 1 95% 45 scripts/slave/recipes/cros_trybot 45 13 71% 118-164 scripts/slave/recipes/dart/dartium 49 4 92% 130-133 scripts/slave/recipes/gatekeeper 6 1 83% 18 scripts/slave/recipes/gatekeeper-failure 8 2 75% 17-18 scripts/slave/recipes/gpu/build_and_test 22 11 50% 40-108 scripts/slave/recipes/gpu/build_and_upload 13 5 62% 30-45 scripts/slave/recipes/gpu/download_and_test 20 7 65% 56-83 scripts/slave/recipes/libvpx/android_unittests 27 1 96% 107 scripts/slave/recipes/oilpan 39 2 95% 123-124 scripts/slave/recipes/ozone_ecs 24 2 92% 101-102 scripts/slave/recipes/perf/telemetry_harness_upload 13 1 92% 39 scripts/slave/recipes/polymer 42 3 93% 108-120 scripts/slave/recipes/run_presubmit 25 6 76% 65-76 scripts/slave/recipes/skia/build_and_test 10 3 70% 23-34 scripts/slave/recipes/v8 92 11 88% 712, 716-740 scripts/slave/recipes/webrtc/android_apk 26 6 77% 54-83 scripts/slave/recipes/webrtc/standalone 63 13 79% 624, 628-645 -------------------------------------------------------------------------------------- TOTAL 2874 243 92% FATAL: Test coverage is not at 100%. Presubmit checks took 47.4s to calculate.
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/220353003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 220353003-200001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Pylint (457 files) (44.83s) failed
************* Module
/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/__init__.py
F0401: 6,0: Unable to import 'type_definitions'
************* Module
/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/handle_debug.py
F0401: 8,0: Unable to import 'type_definitions'
W0232: 11,0:DebugHandler: Class has no __init__ method
W0232: 15,2:DebugHandler.ResultStageHandler: Class has no __init__ method
************* Module
/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/handle_list.py
F0401: 5,0: Unable to import 'type_definitions'
W0232: 8,0:ListHandler: Class has no __init__ method
W0232: 12,2:ListHandler.ResultStageHandler: Class has no __init__ method
************* Module
/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/handle_test.py
F0401: 11,0: Unable to import 'type_definitions'
W0232: 20,0:TestHandler: Class has no __init__ method
E1002: 36,4:TestHandler.ResultStageHandler.__init__: Use of super on an old
style class
************* Module
/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/handle_train.py
F0401: 10,0: Unable to import 'type_definitions'
W0232: 22,0:TrainHandler: Class has no __init__ method
E1002: 67,4:TrainHandler.ResultStageHandler.__init__: Use of super on an old
style class
************* Module
/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/pipeline.py
F0401: 11,0: Unable to import 'type_definitions'
************* Module
/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/serialize.py
F0401: 11,2: Unable to import 'yaml'
scripts/slave/unittests/recipe_simulation_test.py (0.77s) failed
Traceback (most recent call last):
File "scripts/slave/unittests/recipe_simulation_test.py", line 19, in <module>
import expect_tests
File
"/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/__init__.py",
line 5, in <module>
from .main import main
File
"/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/main.py",
line 11, in <module>
from . import handle_list, handle_debug, handle_train, handle_test
File
"/b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/handle_list.py",
line 5, in <module>
from .type_definitions import Handler
ImportError: No module named type_definitions
Presubmit checks took 46.0s to calculate.
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/220353003/120002
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 220353003-120002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Pylint (458 files) (48.08s) failed ************* Module /b/infra_internal/commit_queue/workdir/tools/build/scripts/slave/unittests/expect_tests/serialize.py F0401: 11,2: Unable to import 'yaml' scripts/slave/unittests/recipe_simulation_test.py (8.35s) failed .......................................................................................................................................................................................................................................................................................................................................................................................... ---------------------------------------------------------------------- Ran 378 tests in 5.041s OK Coverage Report Stmts Miss Cover Missing -------------------------------------------------------------------------------------- scripts/slave/recipe_modules/chromium_android/test_api 6 1 83% 13 scripts/slave/recipe_modules/generator_script/test_api 5 2 60% 5-6 scripts/slave/recipe_modules/gpu/test_api 5 1 80% 12 scripts/slave/recipe_modules/json/test_api 41 18 56% 16, 31-52 scripts/slave/recipe_modules/path/test_api 9 3 67% 8-9, 12 scripts/slave/recipe_modules/platform/test_api 12 5 58% 7-8, 13-14, 17 scripts/slave/recipe_modules/properties/test_api 22 15 32% 5-7, 13-23, 30-37, 44-51, 58-70 scripts/slave/recipe_modules/time/test_api 8 2 75% 11, 16 scripts/slave/recipes/android/android_builder 41 11 73% 58-86 scripts/slave/recipes/android/instrumentation_tests 31 18 42% 25-69 scripts/slave/recipes/android_webview_aosp 21 7 67% 59-89 scripts/slave/recipes/blink_trybot 92 27 71% 432, 436-533 scripts/slave/recipes/chromium 129 11 91% 1523, 1527-1553 scripts/slave/recipes/chromium_gn 25 5 80% 67-97 scripts/slave/recipes/chromium_trybot 163 25 85% 385-524 scripts/slave/recipes/codesearch 19 1 95% 45 scripts/slave/recipes/cros_trybot 45 13 71% 118-164 scripts/slave/recipes/dart/dartium 49 4 92% 130-133 scripts/slave/recipes/gatekeeper 6 1 83% 18 scripts/slave/recipes/gatekeeper-failure 8 2 75% 17-18 scripts/slave/recipes/gpu/build_and_test 22 11 50% 40-108 scripts/slave/recipes/gpu/build_and_upload 13 5 62% 30-45 scripts/slave/recipes/gpu/download_and_test 20 7 65% 56-83 scripts/slave/recipes/libvpx/android_unittests 27 1 96% 107 scripts/slave/recipes/oilpan 39 2 95% 123-124 scripts/slave/recipes/ozone_ecs 24 2 92% 101-102 scripts/slave/recipes/perf/telemetry_harness_upload 13 1 92% 39 scripts/slave/recipes/polymer 42 3 93% 108-120 scripts/slave/recipes/run_presubmit 25 6 76% 65-76 scripts/slave/recipes/skia/build_and_test 10 3 70% 23-34 scripts/slave/recipes/v8 92 11 88% 712, 716-740 scripts/slave/recipes/webrtc/android_apk 26 6 77% 54-83 scripts/slave/recipes/webrtc/standalone 63 13 79% 624, 628-645 -------------------------------------------------------------------------------------- TOTAL 2874 243 92% FATAL: Test coverage is not at 100%. Presubmit checks took 49.4s to calculate.
Curious... when I run `git cl presubmit` on cq.golo for the build repo with this patch, it's A.O.K.
On 2014/04/03 05:19:21, iannucci wrote: > Curious... when I run `git cl presubmit` on cq.golo for the build repo with this > patch, it's A.O.K. Correction: when I run the test directly with the patch, it's fine, but when I run under git cl presubmit, it fails. But not on my linux desktop.
Ah... I think recipe_configs_test is racing with this test, and the coverage file sets are getting conflated
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iannucci@chromium.org/220353003/240001
Message was sent while issue was closed.
Change committed as 261284
Message was sent while issue was closed.
Hey I just noticed this CL, and when I run the new script I'm getting an error saying I need python-coverage >= 3.7. Goobuntu is currently on 3.4. Any advice on how to upgrade? Is 3.7 strictly required?
Message was sent while issue was closed.
On 2014/04/04 13:27:02, Kevin Graney wrote: > Hey I just noticed this CL, and when I run the new script I'm getting an error > saying I need python-coverage >= 3.7. Goobuntu is currently on 3.4. Any advice > on how to upgrade? Is 3.7 strictly required? Yes, 3.7 is strictly required (lower version numbers and version not backed by the C library have serious bugs). The correct solution is 'sudo pip install coverage'.
Message was sent while issue was closed.
On 2014/04/04 16:07:49, agable wrote: > The correct solution is 'sudo pip install coverage'. Requiring people to manually install packages to get our tests and presubmit checks to work is bad; we should avoid this if at all possible. In particular, we have a version of coverage in depot_tools/third_party. We should update that if possible and auto-add that to the sys.path for this. If we can't do that, we need to re-evaluate how this works. Perhaps we need to add a general install/upgrade script as part of downloading and installing depot_tools, or the build repo. -- Dirk
Message was sent while issue was closed.
On 2014/04/04 16:18:03, Dirk Pranke wrote: > On 2014/04/04 16:07:49, agable wrote: > > The correct solution is 'sudo pip install coverage'. > > Requiring people to manually install packages to get our tests and presubmit > checks to work is bad; we should avoid this if at all possible. > > In particular, we have a version of coverage in depot_tools/third_party. We > should update that if possible and auto-add that to the sys.path for this. If we > can't do that, we need to re-evaluate how this works. Perhaps we need to add a > general install/upgrade script as part of downloading and installing > depot_tools, or the build repo. > > -- Dirk We can't put it in third_party because the required version of coverage has a compiled C component. A working version of coverage should be installed by the virtualenv work that other members of the infra team are doing right now.
Message was sent while issue was closed.
On 2014/04/04 16:44:36, agable wrote: > On 2014/04/04 16:18:03, Dirk Pranke wrote: > > On 2014/04/04 16:07:49, agable wrote: > > > The correct solution is 'sudo pip install coverage'. > > > > Requiring people to manually install packages to get our tests and presubmit > > checks to work is bad; we should avoid this if at all possible. > > > > In particular, we have a version of coverage in depot_tools/third_party. We > > should update that if possible and auto-add that to the sys.path for this. If > we > > can't do that, we need to re-evaluate how this works. Perhaps we need to add a > > general install/upgrade script as part of downloading and installing > > depot_tools, or the build repo. > > > > -- Dirk > > We can't put it in third_party because the required version of coverage has a > compiled C component. A working version of coverage should be installed by the > virtualenv work that other members of the infra team are doing right now. Okay. I'm clearly not up on what all was buggy in 3.6 that needed the 3.7 upgrade -- iannucci linked to some things but I haven't had a chance to read them yet -- but I am troubled by this. Running the coverage is on by default, right, as is running in parallel? This means that if you don't have 3.7 installed, you can't run the tests properly? If all of this is correct, this patch shouldn't have landed as-is. We shouldn't default into a state that might be broken; either the code should auto-detect the version of coverage and default to serial if it's not new enough, or default to serial period, or we should have a way to auto-install the right version on demand. In addition, requiring a different version than what is in depot_tools is bad. We shouldn't be introducing version skew across our projects like this unless it is absolutely necessary. This isn't.
Message was sent while issue was closed.
On 2014/04/04 16:51:43, Dirk Pranke wrote: > On 2014/04/04 16:44:36, agable wrote: > > On 2014/04/04 16:18:03, Dirk Pranke wrote: > > > On 2014/04/04 16:07:49, agable wrote: > > > > The correct solution is 'sudo pip install coverage'. > > > > > > Requiring people to manually install packages to get our tests and presubmit > > > checks to work is bad; we should avoid this if at all possible. > > > > > > In particular, we have a version of coverage in depot_tools/third_party. We > > > should update that if possible and auto-add that to the sys.path for this. > If > > we > > > can't do that, we need to re-evaluate how this works. Perhaps we need to add > a > > > general install/upgrade script as part of downloading and installing > > > depot_tools, or the build repo. > > > > > > -- Dirk > > > > We can't put it in third_party because the required version of coverage has a > > compiled C component. A working version of coverage should be installed by the > > virtualenv work that other members of the infra team are doing right now. > > Okay. I'm clearly not up on what all was buggy in 3.6 that needed the 3.7 > upgrade -- iannucci linked to some things but I haven't had a chance to read > them yet -- but I am troubled by this. > > Running the coverage is on by default, right, as is running in parallel? This > means that if you don't have 3.7 installed, you can't run the tests properly? > > If all of this is correct, this patch shouldn't have landed as-is. We shouldn't > default into a state that might be broken; either the code should auto-detect > the version of coverage and default to serial if it's not new enough, or default > to serial period, or we should have a way to auto-install the right version on > demand. > It does auto detect, but it doesn't auto install. I'll work on that. > In addition, requiring a different version than what is in depot_tools is bad. > We shouldn't be introducing version skew across our projects like this unless it > is absolutely necessary. This isn't. The version in depot_tools is 3.7, but it doesn't have the CTracer.
Message was sent while issue was closed.
On 2014/04/04 17:15:13, iannucci wrote: > On 2014/04/04 16:51:43, Dirk Pranke wrote: > > On 2014/04/04 16:44:36, agable wrote: > > > On 2014/04/04 16:18:03, Dirk Pranke wrote: > > > > On 2014/04/04 16:07:49, agable wrote: > > > > > The correct solution is 'sudo pip install coverage'. > > > > > > > > Requiring people to manually install packages to get our tests and > presubmit > > > > checks to work is bad; we should avoid this if at all possible. > > > > > > > > In particular, we have a version of coverage in depot_tools/third_party. > We > > > > should update that if possible and auto-add that to the sys.path for this. > > If > > > we > > > > can't do that, we need to re-evaluate how this works. Perhaps we need to > add > > a > > > > general install/upgrade script as part of downloading and installing > > > > depot_tools, or the build repo. > > > > > > > > -- Dirk > > > > > > We can't put it in third_party because the required version of coverage has > a > > > compiled C component. A working version of coverage should be installed by > the > > > virtualenv work that other members of the infra team are doing right now. > > > > Okay. I'm clearly not up on what all was buggy in 3.6 that needed the 3.7 > > upgrade -- iannucci linked to some things but I haven't had a chance to read > > them yet -- but I am troubled by this. > > > > Running the coverage is on by default, right, as is running in parallel? This > > means that if you don't have 3.7 installed, you can't run the tests properly? > > > > If all of this is correct, this patch shouldn't have landed as-is. We > shouldn't > > default into a state that might be broken; either the code should auto-detect > > the version of coverage and default to serial if it's not new enough, or > default > > to serial period, or we should have a way to auto-install the right version on > > demand. > > > > It does auto detect, but it doesn't auto install. I'll work on that. > > > In addition, requiring a different version than what is in depot_tools is bad. > > We shouldn't be introducing version skew across our projects like this unless > it > > is absolutely necessary. This isn't. > > The version in depot_tools is 3.7, but it doesn't have the CTracer. (CL to fix is posted here: https://codereview.chromium.org/225633007/) |
