You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2015/12/04 19:49:58 UTC
Review Request 40978: Modify the Http test process in FetcherTests to
be symmetric in spawn/terminate.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40978/
-----------------------------------------------------------
Review request for mesos, Bernd Mathiske, Ben Mahler, and Artem Harutyunyan.
Repository: mesos
Description
-------
This addresses the comment here: https://reviews.apache.org/r/40501/#comment168391
Diffs
-----
src/tests/fetcher_tests.cpp 069c9bc6bc54d5bc1275c55e329457651d3c7b71
Diff: https://reviews.apache.org/r/40978/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 40978: Modify the Http test process in
FetcherTests to be symmetric in spawn/terminate.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40978/#review109021
-----------------------------------------------------------
Patch looks great!
Reviews applied: [40978]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 4, 2015, 6:49 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40978/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 6:49 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Artem Harutyunyan.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the comment here: https://reviews.apache.org/r/40501/#comment168391
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 069c9bc6bc54d5bc1275c55e329457651d3c7b71
>
> Diff: https://reviews.apache.org/r/40978/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 40978: Modify the Http test process in
FetcherTests to be symmetric in spawn/terminate.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40978/#review109492
-----------------------------------------------------------
src/tests/fetcher_tests.cpp (line 278)
<https://reviews.apache.org/r/40978/#comment169064>
You are right, but all other process constructors use the syntax used here. I suggest we leave it for now and discuss changing this offline (from this ticket).
src/tests/fetcher_tests.cpp (line 295)
<https://reviews.apache.org/r/40978/#comment169063>
Unfortunately there is no precedence for this. All other code like this uses get(). I think this is good enough because it is clear that this is a fresh instance that gets used here.
- Bernd Mathiske
On Dec. 4, 2015, 10:49 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40978/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 10:49 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Artem Harutyunyan.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the comment here: https://reviews.apache.org/r/40501/#comment168391
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 069c9bc6bc54d5bc1275c55e329457651d3c7b71
>
> Diff: https://reviews.apache.org/r/40978/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 40978: Modify the Http test process in
FetcherTests to be symmetric in spawn/terminate.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40978/#review109485
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Dec. 4, 2015, 6:49 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40978/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 6:49 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Artem Harutyunyan.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the comment here: https://reviews.apache.org/r/40501/#comment168391
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 069c9bc6bc54d5bc1275c55e329457651d3c7b71
>
> Diff: https://reviews.apache.org/r/40978/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 40978: Modify the Http test process in
FetcherTests to be symmetric in spawn/terminate.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Dec. 9, 2015, 2:31 a.m., Benjamin Bannier wrote:
> > src/tests/fetcher_tests.cpp, line 278
> > <https://reviews.apache.org/r/40978/diff/1/?file=1154021#file1154021line278>
> >
> > Unneeded dead code which should be removed (otherwise should be `= default`).
You are right, but all other process constructors use the syntax used here. I suggest we leave it for now and discuss changing this offline (from this ticket).
> On Dec. 9, 2015, 2:31 a.m., Benjamin Bannier wrote:
> > src/tests/fetcher_tests.cpp, line 295
> > <https://reviews.apache.org/r/40978/diff/1/?file=1154021#file1154021line295>
> >
> > You could use `spawn(*process)` here to be more brief, have an additional `NULL` check, and to keep the appearance already present in `EXPECT_CALL`s here.
Unfortunately there is no precedence for this. All other code like this uses get(). I think this is good enough because it is clear that this is a fresh instance that gets used here.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40978/#review109486
-----------------------------------------------------------
On Dec. 4, 2015, 10:49 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40978/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 10:49 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Artem Harutyunyan.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the comment here: https://reviews.apache.org/r/40501/#comment168391
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 069c9bc6bc54d5bc1275c55e329457651d3c7b71
>
> Diff: https://reviews.apache.org/r/40978/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 40978: Modify the Http test process in
FetcherTests to be symmetric in spawn/terminate.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40978/#review109486
-----------------------------------------------------------
Ship it!
src/tests/fetcher_tests.cpp (line 278)
<https://reviews.apache.org/r/40978/#comment169061>
Unneeded dead code which should be removed (otherwise should be `= default`).
src/tests/fetcher_tests.cpp (line 295)
<https://reviews.apache.org/r/40978/#comment169060>
You could use `spawn(*process)` here to be more brief, have an additional `NULL` check, and to keep the appearance already present in `EXPECT_CALL`s here.
- Benjamin Bannier
On Dec. 4, 2015, 6:49 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40978/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 6:49 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Artem Harutyunyan.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the comment here: https://reviews.apache.org/r/40501/#comment168391
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 069c9bc6bc54d5bc1275c55e329457651d3c7b71
>
> Diff: https://reviews.apache.org/r/40978/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 40978: Modify the Http test process in
FetcherTests to be symmetric in spawn/terminate.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40978/#review109483
-----------------------------------------------------------
Ship it!
This looks a lot like other HTTP tests now. Good job replicating the prevailing patterns.
- Bernd Mathiske
On Dec. 4, 2015, 10:49 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40978/
> -----------------------------------------------------------
>
> (Updated Dec. 4, 2015, 10:49 a.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Artem Harutyunyan.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This addresses the comment here: https://reviews.apache.org/r/40501/#comment168391
>
>
> Diffs
> -----
>
> src/tests/fetcher_tests.cpp 069c9bc6bc54d5bc1275c55e329457651d3c7b71
>
> Diff: https://reviews.apache.org/r/40978/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>