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