Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

> On May 2, 2018, 11:51 a.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 1289 (patched)
> > 
> >
> > I don't think we need this but it doesn't hurt. I'll leave it up to you 
> > to decide if you want to keep this. Please feel free to drop it if you 
> > prefer keeping it.
> > 
> > This is more of a personal preference ;) I prefer always introducing 
> > synchronizations for a reason but some others prefer making tests as 
> > deterministic as possible.

I think I prefer keeping it for now.


- Zhitao


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


On May 1, 2018, 3:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated May 1, 2018, 3:55 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/tests/persistent_volume_tests.cpp
Lines 1289 (patched)


I don't think we need this but it doesn't hurt. I'll leave it up to you to 
decide if you want to keep this. Please feel free to drop it if you prefer 
keeping it.

This is more of a personal preference ;) I prefer always introducing 
synchronizations for a reason but some others prefer making tests as 
deterministic as possible.


- Chun-Hung Hsiao


On May 1, 2018, 10:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated May 1, 2018, 10:55 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

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




src/tests/persistent_volume_tests.cpp
Line 1212 (original), 1133 (patched)


For my self: consistency.


- Zhitao Li


On May 1, 2018, 3:45 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated May 1, 2018, 3:45 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

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

(Updated May 1, 2018, 3:55 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Several more review comments.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

Added test for authorization actions for `RESIZE_VOLUME`.


Diffs (updated)
-

  src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


Diff: https://reviews.apache.org/r/66532/diff/8/

Changes: https://reviews.apache.org/r/66532/diff/7-8/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

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

(Updated May 1, 2018, 3:45 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Review comments.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

Added test for authorization actions for `RESIZE_VOLUME`.


Diffs (updated)
-

  src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


Diff: https://reviews.apache.org/r/66532/diff/7/

Changes: https://reviews.apache.org/r/66532/diff/6-7/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

> On April 30, 2018, 9:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 1256 (patched)
> > 
> >
> > Suggestion: Start the first framework with `DEFAULT_CREDENTIAL`, to be 
> > consistent with your comment for the second framework.

This is implicit by using `DEFAULT_FRAMEWORK_INFO`. Are you suggesting to call 
set_principal() explicitly?


- Zhitao


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


On April 27, 2018, 1:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 27, 2018, 1:08 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Chun-Hung Hsiao]

> On May 1, 2018, 4:05 a.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 1464 (patched)
> > 
> >
> > No need to resume the clock here, as the fixture teardown will resume 
> > it. However, I'm not sure if you need to resume the clock before stopping 
> > and joining `driver2`.
> 
> Zhitao Li wrote:
> I think enough tests explicitly resume the clock, so I'd like to keep it 
> unless you have issue with it.

That's fine but I was wondering if we should move this before `driver2.stop()`, 
as we do in other tests.


- Chun-Hung


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


On April 27, 2018, 8:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 27, 2018, 8:08 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

> On April 30, 2018, 9:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 1464 (patched)
> > 
> >
> > No need to resume the clock here, as the fixture teardown will resume 
> > it. However, I'm not sure if you need to resume the clock before stopping 
> > and joining `driver2`.

I think enough tests explicitly resume the clock, so I'd like to keep it unless 
you have issue with it.


- Zhitao


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


On April 27, 2018, 1:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 27, 2018, 1:08 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Chun-Hung Hsiao]

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




src/tests/authorization_tests.cpp
Lines 1980 (patched)


`resizing volumes`



src/tests/persistent_volume_tests.cpp
Lines 1027 (patched)


Suggestion: I know this creates a little inconsistency within this test 
file, but it would be more consistent if we use the full operation name (as in 
the code) in the backticks, i.e., "`GROW_VOLUME` and `SHRINK_VOLUME` 
operations." Or, just say "grow and shrink operations."



src/tests/persistent_volume_tests.cpp
Lines 1029-1030 (patched)


We could combine the `CREATE` call with a `GROW_VOLUME` or `SHRINK_VOLUME` 
except for the `GrowVolume` and `ShrinkVolume` tests, then this is TODO seems 
not that necessary.



src/tests/persistent_volume_tests.cpp
Lines 1037 (patched)


Suggestion for the TODO: Make MOUNT a meaningful parameter value for this 
test, or create a new fixture to avoid testing against it.



src/tests/persistent_volume_tests.cpp
Lines 1046-1051 (patched)


This is not required. Also there's no need to create a nested block.



src/tests/persistent_volume_tests.cpp
Lines 1093 (patched)


`Offer offer = offersBeforeCreate->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1095 (patched)


`The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 1100 (patched)


`which does not`



src/tests/persistent_volume_tests.cpp
Lines 1117-1120 (patched)


Let's move this close to its first use.



src/tests/persistent_volume_tests.cpp
Lines 1124 (patched)


`containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 1128 (patched)


s/`and launch task `//



src/tests/persistent_volume_tests.cpp
Lines 1131 (patched)


To make this test more succinct, how about doing
`{CREATE(volume), GROW_VOLUME(volume, addition)}` so we can merge this 
`acceptOffers` with the subsequent one?



src/tests/persistent_volume_tests.cpp
Lines 1140 (patched)


`offer = offersBeforeGrow->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1152 (patched)


`containing the grown volume`



src/tests/persistent_volume_tests.cpp
Lines 1156 (patched)


`Grow the volume.` Or merge this call with the above `CREATE` one.



src/tests/persistent_volume_tests.cpp
Lines 1167 (patched)


`offer = offersAfterGrow->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1177-1178 (patched)


This is not needed since its the same as `addition`. If you prefer not to 
use the name `addition` all the time, how about renaming it to `difference`?



src/tests/persistent_volume_tests.cpp
Lines 1182 (patched)


`containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 1196 (patched)


`offer = offersAfterShrink->at(0);`



src/tests/persistent_volume_tests.cpp
Lines 1213 (patched)


Suggestion: This test verifies that `GROW_VOLUME` and `SHRINK_VOLUME` 
operations (or "grow and shrink operations" if you prefer) get dropped if 
authorization fails and no principal is supplied.



src/tests/persistent_volume_tests.cpp
Lines 1215 (patched)


Suggestion: `BadACLDropGrowAndShrink` for consistency.



src/tests/persistent_volume_tests.cpp
Lines 1221 (patched)


Suggestion for the TODO: Make MOUNT a meaningful parameter value for this 
test, or create a new fixture to avoid testing against it.



src/tests/persistent_volume_tests.cpp
Lines 1230-1235 (patched)


This is not required. Also there's no need to create a nested block.



src/tests/persistent_volume_tests.cpp
Lines 1256 (patched)


Sugges

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

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

(Updated April 27, 2018, 1:08 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

use `Clock::settle()`.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

Added test for authorization actions for `RESIZE_VOLUME`.


Diffs (updated)
-

  src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


Diff: https://reviews.apache.org/r/66532/diff/6/

Changes: https://reviews.apache.org/r/66532/diff/5-6/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

> On April 17, 2018, 4:24 p.m., Greg Mann wrote:
> > src/tests/authorization_tests.cpp
> > Line 1979 (original), 1979 (patched)
> > 
> >
> > Could you also add an end-to-end test of authorization for these 
> > operations?
> 
> Zhitao Li wrote:
> Sure. Do you have an example or existing test to add this?
> 
> Greg Mann wrote:
> There are some similar tests in both 'persistent_volume_tests.cpp' as 
> well as 'master_authorization_tests.cpp'.
> 
> Tests which do something very similar in the persistent volume tests are 
> 'PersistentVolumeTest.BadACLDropCreateAndDestroy' and 
> 'PersistentVolumeTest.GoodACLCreateThenDestroy'. It would be good to verify 
> both the successful and failed authorization cases.

I've added some test cases. It seems that there will be some level of code 
duplication and I'm not sure whether we want to follow the ones for 
`...CreateAndDrop`, or Chun's idea of smaller tests verifying one thing at a 
time. We can definitely iterate on these. Marking this comment as fixed.


- Zhitao


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


On April 24, 2018, 11:16 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> ---
> 
> (Updated April 24, 2018, 11:16 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for authorization actions for `RESIZE_VOLUME`.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66532/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

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

(Updated April 24, 2018, 11:16 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Add initial tests for authorization of new operations.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

Added test for authorization actions for `RESIZE_VOLUME`.


Diffs (updated)
-

  src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 
  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


Diff: https://reviews.apache.org/r/66532/diff/5/

Changes: https://reviews.apache.org/r/66532/diff/4-5/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66532: Added test for authorization actions for `RESIZE_VOLUME`.

[Zhitao Li]

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

(Updated April 20, 2018, 4:17 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Renamed to `ResizeVolume`.


Summary (updated)
-

Added test for authorization actions for `RESIZE_VOLUME`.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description (updated)
---

Added test for authorization actions for `RESIZE_VOLUME`.


Diffs (updated)
-

  src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 


Diff: https://reviews.apache.org/r/66532/diff/4/

Changes: https://reviews.apache.org/r/66532/diff/3-4/


Testing
---


Thanks,

Zhitao Li