-----------------------------------------------------------
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)
<https://reviews.apache.org/r/39223/#comment163192>

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



src/tests/master_quota_tests.cpp (line 429)
<https://reviews.apache.org/r/39223/#comment163158>

    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)
<https://reviews.apache.org/r/39223/#comment163163>

    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)
<https://reviews.apache.org/r/39223/#comment163164>

    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)
<https://reviews.apache.org/r/39223/#comment163165>

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



src/tests/master_quota_tests.cpp (line 450)
<https://reviews.apache.org/r/39223/#comment163167>

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



src/tests/master_quota_tests.cpp (line 451)
<https://reviews.apache.org/r/39223/#comment163166>

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



src/tests/master_quota_tests.cpp (line 453)
<https://reviews.apache.org/r/39223/#comment163168>

    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)
<https://reviews.apache.org/r/39223/#comment163169>

    Same here: 3rd person singular.



src/tests/master_quota_tests.cpp (line 463)
<https://reviews.apache.org/r/39223/#comment163170>

    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)
<https://reviews.apache.org/r/39223/#comment163171>

    Formatting, see above.



src/tests/master_quota_tests.cpp (lines 480 - 481)
<https://reviews.apache.org/r/39223/#comment163172>

    See suggestion for `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 483)
<https://reviews.apache.org/r/39223/#comment163173>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 491)
<https://reviews.apache.org/r/39223/#comment163175>

    3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 493)
<https://reviews.apache.org/r/39223/#comment163174>

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



src/tests/master_quota_tests.cpp (line 513)
<https://reviews.apache.org/r/39223/#comment163193>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 521)
<https://reviews.apache.org/r/39223/#comment163176>

    3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 522)
<https://reviews.apache.org/r/39223/#comment163202>

    Why capitalize "Code"? Here and below.



src/tests/master_quota_tests.cpp (lines 536 - 540)
<https://reviews.apache.org/r/39223/#comment163182>

    Correct indentation.



src/tests/master_quota_tests.cpp (line 542)
<https://reviews.apache.org/r/39223/#comment163194>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 550)
<https://reviews.apache.org/r/39223/#comment163177>

    3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 552)
<https://reviews.apache.org/r/39223/#comment163208>

    How about renaming the test to `SetMultipleRoles`?



src/tests/master_quota_tests.cpp (lines 563 - 564)
<https://reviews.apache.org/r/39223/#comment163204>

    This fits one line.



src/tests/master_quota_tests.cpp (line 564)
<https://reviews.apache.org/r/39223/#comment163205>

    Let's use variables that have been created exactly for this purpose.



src/tests/master_quota_tests.cpp (lines 567 - 568)
<https://reviews.apache.org/r/39223/#comment163206>

    - Fits one line;
    - s/"role2"/role2



src/tests/master_quota_tests.cpp (line 572)
<https://reviews.apache.org/r/39223/#comment163207>

    s/for the specified rols/with resources belonging to different roles



src/tests/master_quota_tests.cpp (lines 573 - 577)
<https://reviews.apache.org/r/39223/#comment163183>

    Correct indentation.



src/tests/master_quota_tests.cpp (line 579)
<https://reviews.apache.org/r/39223/#comment163195>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 587)
<https://reviews.apache.org/r/39223/#comment163178>

    3rd person singular, see reasoning above.



src/tests/master_quota_tests.cpp (line 592)
<https://reviews.apache.org/r/39223/#comment163220>

    Indentation.



src/tests/master_quota_tests.cpp (lines 605 - 606)
<https://reviews.apache.org/r/39223/#comment163221>

    Insert a blank line.



src/tests/master_quota_tests.cpp (line 612)
<https://reviews.apache.org/r/39223/#comment163222>

    Let's move this up several lines after `AWAIT_READY(agentTotalResources);` 
for clarity.



src/tests/master_quota_tests.cpp (lines 615 - 619)
<https://reviews.apache.org/r/39223/#comment163184>

    Correct indentation.



src/tests/master_quota_tests.cpp (line 621)
<https://reviews.apache.org/r/39223/#comment163197>

    See suggestion regarding logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (lines 624 - 628)
<https://reviews.apache.org/r/39223/#comment163185>

    Correct indentation.



src/tests/master_quota_tests.cpp (line 630)
<https://reviews.apache.org/r/39223/#comment163196>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 638)
<https://reviews.apache.org/r/39223/#comment163179>

    3rd person singular, see reasoning above.
    
    Remove one "is".



src/tests/master_quota_tests.cpp (lines 639 - 641)
<https://reviews.apache.org/r/39223/#comment163180>

    I think we should end comments with a period:
      - Also list items.



src/tests/master_quota_tests.cpp (line 647)
<https://reviews.apache.org/r/39223/#comment163209>

    Let's add the comment you have before:
    ```
      // We do not need an agent since a request should be rejected before we 
start
      // looking at available resources.
    ```
    as it documents an important assumption.



src/tests/master_quota_tests.cpp (line 648)
<https://reviews.apache.org/r/39223/#comment163210>

    Wrap types in backticks.



src/tests/master_quota_tests.cpp (line 650)
<https://reviews.apache.org/r/39223/#comment163211>

    s/"role1"/role1



src/tests/master_quota_tests.cpp (lines 650 - 651)
<https://reviews.apache.org/r/39223/#comment163212>

    Newline, please.



src/tests/master_quota_tests.cpp (line 651)
<https://reviews.apache.org/r/39223/#comment163213>

    s/"role1"/role1



src/tests/master_quota_tests.cpp (lines 653 - 654)
<https://reviews.apache.org/r/39223/#comment163215>

    Newline, please.



src/tests/master_quota_tests.cpp (lines 655 - 659)
<https://reviews.apache.org/r/39223/#comment163186>

    Correct indentation.



src/tests/master_quota_tests.cpp (line 660)
<https://reviews.apache.org/r/39223/#comment163198>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 664)
<https://reviews.apache.org/r/39223/#comment163216>

    Wrap types in backticks.



src/tests/master_quota_tests.cpp (lines 667 - 668)
<https://reviews.apache.org/r/39223/#comment163217>

    - Newline
    - s/"role1"/role1



src/tests/master_quota_tests.cpp (lines 670 - 671)
<https://reviews.apache.org/r/39223/#comment163187>

    Newline, please!



src/tests/master_quota_tests.cpp (lines 672 - 676)
<https://reviews.apache.org/r/39223/#comment163188>

    Correct indentation.



src/tests/master_quota_tests.cpp (line 677)
<https://reviews.apache.org/r/39223/#comment163199>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (lines 681 - 682)
<https://reviews.apache.org/r/39223/#comment163201>

    Let's wrap type names in backticks.



src/tests/master_quota_tests.cpp (lines 684 - 685)
<https://reviews.apache.org/r/39223/#comment163218>

    - Newline
    - s/"role1"/role1



src/tests/master_quota_tests.cpp (line 687)
<https://reviews.apache.org/r/39223/#comment163219>

    s/"principal"/DEFAULT_CREDENTIAL.principal()
    
    It won't fit one line, so we need a blank line afterwards.



src/tests/master_quota_tests.cpp (lines 688 - 689)
<https://reviews.apache.org/r/39223/#comment163189>

    Newline.



src/tests/master_quota_tests.cpp (lines 690 - 694)
<https://reviews.apache.org/r/39223/#comment163190>

    Correct indentation.



src/tests/master_quota_tests.cpp (line 695)
<https://reviews.apache.org/r/39223/#comment163200>

    See suggestion regarding FQTN and logging in `Set_InvalidRequest`.



src/tests/master_quota_tests.cpp (line 698)
<https://reviews.apache.org/r/39223/#comment163191>

    Don't we need `Shutdown()` here?


- Alexander Rukletsov


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

Reply via email to