mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.
Date Wed, 13 Apr 2016 22:05:03 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message