-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25569/#review53893
-----------------------------------------------------------
Thanks Tim, this is a great step forward! Getting close!
Some slight suggestions to change the TestFilter design:
(1) I noticed that the `matches` method seems unncessary, since the caller only
cares about filtering when it matches. So why not move the matching logic
inside the test filter, rather than forcing the caller to call `matches` every
time?
(2) The #ifdefs here are a bit confusing, sometimes the filters are always
constructed but contain an #ifdef in the `filter` method, and sometimes the
filters are both constructed in an #ifdef yet **still** contain an #ifdef in
the `filter` method. How about the following to be consistent here: each filter
is contained fully inside the relevant #ifdef, the construction of it is
guarded by the same #ifdef.
(3) We tend to favor composition over inheritance for a number of reasons. Here
is a suggestion to clean up the inheritance tree a bit. Rather than having
TestFilter, TestNameFilter, and TestParamTypeFilter hierarchy. Could we get
away with a single TestFilter parent type?
```
class TestFilter {
public:
virtual TestFilter() {}
virtual ~TestFilter() {}
virtual bool filter(const ::testing::TestInfo* test) const = 0;
// Returns whether the test name / parameterization matches the pattern.
static bool matches(const ::testing::TestInfo* test, const string pattern)
{
if (strings::contains(test->test_case_name(), pattern) ||
strings::contains(test->name(), pattern)) {
return true;
} else if (test->type_param() != NULL &&
strings::contains(test->type_param(), pattern)) {
return true;
}
return false;
}
};
```
Then all the filters inherit directly from `TestFilter` and can leverage the
static `matches` to implement `filter`. This makes the filtering a bit more
direct (no need to worry about the `passed` boolean. It also makes things a bit
more flexible, because we no longer enforce only 1 pattern per filter (which
meant that you needed 2 cgroup filters, one for test name, one for test
parameterization).
(4) Given this, would love to see a single CgroupTestFilter and
PortMappingTestFilter with this approach (instead of 1 per pattern)! :D
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93756>
Still need this?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93757>
Still need this?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93759>
Could we remove isRootUser? Looks like it won't be necessary with the other
comments I've made (less calls needed).
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93760>
Could this be merged into the RootUserFilter within an #ifdef (per my
design comments)?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93753>
Shouldn't the ':' be the responsibility of the caller, when joining the
result of this call?
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93755>
How about s/testFilter/filters/ ? :)
src/tests/environment.cpp
<https://reviews.apache.org/r/25569/#comment93754>
Since this is a .cpp file, you can add a using clause for process::Owned so
that you don't need the line wrapping here.
- Ben Mahler
On Sept. 16, 2014, 10:35 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25569/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 10:35 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
>
>
