> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp, line 35
> > <https://reviews.apache.org/r/46009/diff/1/?file=1339066#file1339066line35>
> >
> >     If on L36 you assume the value, should you not `ASSERT_SOME` here?
> >     
> >     Same below.

I don't know. I don't really understand when we're supposed to `ASSERT` vs 
`EXPECT`. I thought that we were supposed to `ASSERT` only when we wanted the 
whole test to halt, which isn't the case here, right? It seems like we can get 
more information about failure if we keep going.

I'll change it and we'll explain it to me later.


> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp, lines 22-23
> > <https://reviews.apache.org/r/46009/diff/1/?file=1339067#file1339067line22>
> >
> >     This seems generally useful?
> >     I think I've even seen it in other reviews?
> >     Why is it burried in a test?

We are doing this to stay consistent with Neil Conway's pass over the tests to 
use `std::string`. For the general codebase, maybe it's useful, and maybe it's 
not, I'm not sure.


> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp, lines 26-27
> > <https://reviews.apache.org/r/46009/diff/1/?file=1339066#file1339066line26>
> >
> >     Please use plain names that are self descriptive.

I'm not sure whether you're talking about the symbol `key` or the string that 
is its value so I made the first more descriptive and the second more boring. 
Christmas comes early to the Van Remoortere household!


- Alex


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


On April 11, 2016, 8:31 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> -----------------------------------------------------------
> 
> (Updated April 11, 2016, 8:31 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Stout:[1/2] Added simple tests for `os::` functions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> c9d331df2f4496183b5734d2434413f68b9c1b4b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp 
> d5a96ad6b8b1c09b4f016e0c8e3e5c5672b55ef3 
> 
> Diff: https://reviews.apache.org/r/46009/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Reply via email to