----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21968/#review44245 -----------------------------------------------------------
Looks great, but I wonder if we can simplify the unit test a little (test on register, not reregister) 3rdparty/libprocess/include/process/gmock.hpp <https://reviews.apache.org/r/21968/#comment78607> This strategy seems reasonable to me. The only other alternative I can think of is to get the future from FUTURE_MESSAGE, and then ensure that it is never satisfied/ready by the end of your test. We should probably ask BenH for his opinion. src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78588> This could instead go below, just before StartSlave, or above the exec definition (first of 3 parameters to StartSlave). I just don't like to see the exec/flags/detector parameter setup broken up. src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78587> Why needed, to simulate master failover? Can't you just test this during initial registration? src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78589> Why wait until registered the first time, instead of testing TERMINATING during original registration? src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78590> Why create/run a task at all? src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78591> Unnecessary, Times(1) is implied src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78592> Please provide a non-empty task name src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78593> Unnecessary src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78594> Interesting trick. Explain.. src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78595> "expectation" and explain in the comment what you're expecting src/tests/slave_tests.cpp <https://reviews.apache.org/r/21968/#comment78596> You might want to set this expectation a little earlier (right after your await_ready), so you don't miss an earlier message. - Adam B On May 28, 2014, 3:18 p.m., Yifan Gu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21968/ > ----------------------------------------------------------- > > (Updated May 28, 2014, 3:18 p.m.) > > > Review request for mesos, Adam B and Niklas Nielsen. > > > Bugs: MESOS-878 > https://issues.apache.org/jira/browse/MESOS-878 > > > Repository: mesos-git > > > Description > ------- > > Added a sentence to check the slave's state in doReliableRegistration() to > make sure that the function returns when the slave's state is TERMINATING. > Also, to write the test, I need to make sure there's no additional > doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and > NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/gmock.hpp daba7e3 > src/slave/slave.cpp 7fbedb1 > src/tests/mesos.hpp 1b0f358 > src/tests/slave_tests.cpp 80fe3cf > > Diff: https://reviews.apache.org/r/21968/diff/ > > > Testing > ------- > > Slave_tests.cpp:TerminatingSlaveDoesNotReregister. > > > Thanks, > > Yifan Gu > >
