----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29748/#review76762 -----------------------------------------------------------
Great job wiring up tests! I've left some comments, some of them are generic and applied to all your tests, but appear in random tests : ). So please if you accept any of those, do a grep over all your other reservation tests. Also I think we should schedule a quick offline sync about framework and operator reservations tests. src/Makefile.am <https://reviews.apache.org/r/29748/#comment124428> Is one tab missing? src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124516> Do we really want to do these tests for all allocator types? src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124522> Does it make sense to check we can unreserve prior launching anything? src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124535> Shouldn't we `driver.start();` here between setting an expectation and awaiting offers? Ditto above and below. src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124523> This looks shady: why do we send TASK_FINISHED more than one time? src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124524> What do you check with this expectation? src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124525> Can we merge this test with the previous one? src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124537> Ensure `frameworkId` is set by awaiting. Ditto above and below. src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124536> Again, do we need to set this explicitly? src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124533> Do you need to set this expectation explicitly? `TestAllocator`'s c-tor sets it. src/tests/reservation_tests.cpp <https://reviews.apache.org/r/29748/#comment124538> Let's adjust the resources to be not greater than available resources, in order we test what we want : ). - Alexander Rukletsov On March 17, 2015, 4:10 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29748/ > ----------------------------------------------------------- > > (Updated March 17, 2015, 4:10 a.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. > > > Bugs: MESOS-2489 > https://issues.apache.org/jira/browse/MESOS-2489 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/Makefile.am 3059818231c46484039d179cd6916932eff6cd68 > src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8 > src/tests/reservation_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/29748/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
