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 Wed, 21 Sep 2016 18:53:41 GMT

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


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