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 

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