-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51715/#review149849
-----------------------------------------------------------



Looking good! Just a few more comments.


support/mesos-gtest-runner.py (line 7)
<https://reviews.apache.org/r/51715/#comment217571>

    s/allows to execute/allows one to execute/



support/mesos-gtest-runner.py (line 8)
<https://reviews.apache.org/r/51715/#comment217569>

    s/in-built/built-in/



support/mesos-gtest-runner.py (line 111)
<https://reviews.apache.org/r/51715/#comment217611>

    For consistency, I'd either add a comment above each of the next three if 
statements explaining what they are validating, or remove the comments on the 
last two. It's up to you since the error strings they print make it obvious 
what they are validating.



support/mesos-gtest-runner.py (line 161)
<https://reviews.apache.org/r/51715/#comment217624>

    Can we ahve this take `jobs` and `filter` explicitly instead of just 
`options`? The reason should be clearer when you read the comments below.



support/mesos-gtest-runner.py (line 164)
<https://reviews.apache.org/r/51715/#comment217619>

    s/distint/distinct



support/mesos-gtest-runner.py (line 169)
<https://reviews.apache.org/r/51715/#comment217620>

    s/terminal enable/terminal, enable/



support/mesos-gtest-runner.py (line 170)
<https://reviews.apache.org/r/51715/#comment217621>

    s/themself/themselves



support/mesos-gtest-runner.py (lines 187 - 191)
<https://reviews.apache.org/r/51715/#comment217623>

    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.



support/mesos-gtest-runner.py (lines 194 - 202)
<https://reviews.apache.org/r/51715/#comment217618>

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



support/mesos-gtest-runner.py (lines 205 - 212)
<https://reviews.apache.org/r/51715/#comment217626>

    Given the comments above, this would just become:
    ```
    RESULTS.extend(
        POOL.map_async(
            run_test,
            options_gen(1, OPTIONS.sequential, 
EXECUTABLE)).get(timeout=sys.maxint))
    ```



support/mesos-gtest-runner.py (line 214)
<https://reviews.apache.org/r/51715/#comment217627>

    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.



support/mesos-gtest-runner.py (line 241)
<https://reviews.apache.org/r/51715/#comment217613>

    Can you add a new line here?


- Kevin Klues


On Sept. 19, 2016, 2:35 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51715/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2016, 2:35 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
> 
>

Reply via email to