Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[Jan Schlicht]

> On April 18, 2017, 1:56 p.m., Jan Schlicht wrote:
> > Because I need to implement similar filters for tests in `libprocess`, 
> > wouldn't it have been better to create a `Filter` abstraction in stout and 
> > using that here as well as in Mesos instead of copy-pasting the 
> > implementation of Mesos?
> > Also the copied Mesos code seems to not quite fit the stout code-style, 
> > e.g. it uses camel case instead of snake case.

Oh, seems like the same filters should be using in `libprocess` as well, as per 
https://reviews.apache.org/r/57971/. Not a good choice because the filters that 
need to be used for stout might differ from the ones used in libprocess. I'll 
probably have to refactor the current filter handling to support this use case 
(i.e. applying a filter only in `libprocess-tests`).


- Jan


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


On April 10, 2017, 11:41 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57824/
> ---
> 
> (Updated April 10, 2017, 11:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filtered stout tests with symlinks when unable to create symlinks.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
>   3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 
> 
> 
> Diff: https://reviews.apache.org/r/57824/diff/4/
> 
> 
> Testing
> ---
> 
> Ran make check on Linux and the tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[Jan Schlicht]

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



Because I need to implement similar filters for tests in `libprocess`, wouldn't 
it have been better to create a `Filter` abstraction in stout and using that 
here as well as in Mesos instead of copy-pasting the implementation of Mesos?
Also the copied Mesos code seems to not quite fit the stout code-style, e.g. it 
uses camel case instead of snake case.

- Jan Schlicht


On April 10, 2017, 11:41 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57824/
> ---
> 
> (Updated April 10, 2017, 11:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filtered stout tests with symlinks when unable to create symlinks.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
>   3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 
> 
> 
> Diff: https://reviews.apache.org/r/57824/diff/4/
> 
> 
> Testing
> ---
> 
> Ran make check on Linux and the tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[Joseph Wu]

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


Fix it, then Ship it!




I can fix the below issues for you, before committing.  I'd recommend taking a 
glance at the commit after I've pushed it, so that you can cross reference the 
comments with the actual change I made.


3rdparty/stout/include/stout/tests/environment.hpp
Lines 38-59 (patched)


I know this was copy-pasted from the source in the Mesos tests, but most of 
these headers are un-used.

You can whittle these headers down to:
```
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#include 
```



3rdparty/stout/tests/main.cpp
Lines 23 (patched)


Note: For the autotools builds, you'll need to add the new header to 
`3rdparty/stout/include/Makefile.am`.



3rdparty/stout/tests/main.cpp
Lines 27 (patched)


The only reason why the `Environment*` pointer is kept in global scope in 
`src/tests/environment` is for the `environment->mkdtemp` function.  Since we 
don't have such a function for stout tests, we can just keep the pointer inside 
the main function.


- Joseph Wu


On April 10, 2017, 2:41 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57824/
> ---
> 
> (Updated April 10, 2017, 2:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filtered stout tests with symlinks when unable to create symlinks.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
>   3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 
> 
> 
> Diff: https://reviews.apache.org/r/57824/diff/4/
> 
> 
> Testing
> ---
> 
> Ran make check on Linux and the tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[John Kordich via Review Board]

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

(Updated April 10, 2017, 9:41 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Bugs: MESOS-6731
https://issues.apache.org/jira/browse/MESOS-6731


Repository: mesos


Description
---

Filtered stout tests with symlinks when unable to create symlinks.


Diffs (updated)
-

  3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
  3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
  3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 


Diff: https://reviews.apache.org/r/57824/diff/4/

Changes: https://reviews.apache.org/r/57824/diff/3-4/


Testing
---

Ran make check on Linux and the tests on Windows.


Thanks,

John Kordich

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[Joseph Wu]

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




3rdparty/stout/include/stout/tests/environment.hpp
Lines 205-210 (patched)


This is an incomplete function (declared but not defined).  Remove it?



3rdparty/stout/tests/main.cpp
Lines 69 (patched)


Consider putting this into the Environment constructor, so that deriving 
Environments do not need to add this to their own filters.


- Joseph Wu


On April 5, 2017, 12:08 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57824/
> ---
> 
> (Updated April 5, 2017, 12:08 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filtered stout tests with symlinks when unable to create symlinks.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
>   3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 
> 
> 
> Diff: https://reviews.apache.org/r/57824/diff/3/
> 
> 
> Testing
> ---
> 
> Ran make check on Linux and the tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[John Kordich via Review Board]

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

(Updated April 5, 2017, 7:08 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Changes
---

I'm not sure if I should leave it to the reviewer to check "Fixed" or "Drop", 
or if I should do it! In any case, I believe I've addressed all of the comments 
to the previous iteration.


Bugs: MESOS-6731
https://issues.apache.org/jira/browse/MESOS-6731


Repository: mesos


Description
---

Filtered stout tests with symlinks when unable to create symlinks.


Diffs (updated)
-

  3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
  3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
  3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 


Diff: https://reviews.apache.org/r/57824/diff/3/

Changes: https://reviews.apache.org/r/57824/diff/2-3/


Testing
---

Ran make check on Linux and the tests on Windows.


Thanks,

John Kordich

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[Joseph Wu]

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




3rdparty/stout/include/stout/tests/environment.hpp
Lines 17-18 (patched)


Try `__STOUT_TESTS_ENVIRONMENT_HPP__` instead.



3rdparty/stout/include/stout/tests/environment.hpp
Lines 196-198 (patched)


The test filters constitute the main difference between the test 
environment of Stout vs Libprocess vs Mesos.  If we're going to re-use the code 
effectively, the vector of filters should either be:
A) an argument of `Environment` 's constructor; or
B) added via a object method (i.e. `Environment::AddFilter(...)`)

Doing (A) might result in some rather beefy constructors in 
Libprocess/Mesos tests.  I prefer this approach because it does not add a 
method that could be mis-used later on.

Doing (B) would require moving most of the other logic in the `Environment` 
constructor into `Environment::SetUp()`.



3rdparty/stout/include/stout/tests/environment.hpp
Lines 218-224 (patched)


You don't have include these two if they are empty.



3rdparty/stout/include/stout/tests/environment.hpp
Lines 226-248 (patched)


Leave this out for now.  I have a TODO to move this temporary directory 
helper out of the `Environment` entirely, so that we don't need a global 
variable in some of the tests (at the Mesos level).



3rdparty/stout/tests/environment.hpp
Lines 1 (patched)


Delete this file.



3rdparty/stout/tests/environment.cpp
Lines 1 (patched)


Delete this file.


- Joseph Wu


On March 27, 2017, 3:13 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57824/
> ---
> 
> (Updated March 27, 2017, 3:13 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filtered stout tests with symlinks when unable to create symlinks.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/environment.cpp PRE-CREATION 
>   3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
>   3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 
> 
> 
> Diff: https://reviews.apache.org/r/57824/diff/2/
> 
> 
> Testing
> ---
> 
> Ran make check on Linux and the tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[John Kordich via Review Board]

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

(Updated March 27, 2017, 10:13 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Bugs: MESOS-6731
https://issues.apache.org/jira/browse/MESOS-6731


Repository: mesos


Description
---

Filtered stout tests with symlinks when unable to create symlinks.


Diffs (updated)
-

  3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
  3rdparty/stout/tests/environment.hpp PRE-CREATION 
  3rdparty/stout/tests/environment.cpp PRE-CREATION 
  3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
  3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 


Diff: https://reviews.apache.org/r/57824/diff/2/

Changes: https://reviews.apache.org/r/57824/diff/1-2/


Testing (updated)
---

Ran make check on Linux and the tests on Windows.


Thanks,

John Kordich

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[Joseph Wu]

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



In terms of review formatting, it look good.
You can make the review even better by filling in the `Description` field (with 
a beefier message summarizing the changes and reason behind the changes).  And 
in the `Testing Done` section, you can list the steps you took to validate the 
patch.  Usually a `make check` (or Windows equivalent) can suffice.

---

I'd like to work with you on a few adjustments to this patch:

* This patch basically copies chunks of the `src/environment.*`.  We should 
therefore refactor tests up the chain.  `stout::tests::internal::Environment` 
should be used in both `stout-tests` and `libprocess-tests` (which may have 
it's own set of tests that need Administrator rights, in future).
* `mesos::tests::internal::Environment` should be a sub-class of 
`stout::tests::internal::Environment`.
  ^ Note: Because stout/libprocess/mesos are separate components, these changes 
need to be done in separate patches.


3rdparty/stout/tests/environment.cpp
Lines 102-138 (patched)


This check doesn't need to mirror one of the tests, does it?  And can you 
move it into the body of the `SymlinkFilter` class?

---

Also, check your tab settings.  We use 2 spaces per tab.



3rdparty/stout/tests/environment.cpp
Lines 145-164 (patched)


There's no need to add Windows sections here.  Tests on Posix *should* pass 
the filter with no problems.


- Joseph Wu


On March 22, 2017, 11:08 a.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57824/
> ---
> 
> (Updated March 22, 2017, 11:08 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filtered stout tests with symlinks when unable to create symlinks.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am ebf1069eb1b787f063a2066a4db0b3f5de4a56da 
>   3rdparty/stout/tests/CMakeLists.txt 
> 4bbe713f259e7858d423dcb33956d41e62a915eb 
>   3rdparty/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/environment.cpp PRE-CREATION 
>   3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
>   3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 
> 
> 
> Diff: https://reviews.apache.org/r/57824/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[John Kordich via Review Board]

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

Review request for mesos.


Bugs: MESOS-6731
https://issues.apache.org/jira/browse/MESOS-6731


Repository: mesos


Description
---

Filtered stout tests with symlinks when unable to create symlinks.


Diffs
-

  3rdparty/stout/Makefile.am ebf1069eb1b787f063a2066a4db0b3f5de4a56da 
  3rdparty/stout/tests/CMakeLists.txt 4bbe713f259e7858d423dcb33956d41e62a915eb 
  3rdparty/stout/tests/environment.hpp PRE-CREATION 
  3rdparty/stout/tests/environment.cpp PRE-CREATION 
  3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
  3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 


Diff: https://reviews.apache.org/r/57824/diff/1/


Testing
---


Thanks,

John Kordich