----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66230/#review202533 -----------------------------------------------------------
Can we simplify these tests and remove the executors? Also, over the course of reviewing this I realized that role removal should rescind offers for the removed roles and I don't think these patches address that? src/tests/scheduler_tests.cpp Lines 1938 (patched) <https://reviews.apache.org/r/66230/#comment284346> Are you setting this in order to test a non-whitelisted role? If so, maybe a short comment here? ``` // Set the whitelist in order to test a non-whitelisted role // is considered invalid. ``` src/tests/scheduler_tests.cpp Lines 1946-1949 (patched) <https://reviews.apache.org/r/66230/#comment284350> Why do you need an executor and test containerizer for this test? Can we avoid it? src/tests/scheduler_tests.cpp Lines 1951-1956 (patched) <https://reviews.apache.org/r/66230/#comment284349> Are you using this to test that the update makes its way to the allocator? Maybe a short note? ``` // We add an agent in order to test the framework update // makes its way to the allocator. ``` src/tests/scheduler_tests.cpp Lines 2201 (patched) <https://reviews.apache.org/r/66230/#comment284345> End with a period? Also, sometimes the comments quote the roles and sometimes they don't quote them (like here), can you make it consistent? src/tests/scheduler_tests.cpp Lines 2201-2221 (patched) <https://reviews.apache.org/r/66230/#comment284351> This should probably rescind offers to the removed role, can we test that? I'm guessing that's a bug in the call implementation, can you address it in a subsequent patch? - Benjamin Mahler On May 4, 2018, 5:33 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66230/ > ----------------------------------------------------------- > > (Updated May 4, 2018, 5:33 p.m.) > > > Review request for mesos, Benjamin Mahler and Till Toenshoff. > > > Bugs: MESOS-7258 > https://issues.apache.org/jira/browse/MESOS-7258 > > > Repository: mesos > > > Description > ------- > > Added test for adding/removing framework roles. > > > Diffs > ----- > > src/tests/scheduler_tests.cpp 749420a23bc1a3873bd4d5aee56e78cff79bb1af > > > Diff: https://reviews.apache.org/r/66230/diff/8/ > > > Testing > ------- > > > Thanks, > > Kapil Arya > >
