----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44621/#review123302 -----------------------------------------------------------
Looks good. Just some minor style points to clean up, and then we can ship it src/tests/master_maintenance_tests.cpp (line 1749) <https://reviews.apache.org/r/44621/#comment185512> s/POST's/POSTs/ - it's plural, not possessive src/tests/master_maintenance_tests.cpp (line 1750) <https://reviews.apache.org/r/44621/#comment185513> Sorry, but we don't like abbrevs in variable names. How about "unauthenticatedHeaders" or "anonymousHeaders"? src/tests/master_maintenance_tests.cpp (line 1757) <https://reviews.apache.org/r/44621/#comment185514> Seems weird that this one doesn't get a pre-definition comment. src/tests/master_maintenance_tests.cpp (line 1761) <https://reviews.apache.org/r/44621/#comment185515> s/POST's/POSTs/ src/tests/master_maintenance_tests.cpp (line 1762) <https://reviews.apache.org/r/44621/#comment185516> No abbreviations, so "badAuthenticationHeaders"? src/tests/master_maintenance_tests.cpp (lines 1767 - 1768) <https://reviews.apache.org/r/44621/#comment185518> Comment: // Post the maintenance schedule without authentication. src/tests/master_maintenance_tests.cpp (lines 1768 - 1781) <https://reviews.apache.org/r/44621/#comment185517> Could you please be consistent about testing POST first or GET first? src/tests/master_maintenance_tests.cpp (line 1786) <https://reviews.apache.org/r/44621/#comment185519> You can't use `badAuthnHeaders` here, because of the content-type? src/tests/master_maintenance_tests.cpp (line 1821) <https://reviews.apache.org/r/44621/#comment185520> s/up/down/ src/tests/master_tests.cpp (line 4195) <https://reviews.apache.org/r/44621/#comment185521> "Test get requests on various endpoints without..." src/tests/master_tests.cpp (line 4265) <https://reviews.apache.org/r/44621/#comment185522> There is no GET request allowed on /weights (yet), so it's interesting to me that this part of the test passes. I would expect it to return 405 MethodNotAllowed. Or I guess authentication happens before we even get to the handler to check the action? src/tests/repair_tests.cpp (line 225) <https://reviews.apache.org/r/44621/#comment185524> Any reason you only test get, but not post? src/tests/repair_tests.cpp (line 226) <https://reviews.apache.org/r/44621/#comment185523> s/EndpointBadAuthentication/ObserveEndpointBadAuthentication/ since `HealthTest` sounds too generic to just apply to the observe endpoint. - Adam B On March 11, 2016, 11:49 a.m., Joerg Schad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44621/ > ----------------------------------------------------------- > > (Updated March 11, 2016, 11:49 a.m.) > > > Review request for mesos, Adam B and Alexander Rojas. > > > Bugs: MESOS-4844 > https://issues.apache.org/jira/browse/MESOS-4844 > > > Repository: mesos > > > Description > ------- > > With enabling http authentication for http endpoints we should also add tests > to check > that http request to those endpoints return "401 Unauthorized" if queried > without or with > bad credentials. > > > Diffs > ----- > > src/tests/master_maintenance_tests.cpp > 3c7024cfbd7e5bef75f092eace8d0e80000ca423 > src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 > src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da > src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 > > Diff: https://reviews.apache.org/r/44621/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Joerg Schad > >
