mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 55868: Cleanups to the allocator tests.
Date Mon, 30 Jan 2017 20:15:52 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message