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 43613: Refactor cluster test helpers into self-contained objects.
Date Wed, 02 Mar 2016 21:45:10 GMT


> On March 1, 2016, 5:56 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 97
> > <https://reviews.apache.org/r/43613/diff/7/?file=1272581#file1272581line97>
> >
> >     The local assertions here do not stop the test outside the factory method from
progressing. This hazard creates implicit dependencies between the implementation here and
test code following call sites.
> >     
> >     Not making the return type here a Try makes the whole method and code following
its call sites more fragile. Everything may look rock-solid right now, but if anyone later
makes any changes, she must keep in mind that none of the pointers in members of the resulting
master object can become stale. Otherwise they can cause test crashes. Not saying there are
crashes now, but the probability of introducing them just went up.
> >     
> >     Worse: should every test writer read the implementation code here and consider
this? This kind of dependency slows down development. I suggest we avoid it.
> >     
> >     If there were still an ASSERT_SOME(master) in every test right behind the creation
of the master, then the test would exit prematurely and no stale members could cause crashes.

Ok.  Changed all these asserts back into errors.


> On March 1, 2016, 5:56 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 313
> > <https://reviews.apache.org/r/43613/diff/7/?file=1272581#file1272581line313>
> >
> >     FAIL() is commonly used to indicate a test failure. But such a failure should
pertain to the subject matter of the individual test, which is not the case here.
> >     
> >     gtest might simply mark this test as failed, but the situation is actually worse.
Here we are looking at a malfunction of non-specific support code.
> >     
> >     However, if we use a Try return type as discussed above, the ASSERT_SOME in
the top level test code makes things right again.
> >     
> >     Still, strictly speaking, it "look more "correct" if writing this here, assuming
the Try:
> >     
> >         LOG(FATAL) << "Failed to wait for _recover";
> >         return;

I've changed this one to return an Error.

Note that `LOG(FATAL)` is roughly equivalent to `EXIT(EXIT_FAILURE)`.  (Rather than `ASSERT_TRUE(false)`.)


> On March 1, 2016, 5:56 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 124
> > <https://reviews.apache.org/r/43613/diff/7/?file=1272581#file1272581line124>
> >
> >     In order to cause an Error to be returned as the overall result when such an
assertion fires, you could capture a bool by reference that gets set at the very end of the
inline closure. If the bool is false, we have an Error.

Ended up getting rid of the inner closure for the `start` factories.


- Joseph


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


On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> -----------------------------------------------------------
> 
> (Updated Feb. 26, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, and Michael
Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
>     https://issues.apache.org/jira/browse/MESOS-4633
>     https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of test objects
to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The `Slave`
object performs cleanup originally found in `cluster::Slave::stop`.  `cluster::Master::start`
and `cluster::Slave::start` were changed to factory methods.
> 
> 
> Diffs
> -----
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> -------
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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