----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59545/#review176650 -----------------------------------------------------------
Sorry, I couldn't resist a few pedantic coding style suggestions... I also realize that I'm complaining about things that occur in other tests as well :) src/tests/slave_tests.cpp Lines 7255 (patched) <https://reviews.apache.org/r/59545/#comment250049> Do we actually expect subsequent offers in the test, or is this just coding defensively? (If the latter, I'm not sure it is good practice, since it might hide bugs or unexpected behavioral changes.) src/tests/slave_tests.cpp Lines 7259 (patched) <https://reviews.apache.org/r/59545/#comment250046> Do we actually need to wait for a batch allocation here? IMO it would be clearer to first prompt the agent to register (before connecting the framework); then when the framework registers, it should immediately receive an offer. src/tests/slave_tests.cpp Lines 7263 (patched) <https://reviews.apache.org/r/59545/#comment250051> Style-wise, I'd prefer `ASSERT_FALSE(offers.empty())`. src/tests/slave_tests.cpp Lines 7265 (patched) <https://reviews.apache.org/r/59545/#comment250050> Style-wise, I'd prefer `offers->front()`. src/tests/slave_tests.cpp Lines 7322 (patched) <https://reviews.apache.org/r/59545/#comment250053> Won't be marked `TASK_UNREACHABLE` because the framework is not PA; arguably confusing to suggest that in the comment. src/tests/slave_tests.cpp Lines 7335 (patched) <https://reviews.apache.org/r/59545/#comment250056> As above, if we don't expect additional status updates, not sure it is good practice to add the `WillRepeatedly` expectation. src/tests/slave_tests.cpp Lines 7340 (patched) <https://reviews.apache.org/r/59545/#comment250048> I'd argue for `EXPECT` here, not `ASSERT`. - Neil Conway On May 30, 2017, 11:29 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59545/ > ----------------------------------------------------------- > > (Updated May 30, 2017, 11:29 p.m.) > > > Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone. > > > Bugs: MESOS-7540 > https://issues.apache.org/jira/browse/MESOS-7540 > > > Repository: mesos > > > Description > ------- > > This patch adds a test to verify the correct behavior of > the agent flag 'executor_reregistration_timeout. > > > Diffs > ----- > > src/tests/slave_tests.cpp 927b9c3fbc3373b49c799da6e3f770b73964f9e5 > > > Diff: https://reviews.apache.org/r/59545/diff/5/ > > > Testing > ------- > > `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" > --gtest_repeat=-1 --gtest_break_on_failure` > > > Thanks, > > Greg Mann > >
