Re: Review Request 25569: Refactor test environment validations

2014-09-23 Thread Ben Mahler 

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

If possible, we can just keep it as a Try:
s/OptionError/TryDocker/

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

Re: Review Request 25569: Refactor test environment validations

2014-09-23 Thread Timothy Chen 


 On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
  src/tests/environment.cpp, lines 57-59
  https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line57
 
  Missing includes for these?
  
  #include set
  #include vector

I think it's already included, but I'll add it in case.


 On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
  src/tests/environment.cpp, lines 152-160
  https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line152
 
  The other CgroupsParamTypeFilter checks for root user, but this one 
  doesn't?

That's identical to what the original environment validation does.


 On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
  src/tests/environment.cpp, line 210
  https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line210
 
  Any reason to for the conversion to an OptionError?
  
  If possible, we can just keep it as a Try:
  s/OptionError/TryDocker/
  
  That way, the check read a bit more directly:
  
  ```
  return matches(test, DOCKER_)  docker.isError();
  ```
  vs. existing patch:
  ```
  return matches(test, DOCKER_)  dockerError.isSome();
  ```

I tried that, but I can't compile since TryError has no default constructor, 
and I don't think I have anything useful to initialize it with. You have 
suggestions?


 On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
  src/tests/environment.cpp, lines 224-241
  https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line224
 
  Can this one be collapsed into the CgropusFilter? Seems a bit confusing 
  to split the cgroup filter logic into two separate filters.

For some reason the original validation was testing both isRoot and Cgroups 
Params the same type, therefore I split it into two filters.


 On Sept. 23, 2014, 7:10 p.m., Ben Mahler wrote:
  src/tests/environment.cpp, lines 249-269
  https://reviews.apache.org/r/25569/diff/7/?file=701944#file701944line249
 
  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 :)

:) thx, I'll be consistent in my style then!


- Timothy


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


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

Re: Review Request 25569: Refactor test environment validations

2014-09-23 Thread Timothy Chen 

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

(Updated Sept. 23, 2014, 9:24 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos-git


Description
---

Review: https://reviews.apache.org/r/25569


Diffs (updated)
-

  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-23 Thread Timothy Chen 

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

(Updated Sept. 23, 2014, 10:02 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos-git


Description
---

Review: https://reviews.apache.org/r/25569


Diffs (updated)
-

  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-23 Thread Ben Mahler 

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

Ship it!


Thanks so much Tim, this is a great cleanup!

I'll fix the logical bugs and land this for you shortly.


src/tests/environment.cpp
https://reviews.apache.org/r/25569/#comment94479

Missing the include for process/owned



src/tests/environment.cpp
https://reviews.apache.org/r/25569/#comment94487

Looks like the double negatives were pretty tricky per the bug below, I'll 
rename this to 'disabled' to make it a bit clearer.



src/tests/environment.cpp
https://reviews.apache.org/r/25569/#comment94488

This is a bug! Should be ||, not 



src/tests/environment.cpp
https://reviews.apache.org/r/25569/#comment94489

|| here as well


- Ben Mahler


On Sept. 23, 2014, 10:02 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/25569/
 ---
 
 (Updated Sept. 23, 2014, 10:02 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

Re: Review Request 25569: Refactor test environment validations

2014-09-22 Thread Timothy Chen 

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

(Updated Sept. 22, 2014, 9:17 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos-git


Description
---

Review: https://reviews.apache.org/r/25569


Diffs (updated)
-

  src/linux/cgroups.cpp 62df4b7645c6ab061a47634058d79ca849caa6b9 
  src/slave/containerizer/isolators/cgroups/cpushare.hpp 
2187c296ea9b1a7de9ae3f09fdf1983f98a3d01b 
  src/slave/containerizer/isolators/cgroups/cpushare.cpp 
7164ecc0f068d4a72248521e3cbd345958efa880 
  src/slave/containerizer/isolators/cgroups/mem.cpp 
b3d4a5daa90a842e501bc6be2f0cf20fe22906ac 
  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
4ced508e600e13f3e5ae9d12ea199de743def652 
  src/slave/containerizer/linux_launcher.cpp 
f7bc894830a7ca3f55465dacc7b653cdc2d7758b 
  src/slave/slave.cpp 9a6646f0249fd43ae5d13bd9ee3b5da08412 
  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-22 Thread Ben Mahler 

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


Did you look at the diff when you posted this review? Looks like you needed to 
rebase against master.

- Ben Mahler


On Sept. 22, 2014, 9:17 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/25569/
 ---
 
 (Updated Sept. 22, 2014, 9:17 p.m.)
 
 
 Review request for mesos and Ben Mahler.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Review: https://reviews.apache.org/r/25569
 
 
 Diffs
 -
 
   src/linux/cgroups.cpp 62df4b7645c6ab061a47634058d79ca849caa6b9 
   src/slave/containerizer/isolators/cgroups/cpushare.hpp 
 2187c296ea9b1a7de9ae3f09fdf1983f98a3d01b 
   src/slave/containerizer/isolators/cgroups/cpushare.cpp 
 7164ecc0f068d4a72248521e3cbd345958efa880 
   src/slave/containerizer/isolators/cgroups/mem.cpp 
 b3d4a5daa90a842e501bc6be2f0cf20fe22906ac 
   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
 4ced508e600e13f3e5ae9d12ea199de743def652 
   src/slave/containerizer/linux_launcher.cpp 
 f7bc894830a7ca3f55465dacc7b653cdc2d7758b 
   src/slave/slave.cpp 9a6646f0249fd43ae5d13bd9ee3b5da08412 
   src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 
 
 Diff: https://reviews.apache.org/r/25569/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-22 Thread Timothy Chen 


 On Sept. 22, 2014, 10:52 p.m., Ben Mahler wrote:
  Did you look at the diff when you posted this review? Looks like you needed 
  to rebase against master.

Sorry didn't really look at it and you're right it's not rebased!


- Timothy


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


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

Re: Review Request 25569: Refactor test environment validations

2014-09-22 Thread Timothy Chen 

---
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 (updated)
-

  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-20 Thread Timothy Chen 

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

(Updated Sept. 20, 2014, 5:09 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos-git


Description
---

Review: https://reviews.apache.org/r/25569


Diffs (updated)
-

  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-19 Thread Timothy Chen 

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



src/tests/environment.cpp
https://reviews.apache.org/r/25569/#comment93864

Sure, I didn't see any other possibility needed so I just merged it here. I 
can just return the TestInfo instead.


- Timothy Chen


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

Re: Review Request 25569: Refactor test environment validations

2014-09-19 Thread Timothy Chen 

---
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 (updated)
-

  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-19 Thread Ben Mahler 

---
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:
```
vectorstring 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 TryDocker 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 'setstring 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:

Re: Review Request 25569: Refactor test environment validations

2014-09-19 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [25569]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 25569: Refactor test environment validations

2014-09-18 Thread Ben Mahler 

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

Re: Review Request 25569: Refactor test environment validations

2014-09-16 Thread Timothy Chen 

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

(Updated Sept. 16, 2014, 7:07 a.m.)


Review request for mesos and Ben Mahler.


Summary (updated)
-

Refactor test environment validations


Repository: mesos-git


Description (updated)
---

Review: https://reviews.apache.org/r/25569

Refactor how validation is done in the environment by defining a standard test 
filter interface, and only execute the validation once.


Diffs (updated)
-

  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-16 Thread Timothy Chen 


 On Sept. 12, 2014, 11:39 p.m., Ben Mahler wrote:
  Thanks for following up!
  
  Not your fault, but the current design of enable() seems a bit unfortunate, 
  because we will print things excessively unless we use static variables as 
  you've done here.
  What about the following instead?
  
  (1) Add a method called 'disabled()' that returns a list of disabled test 
  names.
  (2) Inside 'disabled()', we can print the disablement messages once at the 
  beginning, then loop over the tests to construct the list of disabled test 
  names to return.
  (3) Inside 'Environment::Environment()', we use disabled() to construct the 
  full 'disabled' string.
  
  Does that seem cleaner?
  
  The static variables seem good for now, modulo my comment below. Consider 
  adding a TODO to clean this up if you go this route!

Ok I've done a refactor based on your comments, let me know what you think.


- Timothy


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


On Sept. 16, 2014, 7:07 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/25569/
 ---
 
 (Updated Sept. 16, 2014, 7:07 a.m.)
 
 
 Review request for mesos and Ben Mahler.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Review: https://reviews.apache.org/r/25569
 
 Refactor how validation is done in the environment by defining a standard 
 test filter interface, and only execute the validation once.
 
 
 Diffs
 -
 
   src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 
 
 Diff: https://reviews.apache.org/r/25569/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-16 Thread Mesos ReviewBot 

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


Bad patch!

Reviews applied: [25569]

Failed command: ./support/mesos-style.py

Error:
 Checking 504 files using filter 
--filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
src/tests/environment.cpp:279:  Lines should be = 80 characters long  
[whitespace/line_length] [2]
Total errors found: 1

- Mesos ReviewBot


On Sept. 16, 2014, 7:07 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/25569/
 ---
 
 (Updated Sept. 16, 2014, 7:07 a.m.)
 
 
 Review request for mesos and Ben Mahler.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Review: https://reviews.apache.org/r/25569
 
 Refactor how validation is done in the environment by defining a standard 
 test filter interface, and only execute the validation once.
 
 
 Diffs
 -
 
   src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 
 
 Diff: https://reviews.apache.org/r/25569/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-16 Thread Dominic Hamon 

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



src/tests/environment.cpp
https://reviews.apache.org/r/25569/#comment93236

vectorprocess::OwnedTestFilter testFilters;

we will eventually move process::Owned to unique_ptr so please start using 
it :)


- Dominic Hamon


On Sept. 16, 2014, 12:07 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/25569/
 ---
 
 (Updated Sept. 16, 2014, 12:07 a.m.)
 
 
 Review request for mesos and Ben Mahler.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Review: https://reviews.apache.org/r/25569
 
 Refactor how validation is done in the environment by defining a standard 
 test filter interface, and only execute the validation once.
 
 
 Diffs
 -
 
   src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 
 
 Diff: https://reviews.apache.org/r/25569/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-16 Thread Timothy Chen 

---
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 (updated)
---

Review: https://reviews.apache.org/r/25569


Diffs (updated)
-

  src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 

Diff: https://reviews.apache.org/r/25569/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 25569: Refactor test environment validations

2014-09-16 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [25569]

All tests passed.

- Mesos ReviewBot


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