mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.
Date Thu, 23 Mar 2017 20:49:37 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/57824/#comment242622>

    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)
<https://reviews.apache.org/r/57824/#comment242632>

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


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