You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/03/12 20:10:29 UTC
Review Request 19144: Added test for subprocess with environment variable
set.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19144/
-----------------------------------------------------------
Review request for mesos and Vinod Kone.
Bugs: MESOS-1050 and MESOS-1063
https://issues.apache.org/jira/browse/MESOS-1050
https://issues.apache.org/jira/browse/MESOS-1063
Repository: mesos-git
Description
-------
see summary
Diffs
-----
3rdparty/libprocess/src/tests/subprocess_tests.cpp e7201763e08277ed6dd4c14b8cdcaf9125e7db2f
Diff: https://reviews.apache.org/r/19144/diff/
Testing
-------
make check
Thanks,
Dominic Hamon
Re: Review Request 19144: Added test for subprocess with environment
variable set.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19144/#review36962
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On March 12, 2014, 7:10 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19144/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 7:10 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-1050 and MESOS-1063
> https://issues.apache.org/jira/browse/MESOS-1050
> https://issues.apache.org/jira/browse/MESOS-1063
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp e7201763e08277ed6dd4c14b8cdcaf9125e7db2f
>
> Diff: https://reviews.apache.org/r/19144/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19144: Added test for subprocess with environment
variable set.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19144/#review36990
-----------------------------------------------------------
If we were to do better testing in r/18974, is there any value in this test?
I can see us adding this test *once* subprocess takes an 'environment' map, but until then this seems a bit arbitrary. See my comment below.
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19144/#comment68232>
Given my comments on r/18974, if we were to improve the testing in that review, I'm not sure this test adds any value.
We wanted to eventually modify os::subprocess to take an optional environment map. Since that functionality is currently missing, I think this test is ok. But, let's at least add a TODO above this test to indicate that we'd like to ultimately be testing the subprocess 'environment' functionality, which currently does not exist.
I'm hesitant to add tests for non-existent features like this, ideally we can kill these tests in place of better testing in r/18974. We can add tests for subprocess 'environment' functionality once it exists.
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19144/#comment68221>
2 space indent
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19144/#comment68223>
Can you add whitespace here to keep this consistent with the other tests? E.g. The 'input' test above.
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19144/#comment68224>
kill newline
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19144/#comment68225>
Ditto about keeping the whitespace consistent with the above tests.
- Ben Mahler
On March 12, 2014, 7:10 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19144/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 7:10 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-1050 and MESOS-1063
> https://issues.apache.org/jira/browse/MESOS-1050
> https://issues.apache.org/jira/browse/MESOS-1063
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp e7201763e08277ed6dd4c14b8cdcaf9125e7db2f
>
> Diff: https://reviews.apache.org/r/19144/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 19144: Added test for subprocess with environment
variable set.
Posted by Jörn Franke <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19144/#review36992
-----------------------------------------------------------
Ship it!
Ship It!
- Jörn Franke
On March 12, 2014, 7:10 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19144/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 7:10 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-1050 and MESOS-1063
> https://issues.apache.org/jira/browse/MESOS-1050
> https://issues.apache.org/jira/browse/MESOS-1063
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> see summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp e7201763e08277ed6dd4c14b8cdcaf9125e7db2f
>
> Diff: https://reviews.apache.org/r/19144/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Dominic Hamon
>
>