----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5532/#review8779 -----------------------------------------------------------
This is cleaning up really nicely! src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18570> Is this Return() necessary? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18571> Is this Return() necessary? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18564> Hmm, I didn't fully get this comment. I think you want to say that the first time you _don't_ invoke resourcesRecovered, but you _wait_ until frameworkRemoved is called to test that doing the dispatch after frameworkRemoved is okay (more or less parroting what the comment above the test says). src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18566> Again, remind the reader of the test that we explicitly do this dispatch on behalf of the master to make sure that the allocator can handle getting resources recovered after the framework has been removed. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18565> Wrap this line please. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18568> '{' on newline. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18577> So, I think the right thing to do here is make this protected not public. In addition, I think you want to add a static void SetUpTestCase() here that just calls BaseZooKeeperTest::SetUpTestCase(), and make BaseZooKeeperTest::SetUpTestCase be protected again. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18567> '{' on new line. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18569> Please use braces. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18572> Please use braces. src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18573> Is .Times(2) necessary if you have two WillOnce? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18574> Sometimes you do .WillRepeatedly(Return()) and sometimes you do .Times(AnyNumber()). Are there semantic differences that keep you from being consistent and just picking one? src/tests/allocator_tests.cpp <https://reviews.apache.org/r/5532/#comment18575> Braces please. But instead of this if check (and the ones above), please turn these all into CHECK(!someTry.isError()) << "error message" << someTry.error(); src/tests/utils.hpp <https://reviews.apache.org/r/5532/#comment18578> I love this factored out and useable by others, but I don't love implicitly passing arguments through environment variables. How hard would it be to explicitly pass the arguments to this function? As in, make the signature something like: launchTask(SchedulerDriver* driver, const std::vector<Offer>& offers, double cpus, double mem) And maybe even take the total number of tasks to launch? src/tests/utils.hpp <https://reviews.apache.org/r/5532/#comment18576> driver->declineOffer(offer.id()); - Benjamin Hindman On June 28, 2012, 10:19 p.m., Thomas Marshall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5532/ > ----------------------------------------------------------- > > (Updated June 28, 2012, 10:19 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/ > > > This addresses bug MESOS-224. > https://issues.apache.org/jira/browse/MESOS-224 > > > Diffs > ----- > > src/Makefile.am 8760f59 > src/common/type_utils.hpp acf3e65 > 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 > src/tests/utils.cpp e7cda40 > > Diff: https://reviews.apache.org/r/5532/diff/ > > > Testing > ------- > > make check on Lion > > > Thanks, > > Thomas Marshall > >
