> 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? > > Timothy Chen wrote: > 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. > > Nishant Suneja wrote: > Hey Timothy > > So, I went through the documentation of gmock, and got a better > understanding of the magic MACROS. > > That said, I still have 2 questions: > > 1) During a unit test run, where we instantiate a master and a slave, how > many processes are we actually dealing with ? > Logic says that we have 1 test case process, and its 2 child processes > (slave and master). > I ask this because it seems that we are sharing the containerizer > object between test case processs and its child (slave) process. > Also, the callback in the ACTION part of the EXPECT_CALL, I believe > that its been invoked in the context of the slave process. > Just wanted to clarify if I am on the correct page. > > 2) So, if the above statement is correct, in the code below, we seem to > have a race. We would expect this test case to pass with our > new code. However, in my patch, the call to delay() happens in > onReady() method of the launch future object, which happens > AFTER invocation of containerizer::launch(). > Now, its very well possible, that we pause and advance the clock in > the parent test case process, before the delay() > gets called in the slave process, in which case the timer wont fire at > all. > > 3) Which brings me to my third question, as to are we sharing the same > clock across these 3 processes ? > Clock class (for eg.) is packaged as part of libprocess library, which > can be shared among these 3 processes, > but there would be separate copies of static/global data of this class > for the 3 processes (only the code segment should be shared). > So, will the timer set by one process, be visible to another process > at all ? > > > CODE: > > Future<Nothing> launch; > EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _)) > .WillOnce(DoAll(FutureSatisfy(&launch), > Return(true))); > > //future object for call to "registerExecutorTimeout" > Future<Nothing> executorRegistrationTimeout = > FUTURE_DISPATCH(_, &Slave::registerExecutorTimeout); > > driver.launchTasks(offers.get()[0].id(), tasks); > > AWAIT_READY(launch); > > Clock::pause(); > > Clock::advance(flags.executor_registration_timeout); > > //ensure that the "registerExecutorTimeout" HAS fired > ASSERT_TRUE(executorRegistrationTimeout.isReady()); > > Clock::resume();
Ok..I have updated the diff with the testcase. I was able to answer my questions. As for question-1), For unit tests, we are running all the components in the same process, rather than spawning one process per component. Essentially, we seem to be relying on a single thread event driven kind of mechanism for unit tests. So, my question 3) becomes irrelevant. For question-2, the trick was to use two futures (a future and a promise), one to track the call of the launch() method, and the 2nd one to trigger the registration timeout. Additionally, I have moved the delay() call from onReady() callback to onAny() callback, since we want the timer to be set, even if the future fails or is dicarded. - Nishant ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29437/#review66171 ----------------------------------------------------------- On Dec. 31, 2014, 3:01 a.m., Nishant Suneja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29437/ > ----------------------------------------------------------- > > (Updated Dec. 31, 2014, 3:01 a.m.) > > > Review request for mesos and Timothy Chen. > > > 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 onAny() 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.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b > src/tests/composing_containerizer_tests.cpp > 5ab5a36cadb7f8622bad0c5814e9a5fb338753ad > src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 > src/tests/slave_tests.cpp f2896a1fc4521452e29fd261a6f117372345dcfc > > Diff: https://reviews.apache.org/r/29437/diff/ > > > Testing > ------- > > Added the unit test : > SlaveTest::AsynchronousExecutorRegistrationTimeoutTrigger > make check succeeds. > > > Thanks, > > Nishant Suneja > >
