----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58951/#review176742 -----------------------------------------------------------
src/tests/reservation_endpoints_tests.cpp Line 374 (original), 349 (patched) <https://reviews.apache.org/r/58951/#comment250195> Pass `masterFlags`? src/tests/reservation_endpoints_tests.cpp Line 442 (original), 413 (patched) <https://reviews.apache.org/r/58951/#comment250196> Do you want to update the comment? src/tests/reservation_endpoints_tests.cpp Lines 419 (patched) <https://reviews.apache.org/r/58951/#comment250197> Is this change related to removal of `TestAllocator` or is it a general improvement? If it is the latter, then please do it in a separate patch. src/tests/reservation_endpoints_tests.cpp Lines 460-474 (original), 431-443 (patched) <https://reviews.apache.org/r/58951/#comment250198> Do we need to check both `StatusUpdateMessage` and delivery of status update to the scheduler? Instead, does it maybe make sense to additionaly check the task state of the task status update delivered to the scheduler? src/tests/reservation_endpoints_tests.cpp Lines 479-482 (original), 448-450 (patched) <https://reviews.apache.org/r/58951/#comment250199> Could you please explain why you are doing this change? src/tests/reservation_endpoints_tests.cpp Lines 570-571 (original), 530-532 (patched) <https://reviews.apache.org/r/58951/#comment250200> Ditto. Separate patch? src/tests/reservation_endpoints_tests.cpp Line 609 (original), 565 (patched) <https://reviews.apache.org/r/58951/#comment250201> Update the comment as well? src/tests/reservation_endpoints_tests.cpp Lines 628-639 (original), 583-590 (patched) <https://reviews.apache.org/r/58951/#comment250202> Let's capture the status update message and check the state is `TASK_KILLED`. src/tests/reservation_endpoints_tests.cpp Lines 640-641 (original) <https://reviews.apache.org/r/58951/#comment250203> This looks like a valuable check. Do we have another test that ensures a similar thing? src/tests/reservation_endpoints_tests.cpp Lines 646-648 (original), 595-597 (patched) <https://reviews.apache.org/r/58951/#comment250204> Same question here: why this change in this patch? src/tests/reservation_endpoints_tests.cpp Lines 1403 (patched) <https://reviews.apache.org/r/58951/#comment250205> You don't necessarily need this here. Actually it might even seem misleading because people may think it is doing something. - Alexander Rukletsov On May 30, 2017, 10:22 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58951/ > ----------------------------------------------------------- > > (Updated May 30, 2017, 10:22 a.m.) > > > Review request for mesos, Alexander Rukletsov and Benjamin Mahler. > > > Bugs: MESOS-7388 > https://issues.apache.org/jira/browse/MESOS-7388 > > > Repository: mesos > > > Description > ------- > > The tests in the case often require an agent ID. To obtain the ID they > were using a mock allocator to grab agent ID, but not other operations > with the allocator were performed. > > Instead we now just capture the SlaveRegisteredMessage in order to > learn an agent's ID. > > > Diffs > ----- > > src/tests/reservation_endpoints_tests.cpp > 8c195eb7d788fbca8722d66d81c77d399702160a > > > Diff: https://reviews.apache.org/r/58951/diff/2/ > > > Testing > ------- > > `make check`, also in repetition. > > While this patch is part of this series since later patches depend on it, it > could be commited independently. > > > Thanks, > > Benjamin Bannier > >
