----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5532/#review8549 -----------------------------------------------------------
src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18210> Newline. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18205> s/MyTypes/AllocatorTypes/ src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18206> This will be great for future allocators! But maybe add a comment explaining that this is instantiating tests for each of the types in 'AllocatorTypes' so future people know exactly what to do. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18209> Newline. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18213> If the 'this->' isn't actually necessary we should remove it (here and everywhere else). src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18223> On previous line please (here and everywhere else). src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18224> Why do you need to invoke this directly? A comment here (and anywhere else you're doing something like this) would be very useful. Also, this isn't "thread" safe, you'll need to dispatch here if you actually need to do this. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18226> Ah, do these need to be done via a DoDefault action in the EXPECT_CALL's above? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18227> I'd like us to consider an alternative to TestScheduler. If you think the code in TestScheduler::resourceOffers is too much, let's create a helper function (maybe in utils) that given some offers and list of tasks does the necessary driver->launchTasks. That will be useful other places too! In addition, we will be able to kill the TestScheduler completely. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18211> An abbreviated version of the comment above would be great! src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18208> Newline. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18222> Fix whitespace problems please. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18212> Newline. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18216> How about we move this test to be the first one in the file? ;) src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18217> Isn't this just DoDefault? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18218> DoDefault? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18219> DoDefault? src/tests/base_zookeeper_test.hpp <https://reviews.apache.org/r/5532/#comment18207> Was SetUpTestCase not protected? - Benjamin Hindman On June 22, 2012, 9:51 p.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5532/ > ----------------------------------------------------------- > > (Updated June 22, 2012, 9:51 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Description > ------- > > Added tests for allocators. > > This patch depends on two other currently pending review requests: > https://reviews.apache.org/r/5448/ > https://reviews.apache.org/r/5451/ > > > Diffs > ----- > > src/Makefile.am 8760f59 > src/tests/allocator_tests.cpp PRE-CREATION > src/tests/base_zookeeper_test.hpp 4613376 > src/tests/resource_offers_tests.cpp c1f1760 > src/tests/utils.hpp e81ec82 > > Diff: https://reviews.apache.org/r/5532/diff/ > > > Testing > ------- > > make check on Lion > > > Thanks, > > Thomas Marshall > >
