Refactored and fixed `DefaultExecutorTest.CommitSuicideOnKillTask`. Make this flaky test more readable, and cleaner by not trying to guess the order in which task status updates will arrive.
Review: https://reviews.apache.org/r/63173/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/5cc37c2c Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/5cc37c2c Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/5cc37c2c Branch: refs/heads/master Commit: 5cc37c2cdf1a4e1ca1c23cbad2764679f24174e1 Parents: 377b684 Author: Gaston Kleiman <gas...@mesosphere.io> Authored: Thu Oct 19 22:55:08 2017 -0700 Committer: Alexander Rukletsov <al...@apache.org> Committed: Thu Oct 19 22:56:59 2017 -0700 ---------------------------------------------------------------------- src/tests/default_executor_tests.cpp | 125 ++++++++++++++++++------------ 1 file changed, 74 insertions(+), 51 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/5cc37c2c/src/tests/default_executor_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/default_executor_tests.cpp b/src/tests/default_executor_tests.cpp index a248aa1..5078bd4 100644 --- a/src/tests/default_executor_tests.cpp +++ b/src/tests/default_executor_tests.cpp @@ -944,34 +944,29 @@ TEST_P(DefaultExecutorTest, CommitSuicideOnKillTask) // The first task finishes successfully while the second // task is explicitly killed later. - v1::TaskInfo taskInfo1 = v1::createTask(agentId, resources, "exit 0"); + v1::TaskInfo task1 = v1::createTask(agentId, resources, "exit 0"); - v1::TaskInfo taskInfo2 = - v1::createTask(agentId, resources, SLEEP_COMMAND(1000)); + v1::TaskInfo task2 = v1::createTask(agentId, resources, SLEEP_COMMAND(1000)); - const hashset<v1::TaskID> tasks{taskInfo1.task_id(), taskInfo2.task_id()}; + const hashset<v1::TaskID> taskIds{task1.task_id(), task2.task_id()}; - Future<v1::scheduler::Event::Update> startingUpdate1; - Future<v1::scheduler::Event::Update> startingUpdate2; - Future<v1::scheduler::Event::Update> runningUpdate1; - Future<v1::scheduler::Event::Update> runningUpdate2; - EXPECT_CALL(*scheduler, update(_, _)) - .WillOnce( - DoAll( - FutureArg<1>(&startingUpdate1), - v1::scheduler::SendAcknowledge(frameworkId, agentId))) - .WillOnce( - DoAll( - FutureArg<1>(&startingUpdate2), - v1::scheduler::SendAcknowledge(frameworkId, agentId))) - .WillOnce( - DoAll( - FutureArg<1>(&runningUpdate1), - v1::scheduler::SendAcknowledge(frameworkId, agentId))) - .WillOnce( - DoAll( - FutureArg<1>(&runningUpdate2), - v1::scheduler::SendAcknowledge(frameworkId, agentId))); + // We expect two TASK_STARTING, two TASK_RUNNING, and one TASK_FINISHED + // updates. + vector<Future<v1::scheduler::Event::Update>> updates(5); + + { + // This variable doesn't have to be used explicitly. We need it so that the + // futures are satisfied in the order in which the updates are received. + testing::InSequence inSequence; + + foreach (Future<v1::scheduler::Event::Update>& update, updates) { + EXPECT_CALL(*scheduler, update(_, _)) + .WillOnce( + DoAll( + FutureArg<1>(&update), + v1::scheduler::SendAcknowledge(frameworkId, agentId))); + } + } Future<v1::scheduler::Event::Failure> executorFailure; EXPECT_CALL(*scheduler, failure(_, _)) @@ -982,49 +977,77 @@ TEST_P(DefaultExecutorTest, CommitSuicideOnKillTask) frameworkId, offer, {v1::LAUNCH_GROUP( - executorInfo, v1::createTaskGroupInfo({taskInfo1, taskInfo2}))})); + executorInfo, v1::createTaskGroupInfo({task1, task2}))})); - AWAIT_READY(startingUpdate1); - ASSERT_EQ(TASK_STARTING, startingUpdate1->status().state()); + enum class Stage + { + INITIAL, + STARTING, + RUNNING, + FINISHED + }; - // We only check the first and last update, because the other two might - // arrive in a different order. + hashmap<v1::TaskID, Stage> taskStages; + taskStages[task1.task_id()] = Stage::INITIAL; + taskStages[task2.task_id()] = Stage::INITIAL; - AWAIT_READY(runningUpdate2); - ASSERT_EQ(TASK_RUNNING, runningUpdate2->status().state()); + foreach (Future<v1::scheduler::Event::Update>& update, updates) { + AWAIT_READY(update); - // When running a task, TASK_RUNNING updates for the tasks in a - // task group can be received in any order. - const hashset<v1::TaskID> tasksRunning{ - startingUpdate1->status().task_id(), - startingUpdate2->status().task_id(), - runningUpdate1->status().task_id(), - runningUpdate2->status().task_id()}; + const v1::TaskStatus& taskStatus = update->status(); - ASSERT_EQ(tasks, tasksRunning); + Option<Stage> taskStage = taskStages.get(taskStatus.task_id()); + ASSERT_SOME(taskStage); - Future<v1::scheduler::Event::Update> finishedUpdate; - EXPECT_CALL(*scheduler, update(_, _)) - .WillOnce(FutureArg<1>(&finishedUpdate)); + switch (taskStage.get()) { + case Stage::INITIAL: { + ASSERT_EQ(TASK_STARTING, taskStatus.state()); - AWAIT_READY(finishedUpdate); - ASSERT_EQ(TASK_FINISHED, finishedUpdate->status().state()); - ASSERT_EQ(taskInfo1.task_id(), finishedUpdate->status().task_id()); + taskStages[taskStatus.task_id()] = Stage::STARTING; + + break; + } + case Stage::STARTING: { + ASSERT_EQ(TASK_RUNNING, taskStatus.state()); + + taskStages[taskStatus.task_id()] = Stage::RUNNING; + + break; + } + case Stage::RUNNING: { + ASSERT_EQ(TASK_FINISHED, taskStatus.state()); + + taskStages[taskStatus.task_id()] = Stage::FINISHED; + + break; + } + case Stage::FINISHED: { + FAIL() << "Unexpected task update: " << update->DebugString(); + break; + } + } + } + + // `task1` should have finished, `task2` should still be running. + ASSERT_EQ(Stage::FINISHED, taskStages[task1.task_id()]); + ASSERT_EQ(Stage::RUNNING, taskStages[task2.task_id()]); - // The executor should still be alive after the task - // has finished successfully. + // The executor should still be alive after task1 has finished successfully. ASSERT_TRUE(executorFailure.isPending()); Future<v1::scheduler::Event::Update> killedUpdate; EXPECT_CALL(*scheduler, update(_, _)) - .WillOnce(FutureArg<1>(&killedUpdate)); + .WillOnce( + DoAll( + FutureArg<1>(&killedUpdate), + v1::scheduler::SendAcknowledge(frameworkId, agentId))); // Now kill the second task in the task group. - mesos.send(v1::createCallKill(frameworkId, taskInfo2.task_id())); + mesos.send(v1::createCallKill(frameworkId, task2.task_id())); AWAIT_READY(killedUpdate); ASSERT_EQ(TASK_KILLED, killedUpdate->status().state()); - ASSERT_EQ(taskInfo2.task_id(), killedUpdate->status().task_id()); + ASSERT_EQ(task2.task_id(), killedUpdate->status().task_id()); // The executor should commit suicide after the task is killed. AWAIT_READY(executorFailure);
