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 Sat, 01 Oct 2016 17:43:00 GMT

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


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