mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 51715: Added a parallel gtest runner.
Date Thu, 15 Sep 2016 01:16:17 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message