> On Dec. 26, 2014, 9:11 p.m., Timothy Chen wrote: > > Can you add a unit test? > > Nishant Suneja wrote: > I wanted to add one, but I would need access to the "Future" object for > the container launch, or "Timer" object for registration timeout trigger, > both of which > I cannot access, since they are local variables, and NOT the member > variables of any class. > > Any ideas as to how do I get around it, without introducting any extra > member variables ? > > Timothy Chen wrote: > you can get hold of the future by passing in a mock containerizzer and > perform a EXPECT_CALL(comtainerizer, launch(.....)) and return a future that > you instantiate in test. > > Nishant Suneja wrote: > Thats a neat way to get hold of the future object. So, for testing I have > to basically verify that the timer object for Slave's "registerExecutor" is > not created, till we reach the READY state of the future object. Now, I could > have done something crazy like iterate the map of timer objects in the Clock > class, and verify that we dont have this timer object till we are in READY > state of the future object. > However, this map is a defined as static in clock.cpp, so I wouldnt be > able to access it in the testcase, without making the map global (not a good > idea). > > So, again, any ideas ? > > Timothy Chen wrote: > The easiest way is to simply advance the clock (Clock::advance(....)) > until the timeout, and verify that the delay call is not yet called. Then you > satisfy the future and advance the time again and verify it is called. > > Nishant Suneja wrote: > Ok...And whats the best way to verify that the > "Slave::registerExecutorTimeout" has been called (after advancing the clock). > I could check the state of the executor, and verify that its TERMINATING, > which would mean that this method has executed. > > Any other cleaner way to verify this callback ? I believe that > EXPECT_CALL macro only works for classes with MOCK_METHOD set of methods > defined. So, I cant use that. > > Timothy Chen wrote: > One way to verify this is to use FUTURE_DISPATCH (you can search for > examples in existing test). FUTURE_DISPATCH returns a future, which becomes > ready when the method provided is dispatched (which means queued to serve in > libprocess). Delay shouldn't queue it until the timeout happen. > > Nishant Suneja wrote: > Yeah.. FUTURE_DISPATCH should do the job. > However, a question about the failure testcase. When we invoke > Clock::advance() by a certain duration, any registered timer object which > falls within that duration gets > called. For 2nd test, I can use the AWAIT_READY() macro on the future, > and then advancing the clock should result in our "registerExecutorTimeout" > being called, which is good. > > Although, for the 1st test case, before advancing the clock, I would want > to make sure that the launch() function has actually returned, and our > current code triggers the timer (otherwise, I would be advancing the clock, > when no timer object has actually been set, so obviously nothing will get > fired by advancing the clock). > EXPECT_CALL gives us a handle to that Future object, but how do I > determine that launch() method has returned (still in pending state though), > so i can advance the clock ? i.e. on what condition should I wait here?
EXPECT_CALL is called when the method is invoked, and whatever action you chain it with (.WillOnce(Returns(....)) is called immediately, so for example if you want to wait on the call, you can use EXPECT_CALL(containerizer, launch(_, _, _......)).WillOnce(FutureSatisify(&future)) and then AWAIT_READY(future), which the EXPECT_CALL will set the future state to be ready when the launch method is called. I think what I would do though is to create a promise, and return that promise's future instead: Promise<Nothing> promise; EXPECT_CALL(containerizer, launch(_, _, _,....)) .WillOnce(Return(promise.future())); And then Clock::pause(), Clock::advance(timeout), and verify that the executor timeout is not ready ASSERT_TRUE(timeoutFuture.isPending()) then satisfy the promise so the future is set to ready: promise.set(Nothing()) After wards advance the clock and verify that it is ready. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29437/#review66171 ----------------------------------------------------------- On Dec. 26, 2014, 6:29 p.m., Nishant Suneja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29437/ > ----------------------------------------------------------- > > (Updated Dec. 26, 2014, 6:29 p.m.) > > > Review request for mesos. > > > Bugs: MESOS-999 > https://issues.apache.org/jira/browse/MESOS-999 > > > Repository: mesos-git > > > Description > ------- > > As part of this bug fix, I have trigerred the executor registration timeout > timer asychronously, > when the onReady() callback is made for the container's future object, > instead of starting the > timer synchronously when the launchExecutor() method of the Framework class > is invoked. > > > Diffs > ----- > > src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6 > src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b > > Diff: https://reviews.apache.org/r/29437/diff/ > > > Testing > ------- > > make check succeeds. > > > Thanks, > > Nishant Suneja > >
