----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41790/#review113689 -----------------------------------------------------------
A good set of tests for the functionality, but I've got some suggestions for you, most importantly: - We should consider an official `http::put()` method like `http::post()`, etc. - Don't forget to clear the permissive bit if you want restrictive ACLs. - I found a few unused methods/variables. Use it or lose it. src/tests/dynamic_weights_tests.cpp (line 70) <https://reviews.apache.org/r/41790/#comment174484> s/std:://g src/tests/dynamic_weights_tests.cpp (line 79) <https://reviews.apache.org/r/41790/#comment174447> s/weithgs/weights/ src/tests/dynamic_weights_tests.cpp (line 88) <https://reviews.apache.org/r/41790/#comment174448> This is never called? src/tests/dynamic_weights_tests.cpp (line 99) <https://reviews.apache.org/r/41790/#comment174449> `s/;/./` src/tests/dynamic_weights_tests.cpp (line 103) <https://reviews.apache.org/r/41790/#comment174450> Error check, for empty vector, etc. src/tests/dynamic_weights_tests.cpp (line 105) <https://reviews.apache.org/r/41790/#comment174483> s/std:://g src/tests/dynamic_weights_tests.cpp (line 106) <https://reviews.apache.org/r/41790/#comment174451> Assert that pair.size() == 2 src/tests/dynamic_weights_tests.cpp (line 130) <https://reviews.apache.org/r/41790/#comment174452> s/std:://g src/tests/dynamic_weights_tests.cpp (lines 130 - 133) <https://reviews.apache.org/r/41790/#comment174454> We usually use `FOO = "bar";` rather than `FOO{"bar"}`. src/tests/dynamic_weights_tests.cpp (line 133) <https://reviews.apache.org/r/41790/#comment174453> Unused? src/tests/dynamic_weights_tests.cpp (line 135) <https://reviews.apache.org/r/41790/#comment174455> Unused? src/tests/dynamic_weights_tests.cpp (line 146) <https://reviews.apache.org/r/41790/#comment174462> I'm not convinced the lambda makes this much more readable. And you aren't actually calling `http::put`, since that doesn't exist. src/tests/dynamic_weights_tests.cpp (line 148) <https://reviews.apache.org/r/41790/#comment174456> Why don't we have a `process::http::put()`? src/tests/dynamic_weights_tests.cpp (line 155) <https://reviews.apache.org/r/41790/#comment174457> What does the false do? Please comment src/tests/dynamic_weights_tests.cpp (line 158) <https://reviews.apache.org/r/41790/#comment174468> s/a update/an update/g src/tests/dynamic_weights_tests.cpp (lines 170 - 173) <https://reviews.apache.org/r/41790/#comment174466> Why the scoping here? If you want to reuse the names 'badRequest' and 'response', you could do so without creating a new variable each time. src/tests/dynamic_weights_tests.cpp (line 186) <https://reviews.apache.org/r/41790/#comment174470> s/update weights/update weights request/ s/invalid fields/an invalid field/ src/tests/dynamic_weights_tests.cpp (line 191) <https://reviews.apache.org/r/41790/#comment174472> What makes this an invalid field? Isn't it a perfectly reasonable role name? Or is it supposed to be `weights=[{ "role":"role1", "weight":1.5}]`, in which case "role1" is also an invalid field name? src/tests/dynamic_weights_tests.cpp (line 204) <https://reviews.apache.org/r/41790/#comment174473> Can you test weight=0 too? src/tests/dynamic_weights_tests.cpp (lines 221 - 222) <https://reviews.apache.org/r/41790/#comment174474> Could you verify after the failed update that the weights are still at their original value? src/tests/dynamic_weights_tests.cpp (lines 264 - 265) <https://reviews.apache.org/r/41790/#comment174475> s/does not contains/not specified/ src/tests/dynamic_weights_tests.cpp (line 267) <https://reviews.apache.org/r/41790/#comment174476> s/NonExistent/Nonexistent/ src/tests/dynamic_weights_tests.cpp (line 292) <https://reviews.apache.org/r/41790/#comment174477> s/the existent roles/a whitelisted role/ src/tests/dynamic_weights_tests.cpp (line 293) <https://reviews.apache.org/r/41790/#comment174478> s/contains in/contained in/ s/is succeeded/succeeds/ src/tests/dynamic_weights_tests.cpp (lines 313 - 314) <https://reviews.apache.org/r/41790/#comment174479> Verify the new weights in `/roles`? src/tests/dynamic_weights_tests.cpp (lines 320 - 321) <https://reviews.apache.org/r/41790/#comment174480> s/is succeeded/succeeds/ src/tests/dynamic_weights_tests.cpp (line 329) <https://reviews.apache.org/r/41790/#comment174481> "specified role"? Which one? You mean WEIGHTS? src/tests/dynamic_weights_tests.cpp (line 372) <https://reviews.apache.org/r/41790/#comment174482> s/absense/absence/ src/tests/dynamic_weights_tests.cpp (lines 394 - 395) <https://reviews.apache.org/r/41790/#comment174485> You should also set `acls.permissive = false;` if you want to actually disallow unknown principals/roles. src/tests/dynamic_weights_tests.cpp (lines 429 - 430) <https://reviews.apache.org/r/41790/#comment174486> also permissive=false - Adam B On Jan. 9, 2016, 3:38 a.m., Yongqiao Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41790/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2016, 3:38 a.m.) > > > Review request for mesos, Adam B, Neil Conway, and Qian Zhang. > > > Bugs: MESOS-4200 > https://issues.apache.org/jira/browse/MESOS-4200 > > > Repository: mesos > > > Description > ------- > > Add tests for /weights endpoint. > > > Diffs > ----- > > src/Makefile.am bbd0c119321fa9d11fea61b140428dd92d1258c8 > src/tests/dynamic_weights_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/41790/diff/ > > > Testing > ------- > > Make and Make check successfully! > > ./src/mesos-tests --gtest_filter=DynamicWeightsTest.* > [==========] Running 10 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 10 tests from DynamicWeightsTest > [ RUN ] DynamicWeightsTest.PutInvalidRequest > [ OK ] DynamicWeightsTest.PutInvalidRequest (102 ms) > [ RUN ] DynamicWeightsTest.NegativeWeight > [ OK ] DynamicWeightsTest.NegativeWeight (42 ms) > [ RUN ] DynamicWeightsTest.MissingRole > [ OK ] DynamicWeightsTest.MissingRole (43 ms) > [ RUN ] DynamicWeightsTest.NonExistentRole > [ OK ] DynamicWeightsTest.NonExistentRole (30 ms) > [ RUN ] DynamicWeightsTest.UpdateWeightsWithExplictRoles > [ OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (38 ms) > [ RUN ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles > [ OK ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles (42 ms) > [ RUN ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest > [ OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (35 ms) > [ RUN ] DynamicWeightsTest.AuthorizedWeightUpdateRequest > [ OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (37 ms) > [ RUN ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal > [ OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal > (44 ms) > [ RUN ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest > [ OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (30 ms) > [----------] 10 tests from DynamicWeightsTest (443 ms total) > > [----------] Global test environment tear-down > [==========] 10 tests from 1 test case ran. (452 ms total) > [ PASSED ] 10 tests. > > > Thanks, > > Yongqiao Wang > >
