> On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 85 > > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line85> > > > > This should just be a plain constructor, right?
+1 > On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 289-293 > > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line289> > > > > 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? Well, to be fair, this is possible with `Allocation::create` too. > On Jan. 30, 2017, 8:11 a.m., Benjamin Bannier wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 3530-3531 > > <https://reviews.apache.org/r/55868/diff/1/?file=1613285#file1613285line3530> > > > > 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`. The general rule is that pointer to non-`const` is preferred over non-`const` references for function parameters. The reason (as far as I see it) is that we can't tell at the callsite whether an argument is to be modified or not. We can talk about this outside of this review if we want. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55868/#review163524 ----------------------------------------------------------- On Jan. 23, 2017, 6:31 p.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55868/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2017, 6:31 p.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 > >
