You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/01/25 23:58:36 UTC

Review Request 42751: Tweaked some resource test cases.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/
-----------------------------------------------------------

Review request for mesos.


Repository: mesos


Description
-------

We should check that two reservations with the same role but different
principals are considered distinct.


Diffs
-----

  src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 

Diff: https://reviews.apache.org/r/42751/diff/


Testing
-------


Thanks,

Neil Conway


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.

> On Jan. 26, 2016, 6:37 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 1603
> > <https://reviews.apache.org/r/42751/diff/1/?file=1220217#file1220217line1603>
> >
> >     Why do you update here?

The goal of this test is to validate that two resources with different `ReservationInfo` are treated as distinct (not equal). If the roles are also different, then the resources will be considered distinct for that reason instead.


> On Jan. 26, 2016, 6:37 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, lines 1672-1673
> > <https://reviews.apache.org/r/42751/diff/1/?file=1220217#file1220217line1672>
> >
> >     Is this useful? I think that we already covered the `same` contain case in `EXPECT_TRUE((r1 + r2).contains(r1 + r2));`

I think it is useful, yes: the checks involving the `+` operator depend on `operator+` being implemented correctly.


- Neil


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review116289
-----------------------------------------------------------


On Jan. 25, 2016, 10:59 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2016, 10:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review116289
-----------------------------------------------------------




src/tests/resources_tests.cpp (line 1603)
<https://reviews.apache.org/r/42751/#comment177313>

    Why do you update here?



src/tests/resources_tests.cpp (lines 1672 - 1673)
<https://reviews.apache.org/r/42751/#comment177314>

    Is this useful? I think that we already covered the `same` contain case in `EXPECT_TRUE((r1 + r2).contains(r1 + r2));`


- Guangya Liu


On 一月 25, 2016, 10:59 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated 一月 25, 2016, 10:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117743
-----------------------------------------------------------




src/tests/resources_tests.cpp (line 185)
<https://reviews.apache.org/r/42751/#comment179025>

    Let's fix this with `Resources::size()` as well :)


- Michael Park


On Feb. 3, 2016, 11:04 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2016, 11:04 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Michael Park <mp...@apache.org>.

> On Feb. 5, 2016, 2:33 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, lines 327-328
> > <https://reviews.apache.org/r/42751/diff/4/?file=1235800#file1235800line327>
> >
> >     Just a question, do we have some guidelines for when to use `ASSERT_EQ` and when to use `EXPECT_EQ` ?

We use `ASSERT_*` in cases where the violation of such condition invalidates the rest of the test. For example, in this case, if `cpus->type()` is not a `Value::SCALAR`, then we violate `cpus->scalar()`'s preconditions and therefore is invalid. We use `EXPECT_*` if the condition is one that we want to test and, if even it fails, it doesn't mean the rest of the test is non-sensical.

Take for example:

```
std::vector<int> x = f(0, 1, 2);

// `ASSERT_*` here since if we have less than 3 elements, `x[2]` is invalid.
ASSERT_GE(x.size(), 3);

// Each of these are independent test cases.
// For example, even if x[0] != 0, it doesn't invalidate the rest of our tests.
EXPECT_EQ(0, x[0]);
EXPECT_EQ(1, x[1]);
EXPECT_EQ(2, x[2]);
```


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117946
-----------------------------------------------------------


On Feb. 5, 2016, 2:19 a.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2016, 2:19 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Guangya Liu <gy...@gmail.com>.

> On 二月 5, 2016, 2:33 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, lines 327-328
> > <https://reviews.apache.org/r/42751/diff/4/?file=1235800#file1235800line327>
> >
> >     Just a question, do we have some guidelines for when to use `ASSERT_EQ` and when to use `EXPECT_EQ` ?
> 
> Michael Park wrote:
>     We use `ASSERT_*` in cases where the violation of such condition invalidates the rest of the test. For example, in this case, if `cpus->type()` is not a `Value::SCALAR`, then we violate `cpus->scalar()`'s preconditions and therefore is invalid. We use `EXPECT_*` if the condition is one that we want to test and, if even it fails, it doesn't mean the rest of the test is non-sensical.
>     
>     Take for example:
>     
>     ```
>     std::vector<int> x = f(0, 1, 2);
>     
>     // `ASSERT_*` here since if we have less than 3 elements, `x[2]` is invalid.
>     ASSERT_GE(x.size(), 3);
>     
>     // Each of these are independent test cases.
>     // For example, even if x[0] != 0, it doesn't invalidate the rest of our tests.
>     EXPECT_EQ(0, x[0]);
>     EXPECT_EQ(1, x[1]);
>     EXPECT_EQ(2, x[2]);
>     ```

Thanks Michael for the explanation, clear now.


- Guangya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117946
-----------------------------------------------------------


On 二月 5, 2016, 2:19 a.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated 二月 5, 2016, 2:19 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117946
-----------------------------------------------------------




src/tests/resources_tests.cpp (lines 326 - 327)
<https://reviews.apache.org/r/42751/#comment179238>

    Just a question, do we have some guidelines for when to use `ASSERT_EQ` and when to use `EXPECT_EQ` ?


- Guangya Liu


On 二月 5, 2016, 2:19 a.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated 二月 5, 2016, 2:19 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/
-----------------------------------------------------------

(Updated Feb. 5, 2016, 2:19 a.m.)


Review request for mesos and Michael Park.


Repository: mesos


Description
-------

We should check that two reservations with the same role but different
principals are considered distinct.


Diffs (updated)
-----

  src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 

Diff: https://reviews.apache.org/r/42751/diff/


Testing
-------

make check


Thanks,

Neil Conway


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/
-----------------------------------------------------------

(Updated Feb. 3, 2016, 11:04 p.m.)


Review request for mesos and Michael Park.


Changes
-------

Code review from mpark.


Repository: mesos


Description
-------

We should check that two reservations with the same role but different
principals are considered distinct.


Diffs (updated)
-----

  src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 

Diff: https://reviews.apache.org/r/42751/diff/


Testing
-------

make check


Thanks,

Neil Conway


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Michael Park <mp...@apache.org>.

> On Feb. 3, 2016, 10:45 p.m., Michael Park wrote:
> > src/tests/resources_tests.cpp, line 957
> > <https://reviews.apache.org/r/42751/diff/2/?file=1231620#file1231620line957>
> >
> >     We used to have a `Resources::size()` function which essentially did this, but intentionally removed it so that people don't rely on number of `Resource` instances. Is there a reason why we want to check for this?
> >     
> >     Here and below.
> 
> Neil Conway wrote:
>     The # of `Resource` instances is part of the public API of `Resources` (e.g., clients can iterate over every `Resource`). If it is part of the public API, it seems like something it would be worth testing.
>     
>     In this particular case, it doesn't matter that much, but in other test cases (e.g., `AdditionDynamicallyReservedWithDistinctLabels`) it seems useful to check.

Ok, synced with Jie on this as well. Let's re-introduce `Resources::size()` and use that instead.

I agree that the # of `Resource` instances is part of the public API since as you say, one can iterate and count.
However, this still does not mean that such iterator math works. For example, we could internally store the `Resource` objects
in a `std::set` (or any other data structure that does not guarantee contiguous memory), rather than `std::vector`.

Providing a `Resources::size()` would more accurately capture this intention anyway. Would you be ok taking that on?


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117698
-----------------------------------------------------------


On Feb. 3, 2016, 11:04 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2016, 11:04 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.

> On Feb. 3, 2016, 10:45 p.m., Michael Park wrote:
> > src/tests/resources_tests.cpp, line 957
> > <https://reviews.apache.org/r/42751/diff/2/?file=1231620#file1231620line957>
> >
> >     We used to have a `Resources::size()` function which essentially did this, but intentionally removed it so that people don't rely on number of `Resource` instances. Is there a reason why we want to check for this?
> >     
> >     Here and below.

The # of `Resource` instances is part of the public API of `Resources` (e.g., clients can iterate over every `Resource`). If it is part of the public API, it seems like something it would be worth testing.

In this particular case, it doesn't matter that much, but in other test cases (e.g., `AdditionDynamicallyReservedWithDistinctLabels`) it seems useful to check.


> On Feb. 3, 2016, 10:45 p.m., Michael Park wrote:
> > src/tests/resources_tests.cpp, line 1603
> > <https://reviews.apache.org/r/42751/diff/2/?file=1231620#file1231620line1603>
> >
> >     Can we just add the (same role, different principal) case rather than replacing it with the (different role, different principal) case?
> >     
> >     For that matter, can we also add the (different role, same principal) case? This will be relevant once we have multi-role frameworks I think, right?

Sure thing. At some point it probably doesn't make sense to test the entire cross-product of different resource properties, but for now it should be fine.


- Neil


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117698
-----------------------------------------------------------


On Feb. 3, 2016, 5:51 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2016, 5:51 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.

> On Feb. 3, 2016, 10:45 p.m., Michael Park wrote:
> > src/tests/resources_tests.cpp, line 957
> > <https://reviews.apache.org/r/42751/diff/2/?file=1231620#file1231620line957>
> >
> >     We used to have a `Resources::size()` function which essentially did this, but intentionally removed it so that people don't rely on number of `Resource` instances. Is there a reason why we want to check for this?
> >     
> >     Here and below.
> 
> Neil Conway wrote:
>     The # of `Resource` instances is part of the public API of `Resources` (e.g., clients can iterate over every `Resource`). If it is part of the public API, it seems like something it would be worth testing.
>     
>     In this particular case, it doesn't matter that much, but in other test cases (e.g., `AdditionDynamicallyReservedWithDistinctLabels`) it seems useful to check.
> 
> Michael Park wrote:
>     Ok, synced with Jie on this as well. Let's re-introduce `Resources::size()` and use that instead.
>     
>     I agree that the # of `Resource` instances is part of the public API since as you say, one can iterate and count.
>     However, this still does not mean that such iterator math works. For example, we could internally store the `Resource` objects
>     in a `std::set` (or any other data structure that does not guarantee contiguous memory), rather than `std::vector`.
>     
>     Providing a `Resources::size()` would more accurately capture this intention anyway. Would you be ok taking that on?

Sounds good.


- Neil


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117698
-----------------------------------------------------------


On Feb. 3, 2016, 11:04 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2016, 11:04 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117698
-----------------------------------------------------------


Fix it, then Ship it!





src/tests/resources_tests.cpp (line 957)
<https://reviews.apache.org/r/42751/#comment178972>

    We used to have a `Resources::size()` function which essentially did this, but intentionally removed it so that people don't rely on number of `Resource` instances. Is there a reason why we want to check for this?
    
    Here and below.



src/tests/resources_tests.cpp (line 1603)
<https://reviews.apache.org/r/42751/#comment178973>

    Can we just add the (same role, different principal) case rather than replacing it with the (different role, different principal) case?
    
    For that matter, can we also add the (different role, same principal) case? This will be relevant once we have multi-role frameworks I think, right?


- Michael Park


On Feb. 3, 2016, 5:51 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2016, 5:51 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/
-----------------------------------------------------------

(Updated Feb. 3, 2016, 5:51 p.m.)


Review request for mesos and Michael Park.


Changes
-------

Fixes per Joerg's review.


Repository: mesos


Description
-------

We should check that two reservations with the same role but different
principals are considered distinct.


Diffs (updated)
-----

  src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a 

Diff: https://reviews.apache.org/r/42751/diff/


Testing
-------

make check


Thanks,

Neil Conway


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.

> On Feb. 3, 2016, 5:34 p.m., Joerg Schad wrote:
> > src/tests/resources_tests.cpp, line 957
> > <https://reviews.apache.org/r/42751/diff/1/?file=1220217#file1220217line957>
> >
> >     I must say this pattern (distance(begin,end)) just to get the item count looks unintuitive here.
> >     I see that is currently the easiest way given Resources, but maybe it makes sense to add a count() or similar function to Resources (Not a real issue for me for this review).

Yeah, I agree it isn't ideal. I didn't want to change the `Resources` API just for a few test cases, but if other places would find the count useful, it would make sense to add support for it in a more intuitive way.


- Neil


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117635
-----------------------------------------------------------


On Jan. 25, 2016, 10:59 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2016, 10:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/#review117635
-----------------------------------------------------------


Fix it, then Ship it!





src/tests/resources_tests.cpp (line 327)
<https://reviews.apache.org/r/42751/#comment178877>

    if you are checking and relying on the outcome (i.e. next line) shouldn't this be an Assert_EQ?



src/tests/resources_tests.cpp (line 957)
<https://reviews.apache.org/r/42751/#comment178880>

    I must say this pattern (distance(begin,end)) just to get the item count looks unintuitive here.
    I see that is currently the easiest way given Resources, but maybe it makes sense to add a count() or similar function to Resources (Not a real issue for me for this review).


- Joerg Schad


On Jan. 25, 2016, 10:59 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2016, 10:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -----
> 
>   src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


Re: Review Request 42751: Tweaked some resource test cases.

Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42751/
-----------------------------------------------------------

(Updated Jan. 25, 2016, 10:59 p.m.)


Review request for mesos and Michael Park.


Repository: mesos


Description
-------

We should check that two reservations with the same role but different
principals are considered distinct.


Diffs
-----

  src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 

Diff: https://reviews.apache.org/r/42751/diff/


Testing (updated)
-------

make check


Thanks,

Neil Conway