Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 25, 2015, 4:09 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

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



src/tests/master_quota_tests.cpp 


Tests for these ones are not implemented yet, mind restoring the item?



src/tests/master_quota_tests.cpp (line 186)


Let's wrap "start" for consistency. Here and below.



src/tests/master_quota_tests.cpp (line 189)


s/http::post/`http::post()`
(note backticks)



src/tests/master_quota_tests.cpp (lines 198 - 207)


Let's put these lines into a scope and prepend the scope with the comment 
you have before `badRequest`.

Same for two other sections in this test.



src/tests/master_quota_tests.cpp (lines 235 - 260)


This is covered in the previous test already, right?



src/tests/master_quota_tests.cpp (lines 263 - 264)


How about a one-liner:
```
// Tests a quota request with invalid json fails with '400 Bad Request'.
```
?



src/tests/master_quota_tests.cpp (lines 290 - 291)


One-liner is possible:
```
// Tests a quota request with multiple roles fails with '400 Bad Request'.
```



src/tests/master_quota_tests.cpp (line 318)


s/results in an error. Should return a '400 BadRequest' return code./fails 
with '400 Bad Request'.



src/tests/master_quota_tests.cpp (line 340)


10 is less than `MIN_MEM`, does it make sense to simply follow the pattern 
in other tests in this file and use "cpus:1;mem:512;"?



src/tests/master_quota_tests.cpp (line 352)


Why incremental? Let's avoid describing semantics for prohibited operations 
: ). How about:
"Try to repeatedly set quota via POST"?



src/tests/master_quota_tests.cpp (lines 378 - 393)


Again, will it be easier for a reader to understand if you scope this 
section?


- Alexander Rukletsov


On Nov. 20, 2015, 9:49 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 20, 2015, 9:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

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


Please adjust the JIRA ticket: MESOS-3983

- Alexander Rukletsov


On Nov. 20, 2015, 9:49 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 20, 2015, 9:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 20, 2015, 9:49 a.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 19, 2015, 9:39 a.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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



src/tests/master_quota_tests.cpp (line 332)


Move before StartSlave here and below.


- Joerg Schad


On Nov. 19, 2015, 9:39 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 19, 2015, 9:39 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 17, 2015, 3:44 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 17, 2015, 10:29 a.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 16, 2015, 12:48 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

> On Nov. 13, 2015, 2:23 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, lines 201-203
> > 
> >
> > This format may be better? 
> > https://github.com/apache/mesos/blob/master/src/tests/resources_tests.cpp#L417-L426

Thanks adapted to that style. Maybe we should start a general discussion/add it 
to the style-guide?


- Joerg


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


On Nov. 15, 2015, 6:05 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 15, 2015, 6:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 15, 2015, 6:05 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Guangya Liu]

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



src/tests/master_quota_tests.cpp (lines 196 - 198)


This format may be better? 
https://github.com/apache/mesos/blob/master/src/tests/resources_tests.cpp#L417-L426



src/tests/master_quota_tests.cpp (lines 228 - 230)


What about this format? 
https://github.com/apache/mesos/blob/master/src/tests/resources_tests.cpp#L331-L340



src/tests/master_quota_tests.cpp (lines 260 - 262)


ditto


- Guangya Liu


On 十一月 12, 2015, 3:22 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated 十一月 12, 2015, 3:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 12, 2015, 3:22 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

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



src/tests/master_quota_tests.cpp (line 288)


s/We request/Request



src/tests/master_quota_tests.cpp (line 320)


s/We request/Request



src/tests/master_quota_tests.cpp (lines 320 - 324)


I think the one before you create the `Resources` object is better because 
the `process::http::post()` block is the same in every test. 

So how about
```
s/We request quota for two different roles./Create a quota request with 
resources belonging to different roles.
```
and
```
s/Send a quota request with resources belonging to different roles.//
```
?



src/tests/master_quota_tests.cpp (line 336)


Isn't it safer to use constants here?



src/tests/master_quota_tests.cpp (line 342)


Maybe wrap /master/quota in backticks?



src/tests/master_quota_tests.cpp (line 359)


Formatting.



src/tests/master_quota_tests.cpp (line 407)


s\Resources\a Resources instance

Same everywhere below.



src/tests/master_quota_tests.cpp (line 415)


Does this comment add some value here? You haven't used any comments for 
`process::http::post()` blocks before.



src/tests/master_quota_tests.cpp (line 437)


Again, this comment looks strange here. Moreover, in the combination with 
the comment for the same block above, it make the reader think this request is 
not for a specified role.



src/tests/master_quota_tests.cpp (line 460)


Ditto.


- Alexander Rukletsov


On Nov. 10, 2015, 10:27 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 10, 2015, 10:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

> On Nov. 3, 2015, 12:35 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 463
> > 
> >
> > We try to keep tests atomic: testing one thing at a time. However, I 
> > think it does make sense to merge `Set_InvalidJson`, `Set_InvalidJson2`, 
> > and `Set_InvalidResources` into one tests for brevity and speed. These 3 
> > tests address the same issue: malformed JSON, similar to what you have done 
> > with `Set_InvalidResourceInfos`.
> 
> Joerg Schad wrote:
> I feel that these are actually testing slightly different stuff 
> (different from the invalidResource Info)

I think it's fine to have multiple related "test blocks" in a test case, if it 
is clear where the problem is when a test fails. Reasons for that are code 
reusal (less copy-paste code — less typos, less confusion, easier reviewing, 
smaller codebase), faster compilation and saving `SetUp()`, `TearDown()` 
cycles. 

In this case I think it's fine to have a single test.


- Alexander


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


On Nov. 10, 2015, 10:27 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 10, 2015, 10:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 10:27 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 2:05 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

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



src/tests/master_quota_tests.cpp (line 449)


Do you think it's safer to use "cpus" as revocable resoource?


- Alexander Rukletsov


On Nov. 9, 2015, 10:45 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 9, 2015, 10:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 9, 2015, 10:45 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Guangya Liu]

> On Nov. 8, 2015, 2:24 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, lines 209-210
> > 
> >
> > Can you please make this comments limit in 70 per line and ditto for 
> > the following?
> 
> Alexander Rukletsov wrote:
> Why do you suggest to limit to 70?

Well, I do not have strong opinion on this but as we are now still discussing 
http://search-hadoop.com/m/0Vlr6MlNkc1FGzCj2/Mesos+Style+Guideline+Adjustments%25E2%2580%258F+Ben+Mahler+This+has+come+up+in+a+couple+of+reviews%252C+seems+like+we+should+add+some+soft=Re+Mesos+Style+Guideline+Adjustments
 this.

But keeping the comments limit in 70 chars can make the comments more readable 
here as the two lines will almost have same length.


- Guangya


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


On Nov. 6, 2015, 5:02 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 6, 2015, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

> On Nov. 8, 2015, 2:24 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, lines 209-210
> > 
> >
> > Can you please make this comments limit in 70 per line and ditto for 
> > the following?

Why do you suggest to limit to 70?


- Alexander


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


On Nov. 6, 2015, 5:02 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 6, 2015, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 9, 2015, 7:29 a.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing (updated)
---

make check


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Guangya Liu]

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



src/tests/master_quota_tests.cpp (lines 209 - 210)


Can you please make this comments limit in 70 per line and ditto for the 
following?



src/tests/master_quota_tests.cpp (lines 212 - 215)


indent should be 2 space here?



src/tests/master_quota_tests.cpp (lines 243 - 246)


2 spaces?



src/tests/master_quota_tests.cpp (lines 274 - 276)


Keep consistent with above test case request?


- Guangya Liu


On 十一月 6, 2015, 5:02 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated 十一月 6, 2015, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

> On Nov. 3, 2015, 12:35 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 431
> > 
> >
> > It looks like this comment is a victim of partial refactoring : ). 
> > Could you please udpate it?
> > 
> > Also we usually use 3rd person in test comments.
> > 
> > ```
> > // This test ensures...
> > ```
> > or
> > ```
> > // This tests that...
> > ```
> > or simply
> > ```
> > // Tests whether...
> > ```

Just following your lead on your previous comment ;-): "Verifies that a request 
for a non-existent role is rejected."


- Joerg


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


On Oct. 23, 2015, 9:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Oct. 23, 2015, 9:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 6, 2015, 5:02 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

> On Nov. 3, 2015, 12:35 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 463
> > 
> >
> > We try to keep tests atomic: testing one thing at a time. However, I 
> > think it does make sense to merge `Set_InvalidJson`, `Set_InvalidJson2`, 
> > and `Set_InvalidResources` into one tests for brevity and speed. These 3 
> > tests address the same issue: malformed JSON, similar to what you have done 
> > with `Set_InvalidResourceInfos`.

I feel that these are actually testing slightly different stuff (different from 
the invalidResource Info)


- Joerg


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


On Oct. 23, 2015, 9:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Oct. 23, 2015, 9:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

> On Oct. 25, 2015, 8:31 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, line 564
> > 
> >
> > We should unify if use "role1" or role1 for all of the tests

I vote for variables, since they were introduced exactly for this.


> On Oct. 25, 2015, 8:31 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, line 433
> > 
> >
> > s/Set_InvalidRequest/SetInvalidRequest?
> > 
> > Ditto for the following

I think the intention was to indicate these tests relate to quota set requests. 
However, I'm +1 for removing the underscore, because it violates the 
consistency.


- Alexander


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


On Oct. 23, 2015, 9:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Oct. 23, 2015, 9:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

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


Great tests, thanks a lot Jörg!


src/tests/master_quota_tests.cpp (lines 156 - 163)


It looks like you have implemented most of those. Mind updating this 
`TODO`? Thanks!



src/tests/master_quota_tests.cpp (line 429)


We already have a section for validation requests, I'd suggest you move 
those there : ). I'd say, around L197, after `NonExistentRole`.



src/tests/master_quota_tests.cpp (line 431)


It looks like this comment is a victim of partial refactoring : ). Could 
you please udpate it?

Also we usually use 3rd person in test comments.

```
// This test ensures...
```
or
```
// This tests that...
```
or simply
```
// Tests whether...
```



src/tests/master_quota_tests.cpp (line 433)


I think what Jörg tries to express is that these tests are for "set-path" 
of quota. However it indeed violates consistency, hence I'm +1 for removing the 
underscore.



src/tests/master_quota_tests.cpp (lines 442 - 444)


`clang-format` suggests the following:
```
  string badRequest =
"{"
"invalidJson"
"}";
```



src/tests/master_quota_tests.cpp (line 450)


You can avoid wrapping `badRequest` in `Option<>`.



src/tests/master_quota_tests.cpp (line 451)


Do you need `None()` here? I think you can omit it.



src/tests/master_quota_tests.cpp (line 453)


You don't need a fully qualified type name here thanks to `using` 
directives above. I would also suggest to print additional info in case of 
failure. Putting all together, how about
```
AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response)
<< response.get().body;
```
?

The suggestion holds for tests below as well.



src/tests/master_quota_tests.cpp (line 461)


Same here: 3rd person singular.



src/tests/master_quota_tests.cpp (line 463)


We try to keep tests atomic: testing one thing at a time. However, I think 
it does make sense to merge `Set_InvalidJson`, `Set_InvalidJson2`, and 
`Set_InvalidResources` into one tests for brevity and speed. These 3 tests 
address the same issue: malformed JSON, similar to what you have done with 
`Set_InvalidResourceInfos`.



src/tests/master_quota_tests.cpp (lines 472 - 474)


Formatting, see above.



src/tests/master_quota_tests.cpp (lines 480 - 481)


See suggestion for `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 483)


See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 491)


3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 493)


See suggestion above, does it make sense to meld this test into previous 
ones?



src/tests/master_quota_tests.cpp (line 513)


See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 521)


3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 522)


Why capitalize "Code"? Here and below.



src/tests/master_quota_tests.cpp (lines 536 - 540)


Correct indentation.



src/tests/master_quota_tests.cpp (line 542)


See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 550)


3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 552)


How about renaming the test to `SetMultipleRoles`?



src/tests/master_quota_tests.cpp (lines 563 - 564)


This fits one line.



src/tests/master_quota_tests.cpp (line 564)

Re: Review Request 39223: Added Quota Request Validation Tests.

[Guangya Liu]

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



src/tests/master_quota_tests.cpp (line 433)


s/Set_InvalidRequest/SetInvalidRequest?

Ditto for the following



src/tests/master_quota_tests.cpp (line 564)


We should unify if use "role1" or role1 for all of the tests



src/tests/master_quota_tests.cpp (line 659)


new line



src/tests/master_quota_tests.cpp (line 676)


new line



src/tests/master_quota_tests.cpp (line 694)


new line


- Guangya Liu


On 十月 23, 2015, 9:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated 十月 23, 2015, 9:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Oct. 23, 2015, 9:10 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


Summary (updated)
-

Added Quota Request Validation Tests.


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


Repository: mesos


Description (updated)
---

see Summary.


Diffs
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad