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