> On Sept. 21, 2016, 8:53 p.m., Kevin Klues wrote: > > support/mesos-gtest-runner.py, line 214 > > <https://reviews.apache.org/r/51715/diff/2/?file=1502617#file1502617line214> > > > > Can you add a quick comment about how this filter is working? It's not > > obvious at first glance what it's doing. > > > > Also, do we have to do this here, or can we do it below just before we > > iterate over it.
The form of this code from changed from python3 to python2 style, but is overly obscure. I replaced this with a list comprehension and also added a comment. > On Sept. 21, 2016, 8:53 p.m., Kevin Klues wrote: > > support/mesos-gtest-runner.py, lines 187-191 > > <https://reviews.apache.org/r/51715/diff/2/?file=1502617#file1502617line187> > > > > Can we cahnge the name of this variable to OPTIONS.parallel to be > > symmetrical with OPTIONS.sequential? > > > > Can we also move this into the parse_arguments() function? I know it's > > not technically part of the arguments, but it relies on the arguments and > > seems to make more sense up there. Makes sense, especially since it allows us to keep mutations of `OPTIONS` in one place. > On Sept. 21, 2016, 8:53 p.m., Kevin Klues wrote: > > support/mesos-gtest-runner.py, lines 194-202 > > <https://reviews.apache.org/r/51715/diff/2/?file=1502617#file1502617line194> > > > > Can you remove the line break between the comment and the code and > > reformat the comment as below? Also, given the feedback from above, the > > code itself can change to just take jobs/filter explicitly. > > > > ``` > > POOL = multiprocessing.Pool(processes=OPTIONS.jobs) > > > > # Run the parallel tests > > # > > # NOTE: Multiprocessing's `map` cannot properly handle > > `KeyboardInterrupt` in > > # some python versions. Use `map_async` with an explicit timeout > > # instead. See http://stackoverflow.com/a/1408476. > > RESULTS.extend( > > POOL.map_async( > > run_test, > > options_gen(OPTIONS.jobs, OPTIONS.parallel, > > EXECUTABLE)).get(timeout=sys.maxint)) > > ``` Done. I also changed to ordering of parameters to `options_gen`. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51715/#review149849 ----------------------------------------------------------- On Oct. 1, 2016, 9:06 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51715/ > ----------------------------------------------------------- > > (Updated Oct. 1, 2016, 9:06 p.m.) > > > Review request for mesos, Kevin Klues and Till Toenshoff. > > > Bugs: MESOS-6140 > https://issues.apache.org/jira/browse/MESOS-6140 > > > Repository: mesos > > > Description > ------- > > This adds `b814987b28cfb65a7c9635c83399545e423e690a` of > https://github.com/bbannier/gtest-shrdr. > > > Diffs > ----- > > support/mesos-gtest-runner.py PRE-CREATION > > Diff: https://reviews.apache.org/r/51715/diff/ > > > Testing > ------- > > Tested with e.g., `stout-tests` > > $ ./support/mesos-gtest-runner.py build/3rdparty/stout/stout-tests > ............ > [PASS] > > > Thanks, > > Benjamin Bannier > >
