----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45040/#review124343 -----------------------------------------------------------
Thanks for the test! Although I wouldn't want to ship this unless you show some manual testing in the 'testing done' section, since this test doesn't exercise the functionality :) src/tests/slave_tests.cpp (line 3372) <https://reviews.apache.org/r/45040/#comment186842> No need for the NOTE (per the previous test review feedback). src/tests/slave_tests.cpp (lines 3393 - 3431) <https://reviews.apache.org/r/45040/#comment186826> Per the feedback from the other test review, let's pull this up into a test within master_validation_tests.cpp. It can be a trivial unit test since the validation function is stateless :) src/tests/slave_tests.cpp (lines 3419 - 3424) <https://reviews.apache.org/r/45040/#comment186841> You won't need to bother with this once we pull out the validation test, right? src/tests/slave_tests.cpp (lines 3433 - 3477) <https://reviews.apache.org/r/45040/#comment186835> Once you pull out the validation test you won't need this scope anymore. This is just testing that we don't mutate the grace period? Can you add a TODO to the top of this test to mention that we should also actually test that it does what it is supposed to do? Did you manually test this? I wouldn't feel comfortable shipping this unless you did, since this test doesn't cover the signal escalation behavior :) src/tests/slave_tests.cpp (lines 3442 - 3444) <https://reviews.apache.org/r/45040/#comment186840> You shouldn't need to do this once the validation test is pulled out, right? src/tests/slave_tests.cpp (lines 3466 - 3476) <https://reviews.apache.org/r/45040/#comment186838> It doesn't look like you need to bother sending a status update here, just use FutureArg instead of SaveArg and wait for the argument to arrive. In general we try to avoid SaveArg since it doesn't indicate when the value arrives. src/tests/slave_tests.cpp (line 3474) <https://reviews.apache.org/r/45040/#comment186836> Please sweep for s/.get()./->/ src/tests/slave_tests.cpp (line 3482) <https://reviews.apache.org/r/45040/#comment186837> Why the resume here? - Ben Mahler On March 18, 2016, 5:17 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45040/ > ----------------------------------------------------------- > > (Updated March 18, 2016, 5:17 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4909 > https://issues.apache.org/jira/browse/MESOS-4909 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 > > Diff: https://reviews.apache.org/r/45040/diff/ > > > Testing > ------- > > On Mac OS 10.104: > `make check` > `GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 > --gtest_break_on_failure` > > > Thanks, > > Alexander Rukletsov > >
