mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bernd Mathiske <be...@mesosphere.io>
Subject Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.
Date Tue, 01 Mar 2016 13:56:18 GMT

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




src/tests/cluster.cpp (line 91)
<https://reviews.apache.org/r/43613/#comment183126>

    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.



src/tests/cluster.cpp (line 109)
<https://reviews.apache.org/r/43613/#comment183127>

    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.



src/tests/cluster.cpp (line 233)
<https://reviews.apache.org/r/43613/#comment183128>

    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;


- Bernd Mathiske


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