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