Re: Review Request 40995: Added test cases for role behavior.

[Adam B]

> On Dec. 17, 2015, 4:57 p.m., Adam B wrote:
> > src/tests/role_tests.cpp, lines 79-83
> > 
> >
> > `AWAIT_EXPECT_RESPONSE_HEADER_EQ(APPLICATION_JSON, "Content-Type", 
> > response);` all fits on one line, and does the AWAITY_READY(response) as 
> > well as the header EQ check.
> > You'll have to `include ` to get `APPLICATION_JSON`.
> 
> Neil Conway wrote:
> Fixed. To avoid the need to resolve some merge conflicts on rebase, I 
> actually made this fix in https://reviews.apache.org/r/41225/ -- let me know 
> if you'd rather I do the rebase + cleanup.

That's fine.


- Adam


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


On Dec. 17, 2015, 5:54 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 17, 2015, 5:54 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and 
> Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e6d48dc16135b5d147d036e851422686eff7d5ef 
>   src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 18, 2015, 1:54 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yongqiao 
Wang.


Changes
---

Address review comments.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am e6d48dc16135b5d147d036e851422686eff7d5ef 
  src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 18, 2015, 12:57 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, lines 79-83
> > 
> >
> > `AWAIT_EXPECT_RESPONSE_HEADER_EQ(APPLICATION_JSON, "Content-Type", 
> > response);` all fits on one line, and does the AWAITY_READY(response) as 
> > well as the header EQ check.
> > You'll have to `include ` to get `APPLICATION_JSON`.

Fixed. To avoid the need to resolve some merge conflicts on rebase, I actually 
made this fix in https://reviews.apache.org/r/41225/ -- let me know if you'd 
rather I do the rebase + cleanup.


- Neil


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


On Dec. 16, 2015, 9:05 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 16, 2015, 9:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and 
> Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8f6b98b5b0ddbfe6b97346704cb96937e0eca02e 
>   src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Adam B]

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

Ship it!


Looks great! Just a couple of test macro suggestions (applicable in multiple 
tests), and then I'll commit it.


src/tests/role_tests.cpp (lines 79 - 83)


`AWAIT_EXPECT_RESPONSE_HEADER_EQ(APPLICATION_JSON, "Content-Type", 
response);` all fits on one line, and does the AWAITY_READY(response) as well 
as the header EQ check.
You'll have to `include ` to get `APPLICATION_JSON`.



src/tests/role_tests.cpp (line 105)


Not sure why you're using EXPECT_SOME_EQ rather than EXPECT_EQ, since 
you're already doing ASSERT_SOME on `parse` and `expected`.


- Adam B


On Dec. 16, 2015, 1:05 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 16, 2015, 1:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and 
> Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8f6b98b5b0ddbfe6b97346704cb96937e0eca02e 
>   src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 15, 2015, 10:18 a.m., Adam B wrote:
> > Thanks for validating the expected `--roles` behavior with tests, but I 
> > think you're mixing up the scheduler reservation/volume API (where only the 
> > framework's role is valid) with the operator API (where any whitelisted 
> > role is valid).

Ah, good point! Turns out that we weren't properly enforcing `--roles` for 
dynamic reservations anyway (see MESOS-4143). I'll remove these tests for now 
and fix 4143 shortly.


- Neil


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


On Dec. 16, 2015, 9:05 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 16, 2015, 9:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and 
> Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8f6b98b5b0ddbfe6b97346704cb96937e0eca02e 
>   src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 16, 2015, 9:05 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yongqiao 
Wang.


Changes
---

Remove tests for explicit role whitelist + dynamic reservations/persistent 
volumes.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am 8f6b98b5b0ddbfe6b97346704cb96937e0eca02e 
  src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 15, 2015, 8:56 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yongqiao 
Wang.


Changes
---

Address review comments, rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am 8f6b98b5b0ddbfe6b97346704cb96937e0eca02e 
  src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Adam B]

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


Thanks for validating the expected `--roles` behavior with tests, but I think 
you're mixing up the scheduler reservation/volume API (where only the 
framework's role is valid) with the operator API (where any whitelisted role is 
valid).


src/tests/role_tests.cpp (line 44)


A little counterintuitive that the master whitelist includes "role-bad" 
while the framework is trying to register with "role". How about you make it 
explicit that `frameworkInfo.set_role("invalid")` and `masterFlags.roles = 
"foo, bar"`?



src/tests/role_tests.cpp (line 124)


This should fail because the framework is reserving resources for a role 
other than its own, not because the role is not on the whitelist. I'm not sure 
this is a relevant test.



src/tests/role_tests.cpp (lines 141 - 142)


Again, using the scheduler `driver.acceptOffers` API means that the 
framework already shouldn't be able to create a volume with a role other than 
the role with which the framework registered.
Instead, maybe you're thinking of the `/reserve` and `/create` operator 
endpoints, which would want to verify the operation against the role whitelist.



src/tests/role_tests.cpp (lines 267 - 268)


I'd like to see one role explicitly configured, but without a weight, and 
another explicitly configured but also with a weight.


- Adam B


On Dec. 14, 2015, 3:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 14, 2015, 3:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and 
> Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am acd17de04bce81f1d0550abfa0f43dec1a25fe7c 
>   src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 14, 2015, 11:15 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yongqiao 
Wang.


Changes
---

Rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am acd17de04bce81f1d0550abfa0f43dec1a25fe7c 
  src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 10, 2015, 10:08 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yongqiao 
Wang.


Changes
---

Rebase, new tests.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  src/tests/reservation_tests.cpp 54a1b23b22087b5152825125ba146b4cc47af88d 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 8, 2015, 2:55 p.m., Alexander Rukletsov wrote:
> > src/tests/role_tests.cpp, lines 83-84
> > 
> >
> > `MesosTest` exposes the `defaultAgentResourcesString` constant. I think 
> > you can get rid of these lines in order not to distract the user and make 
> > sure `unreserved` is contained in `defaultAgentResourcesString` later in 
> > the code:
> > ```
> > 
> > EXPECT_TRUE(Resources::parse(defaultAgentResourcesString).get().contains(unreserved));
> > ```

I looked at doing this, but I didn't feel like it was an improvement: I think 
it is more readable for the test case to be explicit about the resources we 
expect to be available on the agent.


- Neil


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


On Dec. 9, 2015, 5:53 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 9, 2015, 5:53 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 9, 2015, 5:53 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong Qiao 
Wang.


Changes
---

Rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 8, 2015, 7:18 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong Qiao 
Wang.


Changes
---

Addressed review comments, rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
  src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 8, 2015, 2:55 p.m., Alexander Rukletsov wrote:
> > One thing we also agreed is to disallow empty string roles. Do you think it 
> > makes sense to extend this patch with a test for that or do it separately?

I think we should do this separately, perhaps as part of fixing MESOS-2210.


> On Dec. 8, 2015, 2:55 p.m., Alexander Rukletsov wrote:
> > src/tests/role_tests.cpp, line 36
> > 
> >
> > I believe we use third person singular in such cases.

Fixed.


> On Dec. 8, 2015, 2:55 p.m., Alexander Rukletsov wrote:
> > src/tests/role_tests.cpp, lines 76-77
> > 
> >
> > I think a comment here expaining why you reduce the allocation interval 
> > will be good for posterity. Alternatively, you can use 
> > `Clock::advance(flags.allocation_interval)` before `AWAIT_READY(offers)` to 
> > achieve the same. Perhaps it's a cleaner solution.

Yeah, it would probably be cleaner to use `advance` instead of reducing the 
allocation interval (although that applies to many other tests as well). I'll 
take a look at doing that.


- Neil


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


On Dec. 8, 2015, 8:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 8, 2015, 8:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Alexander Rukletsov]

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


One thing we also agreed is to disallow empty string roles. Do you think it 
makes sense to extend this patch with a test for that or do it separately?


src/tests/role_tests.cpp (line 36)


I believe we use third person singular in such cases.



src/tests/role_tests.cpp (lines 76 - 77)


I think a comment here expaining why you reduce the allocation interval 
will be good for posterity. Alternatively, you can use 
`Clock::advance(flags.allocation_interval)` before `AWAIT_READY(offers)` to 
achieve the same. Perhaps it's a cleaner solution.



src/tests/role_tests.cpp (lines 83 - 84)


`MesosTest` exposes the `defaultAgentResourcesString` constant. I think you 
can get rid of these lines in order not to distract the user and make sure 
`unreserved` is contained in `defaultAgentResourcesString` later in the code:
```

EXPECT_TRUE(Resources::parse(defaultAgentResourcesString).get().contains(unreserved));
```



src/tests/role_tests.cpp (line 172)


Backticks?


- Alexander Rukletsov


On Dec. 8, 2015, 8:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 8, 2015, 8:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Alexander Rukletsov]

> On Dec. 8, 2015, 12:51 a.m., Guangya Liu wrote:
> > src/tests/role_tests.cpp, lines 24-27
> > 
> >
> > I think that we should always use std first?
> > 
> > using std::vector;
> > 
> > using process::Future;
> > using process::PID;
> 
> Neil Conway wrote:
> All the test cases I looked at place `process` before `std`: e.g., 
> `fault_tolerance_tests.cpp`, `master_tests.cpp`, `reservation_tests.cpp`.
> 
> Guangya Liu wrote:
> I think that there are bugs here, you can also refer to 
> master_quota_test.cpp , I recalled that I discussed with alex-mesos for this 
> issue and his comments is we should put std first.
> 
> Neil Conway wrote:
> Can you open a JIRA for this? ISTM we aren't consistent right now, and as 
> far as I can tell, the style guide doesn't say anything about the order of 
> `using` directives.
> 
> Guangya Liu wrote:
> Filed a JIRA here https://issues.apache.org/jira/browse/MESOS-4093 Thanks!

We are not consistent (I think we do 50 / 50 now), but we try to become one day 
: ). An argument for putting `std` first is because we do the same for includes.


- Alexander


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


On Dec. 8, 2015, 8:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 8, 2015, 8:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 8, 2015, 8:28 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong Qiao 
Wang.


Changes
---

Rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
  src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 8, 2015, 5:28 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong Qiao 
Wang.


Changes
---

Whitespace fixes, rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
  src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Yong Qiao Wang]

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

Ship it!


Ship It!

- Yong Qiao Wang


On Dec. 7, 2015, 9:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 7, 2015, 9:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Guangya Liu]

> On 十二月 8, 2015, 12:51 a.m., Guangya Liu wrote:
> > src/tests/role_tests.cpp, lines 24-27
> > 
> >
> > I think that we should always use std first?
> > 
> > using std::vector;
> > 
> > using process::Future;
> > using process::PID;
> 
> Neil Conway wrote:
> All the test cases I looked at place `process` before `std`: e.g., 
> `fault_tolerance_tests.cpp`, `master_tests.cpp`, `reservation_tests.cpp`.
> 
> Guangya Liu wrote:
> I think that there are bugs here, you can also refer to 
> master_quota_test.cpp , I recalled that I discussed with alex-mesos for this 
> issue and his comments is we should put std first.
> 
> Neil Conway wrote:
> Can you open a JIRA for this? ISTM we aren't consistent right now, and as 
> far as I can tell, the style guide doesn't say anything about the order of 
> `using` directives.

Filed a JIRA here https://issues.apache.org/jira/browse/MESOS-4093 Thanks!


- Guangya


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


On 十二月 7, 2015, 9:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated 十二月 7, 2015, 9:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 8, 2015, 12:51 a.m., Guangya Liu wrote:
> > src/tests/role_tests.cpp, lines 24-27
> > 
> >
> > I think that we should always use std first?
> > 
> > using std::vector;
> > 
> > using process::Future;
> > using process::PID;
> 
> Neil Conway wrote:
> All the test cases I looked at place `process` before `std`: e.g., 
> `fault_tolerance_tests.cpp`, `master_tests.cpp`, `reservation_tests.cpp`.
> 
> Guangya Liu wrote:
> I think that there are bugs here, you can also refer to 
> master_quota_test.cpp , I recalled that I discussed with alex-mesos for this 
> issue and his comments is we should put std first.

Can you open a JIRA for this? ISTM we aren't consistent right now, and as far 
as I can tell, the style guide doesn't say anything about the order of `using` 
directives.


- Neil


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


On Dec. 7, 2015, 9:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 7, 2015, 9:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Guangya Liu]

> On 十二月 8, 2015, 12:51 a.m., Guangya Liu wrote:
> > src/tests/role_tests.cpp, lines 24-27
> > 
> >
> > I think that we should always use std first?
> > 
> > using std::vector;
> > 
> > using process::Future;
> > using process::PID;
> 
> Neil Conway wrote:
> All the test cases I looked at place `process` before `std`: e.g., 
> `fault_tolerance_tests.cpp`, `master_tests.cpp`, `reservation_tests.cpp`.

I think that there are bugs here, you can also refer to master_quota_test.cpp , 
I recalled that I discussed with alex-mesos for this issue and his comments is 
we should put std first.


- Guangya


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


On 十二月 7, 2015, 9:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated 十二月 7, 2015, 9:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 8, 2015, 12:51 a.m., Guangya Liu wrote:
> > src/tests/role_tests.cpp, lines 24-27
> > 
> >
> > I think that we should always use std first?
> > 
> > using std::vector;
> > 
> > using process::Future;
> > using process::PID;

All the test cases I looked at place `process` before `std`: e.g., 
`fault_tolerance_tests.cpp`, `master_tests.cpp`, `reservation_tests.cpp`.


- Neil


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


On Dec. 7, 2015, 9:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 7, 2015, 9:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Guangya Liu]

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



src/tests/role_tests.cpp (lines 24 - 27)


I think that we should always use std first?

using std::vector;

using process::Future;
using process::PID;


- Guangya Liu


On 十二月 7, 2015, 9:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated 十二月 7, 2015, 9:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 8, 2015, 12:17 a.m., Joseph Wu wrote:
> > LGTM!
> > 
> > One question: Is the positive case (correct role -> successful operation) 
> > not interesting or just tested in other files?

There are tests for successful cases in other files. Maybe not exhaustive 
though...


- Neil


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


On Dec. 7, 2015, 9:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 7, 2015, 9:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Joseph Wu]

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

Ship it!


LGTM!

One question: Is the positive case (correct role -> successful operation) not 
interesting or just tested in other files?


src/Makefile.am (line 1720)


Nit: Off by one tab.


- Joseph Wu


On Dec. 7, 2015, 1:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 7, 2015, 1:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40995]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 4, 2015, 11:09 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 4, 2015, 11:09 p.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

Review request for mesos, Adam B and Greg Mann.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs
-

  src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
  src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway