----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51715/#review151118 -----------------------------------------------------------
Fix it, then Ship it! The code itself looks great! Just a few suggestions about the comments in the code. If I were a committer I would just patch those up myself before committing. Since I'm not, I'll leave that to either you or Till to patch up. support/mesos-gtest-runner.py (lines 5 - 12) <https://reviews.apache.org/r/51715/#comment219254> I would consider rewording this slightly, as it's not exactly clear to me what the difference between a GoogleTest suite and a GoogleTest test is. I'm also fine with leaving it as is though. support/mesos-gtest-runner.py (lines 190 - 200) <https://reviews.apache.org/r/51715/#comment219250> I would cahnge the comment as follows: ``` 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( EXECUTABLE, OPTIONS.parallel, OPTIONS.jobs)).get( timeout=sys.maxint)) ``` support/mesos-gtest-runner.py (lines 211 - 229) <https://reviews.apache.org/r/51715/#comment219253> I would reword the comment below as: ``` # Grab the number of failed tests and print the results from each test run. # # NOTE: The RESULTS array stores the results from each `run_test()` # invocation as a tuple (success, output). NFAILED = len([success for success, __ in RESULTS if not success]) for result in RESULTS: if not result[0]: if OPTIONS.verbosity > 0: print(result[1], file=sys.stderr) else: if OPTIONS.verbosity > 1: print(result[1], file=sys.stdout) if NFAILED > 0: print(Bcolors.colorize('\n[FAIL]', Bcolors.FAIL, Bcolors.BOLD), file=sys.stderr) else: print(Bcolors.colorize('\n[PASS]', Bcolors.OKGREEN, Bcolors.BOLD)) sys.exit(NFAILED) ``` support/mesos-gtest-runner.py (line 223) <https://reviews.apache.org/r/51715/#comment219251> Can we be explicit about `NFAILED > 0`? - Kevin Klues On Oct. 1, 2016, 10:54 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51715/ > ----------------------------------------------------------- > > (Updated Oct. 1, 2016, 10:54 a.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 > >
