----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57883/#review170030 -----------------------------------------------------------
There might be some comments I would have missed that apply to the second test too. When modifying the second test, please ensure that you fix the second test too. src/tests/slave_tests.cpp Lines 5116 (patched) <https://reviews.apache.org/r/57883/#comment242766> How about: ``` This test verifies that TASK_FAILED updates are sent correctly for all the tasks in a task group when secret generation fails. ``` src/tests/slave_tests.cpp Lines 5125 (patched) <https://reviews.apache.org/r/57883/#comment242767> Move it to after L5138 closer to where it's used. Also, might need to change to `Owned` as per my last review comment. src/tests/slave_tests.cpp Lines 5127-5128 (patched) <https://reviews.apache.org/r/57883/#comment242771> Can you use the v1 classes/helpers that we recently added? I am already working on doing a sweep for moving the others tests in this file to use them similar to what we did for `default_executor_tests.cpp`. Hopefully, we can land them together. This would reduce the `evolve` calls needed in this test and make it more readable. e.g., `v1::Resources`, `v1::ExecutorInfo` `v1::createTask` etc. src/tests/slave_tests.cpp Lines 5167 (patched) <https://reviews.apache.org/r/57883/#comment242768> // Ignore subsequent offers. src/tests/slave_tests.cpp Lines 5189 (patched) <https://reviews.apache.org/r/57883/#comment242774> s/EXPECT_NE/ASSERT_NE It would crash on the next line otherwise when you access offers[0]. src/tests/slave_tests.cpp Lines 5191-5228 (patched) <https://reviews.apache.org/r/57883/#comment242772> Move this after L5241. src/tests/slave_tests.cpp Lines 5208 (patched) <https://reviews.apache.org/r/57883/#comment242777> Add a comment here for posterity that the tasks fail due to the secret generation failing? src/tests/slave_tests.cpp Lines 5215 (patched) <https://reviews.apache.org/r/57883/#comment242784> Kill this newline? src/tests/slave_tests.cpp Lines 5219 (patched) <https://reviews.apache.org/r/57883/#comment242769> Why do you need this? src/tests/slave_tests.cpp Lines 5220 (patched) <https://reviews.apache.org/r/57883/#comment242770> New line before. Also, can we wait on the `Future` for failure? It's not immediately clear to me if by the time `removeFramework` is invoked `failure()` would be invoked once. The test might be flaky otherwise. src/tests/slave_tests.cpp Lines 5267 (patched) <https://reviews.apache.org/r/57883/#comment242782> 4 space indent. src/tests/slave_tests.cpp Lines 5269 (patched) <https://reviews.apache.org/r/57883/#comment242783> s/EXPECT_EQ/ASSERT_EQ Do you want the expectation on L5272 to run otherwise? src/tests/slave_tests.cpp Lines 5277-5301 (patched) <https://reviews.apache.org/r/57883/#comment242781> Kill this? You are not expecting more status updates for the same task. src/tests/slave_tests.cpp Lines 5310-5311 (patched) <https://reviews.apache.org/r/57883/#comment242786> Reword as per the comments above? src/tests/slave_tests.cpp Lines 5319 (patched) <https://reviews.apache.org/r/57883/#comment242788> Move this. src/tests/slave_tests.cpp Lines 5321-5322 (patched) <https://reviews.apache.org/r/57883/#comment242789> Use v1 helpers as per earlier comment. src/tests/slave_tests.cpp Lines 5383 (patched) <https://reviews.apache.org/r/57883/#comment242797> ASSERT_NE src/tests/slave_tests.cpp Lines 5385-5401 (patched) <https://reviews.apache.org/r/57883/#comment242790> Move this before sending the `ACCEPT` call as per my earlier comment. src/tests/slave_tests.cpp Lines 5403 (patched) <https://reviews.apache.org/r/57883/#comment242793> Also add a comment on why the task failed updates are generated? src/tests/slave_tests.cpp Lines 5406-5409 (patched) <https://reviews.apache.org/r/57883/#comment242792> Newline before. ```cpp authenticationToken.mutable_reference()->set_name(...); authenticationToken.mutable_reference()->set_key(...); ``` src/tests/slave_tests.cpp Lines 5416 (patched) <https://reviews.apache.org/r/57883/#comment242794> Kill this newline src/tests/slave_tests.cpp Lines 5421 (patched) <https://reviews.apache.org/r/57883/#comment242795> newline Also, might want to wait on this `Future` similar to my comment in the earlier test. src/tests/slave_tests.cpp Lines 5470 (patched) <https://reviews.apache.org/r/57883/#comment242799> s/EXPECT_EQ/ASSERT_EQ. src/tests/slave_tests.cpp Lines 5475 (patched) <https://reviews.apache.org/r/57883/#comment242798> Newline after a multi line statement. src/tests/slave_tests.cpp Lines 5480-5504 (patched) <https://reviews.apache.org/r/57883/#comment242796> Kill these - Anand Mazumdar On March 23, 2017, 8:35 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57883/ > ----------------------------------------------------------- > > (Updated March 23, 2017, 8:35 p.m.) > > > Review request for mesos, Anand Mazumdar and Vinod Kone. > > > Bugs: MESOS-6999 > https://issues.apache.org/jira/browse/MESOS-6999 > > > Repository: mesos > > > Description > ------- > > This patch adds new tests, > `SlaveTest.RunTaskGroupFailedSecretGeneration` and > `SlaveTest.RunTaskGroupInvalidExecutorSecret`, to > verify the agent's behavior when generation of the > executor secret fails. > > > Diffs > ----- > > src/tests/slave_tests.cpp c31c670b667240c4876d415aa5cf90cb34273e8a > > > Diff: https://reviews.apache.org/r/57883/diff/2/ > > > Testing > ------- > > `make check` > `bin/mesos-tests.sh > --gtest_filter="*SlaveTest.RunTaskGroupFailedSecretGeneration*:SlaveTest.RunTaskGroupInvalidExecutorSecret*" > --gtest_repeat=-1 --gtest_break_on_failure` was used to verify that the new > tests are not flaky. > > > Thanks, > > Greg Mann > >
