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