----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51715/#review149005 -----------------------------------------------------------
support/mesos-gtest-runner.py (line 13) <https://reviews.apache.org/r/51715/#comment216532> Can we add some comment (or docstring) at the top here that actually describe how to invoke this script? I know other scripts don't necessarily do this, but it would be really nice to open this up and immediately see how it should be used. support/mesos-gtest-runner.py (lines 15 - 29) <https://reviews.apache.org/r/51715/#comment216544> I'd prefer to see each of the variables in this class not include the whole `'if sys.stdout.isatty()'` suffix and instead provide a `COLORIZE` helper that does that instead. i.e. ``` class Bcolors: HEADER = '\033[95m' OKBLUE = '\033[94m' OKGREEN = '\033[92m' WARNING = '\033[93m' FAIL = '\033[91m' BOLD = '\033[1m' UNDERLINE = '\033[4m' ENDC = '\033[0m' @staticmethod def COLORIZE(string, color_codes): colors = "".join(color_codes) return '{}{}{}'.format(colors if sys.stdout.isatty() else '', string, ENDC if sys.stdout.isatty() else '') ``` This would then be used below as e.g.: ``` Bcolors.COLORIZE('[FAIL]', [Bcolors.FAIL, Bcolors.BOLD]) ``` instead of ``` Bcolors.FAIL + Bcolors.BOLD + '[FAIL]' + Bcolors.ENDC ``` support/mesos-gtest-runner.py (line 32) <https://reviews.apache.org/r/51715/#comment216539> This function name doesn't tell me anything. If it is intended to run an executable, why not call it `run_executable()` or better `run_test()`? support/mesos-gtest-runner.py (line 34) <https://reviews.apache.org/r/51715/#comment216540> s/acutal/actual support/mesos-gtest-runner.py (lines 36 - 37) <https://reviews.apache.org/r/51715/#comment216541> What is a shard in this context? support/mesos-gtest-runner.py (line 63) <https://reviews.apache.org/r/51715/#comment216545> I would prefer to see this function inline under the `__main__` directive below, and instead have a helper function for parsing the args instead of inlining that content as done below. support/mesos-gtest-runner.py (line 67) <https://reviews.apache.org/r/51715/#comment216542> Here you call it a 'binary', whereas above you call it an 'executable'. We should probably be consistent (and I prefer 'exectuable' because it may be a script and not necessarily an actual binary file). support/mesos-gtest-runner.py (line 70) <https://reviews.apache.org/r/51715/#comment216546> At first glance it's not clear to me what this function is used for. Can you add a more descriptive comment about what this is used for? i.e. explain how this is passed to each subcommand that gets spawned asynchronously in order to pass it the proper options support/mesos-gtest-runner.py (lines 122 - 123) <https://reviews.apache.org/r/51715/#comment216547> Sometimes you use `.format()` sometimes you use `"" + ""`. Can we be more consistent? I prefer `.format()`. support/mesos-gtest-runner.py (lines 131 - 147) <https://reviews.apache.org/r/51715/#comment216548> In the first exception you print then terminate, then exit. In the second exception you terminate then print then exit. Is there a reason for the inconsistency? Could we do the operations in the same order for more consistency? support/mesos-gtest-runner.py (line 151) <https://reviews.apache.org/r/51715/#comment216549> As mentioned above, I would move all of this stuff into a `parse_arguments()` helper function instead of putting the main stuff in a helper function as is currently done. support/mesos-gtest-runner.py (line 154) <https://reviews.apache.org/r/51715/#comment216534> How did you come up with this default number of jobs? I would think we should spawn slightly *less* jobs than the number of CPUs on the machine (not 1.5 times more). Also it's not a big deal since we only have one constant for now, but I would prefer to move this line to the top of the file instead of in line here (e.g. similar to how we keep all constants at the top of the file in support/generate-endpoint-help.py). If we keep it here, I would add a newline after it. support/mesos-gtest-runner.py (line 178) <https://reviews.apache.org/r/51715/#comment216543> I typically prefer named format parameters over unnamed ones. If we use the COLORIZE helper as suggested above, we can wrap this string in the COLORIZE call, and then use a named parameter inline here (as well as all instances below). - Kevin Klues On Sept. 14, 2016, 12:02 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51715/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2016, 12:02 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 > >
