----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25569/#review54045 -----------------------------------------------------------
Thanks Tim, this is looking really nice! Appreciate the patience, just one more round of cleanup and we should be all set. :D src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93977> Can you remove this? We only have two callers, and the code is really trivial in the callers. :) src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93976> This might be a bit more succinct: // Returns whether the test should be filtered. src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93965> Curious why you want this vector version of matches. It adds a method, and there is still more code in the call site than just calling matches twice: Current: ``` vector<string> patterns; patterns.push_back("PortMappingIsolatorTest"); patterns.push_back("PortMappingMesosTest"); return matches(test, patterns) && !isRoutingEnabled; ``` vs. removing vector matches: ``` return (matches(test, "PortMappingIsolatorTest") matches(test, "PortMappingMesosTest") && !isRoutingEnabled; ``` Probably an if is a bit easier to read: ``` if (matches(test, "PortMappingIsolatorTest") || matches(test, "PortMappingMesosTest") { return routingDisabled; } return false; ``` src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93979> Could we avoid the need for a constructor and a member variable by calling os::user() inside filter()? Let's avoid trying to optimize this since it's likely not noticeably expensive. IMO doing it all in filter() actually makes the code more readable as well. :) src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93980> Can we avoid the need for the constructor + member variable here as well, by doing it all in filter()? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93972> We have 'enabled' in the name, so the 'is' seems redundant, the following seem clear without the 'is' prefix: 'cgroupsEnabled', 'dockerEnabled', etc. What do you think? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93981> How about including the message for non-linux systems as well: ``` #ifdef __linux__ docker = Docker::create(flags.docker); #else docker = Error("Docker tests not supported on non-Linux systems"); #endif if (docker.isError()) { // print message; } ``` Could we store the Try<Docker> as a member instead of converting to the 'isDockerEnabled' boolean? Doesn't look like the boolean is giving us very much? It even lets us make the call to Docker::create vs Error("non linux") in the initializer list. src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93984> Do you want to remove the std:: qualifiers here? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93982> This is pretty implicit for non-Linux systems, in that we treat them as having hierarchies. Could we have the #ifdef in the filter() method to make this a bit more clear? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93983> Based on my other comment, how about just storing 'set<string> hierarchies' and checking .empty() in filter()? But inside a linux #ifdef! ;) src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93985> We can avoid the need for the Constructor and the member variable if we just call these in filter(). The #ifdef inside filter will make things a bit clearer as well! src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93986> Ditto, can you avoid the constructor + member variable, and instead push the #ifdef down into filter() to make things a bit clearer? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93960> Leftover 'process::'? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93958> Very clean, thanks Tim! src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment93954> What about s/disabledTest/filtered/ ? If disabledTest() returned a vector of strings as before you can do the following here: ``` disabled += strings::join(":", filtered(unitTest, filters); ``` No clunky loop necessary. :) I should have been a bit more clear in my previous comment, what I meant was that we can do the strings::join here if we're not tacking on ":" in our vector items in disabledTest(). Sound good? - Ben Mahler On Sept. 19, 2014, 10:27 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25569/ > ----------------------------------------------------------- > > (Updated Sept. 19, 2014, 10:27 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > Review: https://reviews.apache.org/r/25569 > > > Diffs > ----- > > src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 > > Diff: https://reviews.apache.org/r/25569/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
