Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-18 Thread Joris Van Remoortere

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


Ship it!




rename the test variables as suggested.

- Joris Van Remoortere


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.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
> 
>



Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-14 Thread Alex Clemmer


> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp, lines 22-23
> > 
> >
> > This seems generally useful?
> > I think I've even seen it in other reviews?
> > Why is it burried in a test?
> 
> Alex Clemmer wrote:
> 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.
> 
> Joris Van Remoortere wrote:
> Sorry I must have messed up the selection highlight.
> I meant this: `const string errorMessage(int err)`.

We already have code like this in `WindowsError`, and slightly different code 
like this in `os::hstrerror`.

The thinking was, the tests below are calling `::strerror` and comparing them 
to `os::strerror`, and what we'd really like to do is just call 
`::FormatMessage` straight up, but it's a bit too verbose to embed directly in 
the test. Hence, I wrote a small helper function.

Let me know what else you would like to see here.


- Alex


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


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.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
> 
>



Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-14 Thread Joris Van Remoortere


> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp, lines 26-27
> > 
> >
> > Please use plain names that are self descriptive.
> 
> Alex Clemmer wrote:
> 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!

How about:
```
string key = "key";
string value = "value";
string new_value = "new_value";
```


- Joris


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


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.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
> 
>



Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-14 Thread Joris Van Remoortere


> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp, lines 22-23
> > 
> >
> > This seems generally useful?
> > I think I've even seen it in other reviews?
> > Why is it burried in a test?
> 
> Alex Clemmer wrote:
> 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.

Sorry I must have messed up the selection highlight.
I meant this: `const string errorMessage(int err)`.


- Joris


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


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.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
> 
>



Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-13 Thread Alex Clemmer


> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp, line 35
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>



Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-13 Thread Alex Clemmer

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

(Updated April 13, 2016, 10:05 p.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 (updated)
-

  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



Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-12 Thread Joris Van Remoortere


> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/Makefile.am, line 48
> > 
> >
> > Are you missing an entry in `3rdparty/libprocess/3rdparty/Makefile.am`?

I see it's the subsequent review.


- Joris


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



Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

2016-04-12 Thread Joris Van Remoortere

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




3rdparty/libprocess/3rdparty/stout/Makefile.am (line 48)


Are you missing an entry in `3rdparty/libprocess/3rdparty/Makefile.am`?



3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp (line 22)


Please move this above the `EnvTest` class declaration.



3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp (lines 26 - 27)


Please use plain names that are self descriptive.



3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp (line 35)


If on L36 you assume the value, should you not `ASSERT_SOME` here?

Same below.



3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp (line 38)


s/set environment/set the environment/



3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp (lines 22 - 23)


This seems generally useful?
I think I've even seen it in other reviews?
Why is it burried in a test?


- Joris Van Remoortere


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