mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 51715: Added a parallel gtest runner.
Date Sat, 01 Oct 2016 23:58:50 GMT


> On Sept. 21, 2016, 8:53 p.m., Kevin Klues wrote:
> > support/mesos-gtest-runner.py, line 214
> > <https://reviews.apache.org/r/51715/diff/2/?file=1502617#file1502617line214>
> >
> >     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.

The form of this code from changed from python3 to python2 style, but is overly obscure. I
replaced this with a list comprehension and also added a comment.


> On Sept. 21, 2016, 8:53 p.m., Kevin Klues wrote:
> > support/mesos-gtest-runner.py, lines 187-191
> > <https://reviews.apache.org/r/51715/diff/2/?file=1502617#file1502617line187>
> >
> >     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.

Makes sense, especially since it allows us to keep mutations of `OPTIONS` in one place.


> On Sept. 21, 2016, 8:53 p.m., Kevin Klues wrote:
> > support/mesos-gtest-runner.py, lines 194-202
> > <https://reviews.apache.org/r/51715/diff/2/?file=1502617#file1502617line194>
> >
> >     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))
> >     ```

Done. I also changed to ordering of parameters to `options_gen`.


- Benjamin


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


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