----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55868/#review163524 -----------------------------------------------------------
src/tests/hierarchical_allocator_tests.cpp (line 85) <https://reviews.apache.org/r/55868/#comment234984> This should just be a plain constructor, right? src/tests/hierarchical_allocator_tests.cpp (lines 289 - 293) <https://reviews.apache.org/r/55868/#comment234985> If we made `Allocation::create` just a constructor, it should be possible to create `Allocation`s in test assertions, e.g., AWAIT_EXPECT_EQ(Allocation(framework1.id(), {slave1.id(), slave1.resources()}), allocations.get()); Here and below. This should also help remove instances where `expected` is reused to hold completely different values. What do you think? src/tests/hierarchical_allocator_tests.cpp (lines 3367 - 3368) <https://reviews.apache.org/r/55868/#comment234986> This does look like Google style while we seem to prefer passing a non-`const` ref in such instances. Otherwise lets at least `CHECK_NOTNULL`. - Benjamin Bannier On Jan. 24, 2017, 3:31 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55868/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2017, 3:31 a.m.) > > > Review request for mesos and Michael Park. > > > Repository: mesos > > > Description > ------- > > This was necessary to greatly simplify the changes needed to the > allocator tests as we introduce support for multi-role frameworks. > > The main improvement here is to establish and use equality on the > `Allocation` struct, which makes the tests more readable and avoids > the manual probing of the allocation structure across all the tests. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 > > Diff: https://reviews.apache.org/r/55868/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Mahler > >
