----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25569/#review54300 -----------------------------------------------------------
This is coming together really nicely Tim! Just some minor cleanups left at this point, and then we can get this committed! :) src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94351> Missing includes for these? #include <set> #include <vector> src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94356> At this point I think these methods are self-evident, but I will leave it to you as to whether you think these comments are still beneficial. :) src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94357> const string& pattern src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94398> The other CgroupsParamTypeFilter checks for "root" user, but this one doesn't? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94369> Any reason to for the conversion to an Option<Error>? If possible, we can just keep it as a Try: s/Option<Error>/Try<Docker>/ That way, the check read a bit more directly: ``` return matches(test, "DOCKER_") && docker.isError(); ``` vs. existing patch: ``` return matches(test, "DOCKER_") && dockerError.isSome(); ``` src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94396> Can this one be collapsed into the CgropusFilter? Seems a bit confusing to split the cgroup filter logic into two separate filters. src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94392> Does the following seem a bit easier to read? Trying to keep it as only one logical structure (only 1 "matches" expression, but what happens is controlled by the #ifdef): ``` #ifdef WITH_NETWORK_ISOLATOR if (matches(test, "MultipleSlaves")) { return !routing::check().isError(); } #endif if (matches(test, "PortMappingIsolatorTest") || matches(test, "PortMappingMesosTest")) { #ifdef WITH_NETWORK_ISOLATOR return routing::check().isError(); #else return true; #endif } return false; ``` vs. current patch: ``` #ifdef WITH_NETWORK_ISOLATOR if (matches(test, "MultipleSlaves")) { return !routing::check().isError(); } if (matches(test, "PortMappingIsolatorTest") || matches(test, "PortMappingMesosTest")) { return routing::check().isError(); } return false; #else return matches(test, "PortMappingIsolatorTest") || matches(test, "PortMappingMesosTest"); #endif // WITH_NETWORK_ISOLATOR } ``` FWIW, this is what you did for CgroupsFilter as well :) src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94350> How about s/disabledTests/disabled/ ? src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment94349> Looks like you're always adding two colons because of the outer strings::join(). You shouldn't need the ":" here anymore, right? How about the following to be consistent with the rest of our code formatting: ``` disabled.push_back( test->test_case_name() + string(".") + test->name()); ``` - Ben Mahler On Sept. 23, 2014, 5:28 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25569/ > ----------------------------------------------------------- > > (Updated Sept. 23, 2014, 5:28 a.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 > >
