----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38000/#review109405 -----------------------------------------------------------
Second part of my earlier review, this time I went throug the test code. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1214 - 1216) <https://reviews.apache.org/r/38000/#comment168899> Looking at this, it's a little strange that realm comes after help. Also, what's "mesos"? :) In the interest of making this as readable as possible, how about we just use "realm" in the tests here? Why do anything less direct? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1226 - 1250) <https://reviews.apache.org/r/38000/#comment168906> After you re-use HttpProxy, the only thing that should be in this fixture is setting the authenticator during setup and unsetting during teardown. How about naming this fixture 'HttpAuthenticationTest'? I can't quite tell why some names have http as the test case name, and some don't: ``` AuthenticationTest. HTTPAuthenticationNoAuthenticator AuthenticationTest, AuthenticationNoCredentials AuthenticationTest, AuthenticationSuccess AuthenticationTest, DelayedAuthentication ``` How about the following, also note I tried not to repeat authentication since it's clear from the test fixture name: ``` HttpAuthenticationTest, Disabled HttpAuthenticationTest, Unauthorized HttpAuthenticationTest, Forbidden HttpAuthenticationTest, Authenticated HttpAuthenticationTest, Pipelining ``` 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1253 - 1254) <https://reviews.apache.org/r/38000/#comment168909> An endpoint doesn't really request authentication, it just specifies a realm, no? I imagine that the wiring of authenticators to realms isn't tightly tied to the route() calls. How about: ``` Ensures that when there is no authenticator for a realm, requests are not authenticated (i.e. the principal is None). ``` 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1258 - 1259) <https://reviews.apache.org/r/38000/#comment168912> Ugh. I suspected I was going to see something like this when I saw your test fixture. If we're going to put things in test fixtures I would suggest making them flexible and explicit, rather than implicit and static. For example, rather than statically deciding to add an authenticator to the "mesos" realm (which let's just call "realm"!), we can have an explicit method for adding an authenticator: ``` addAuthenticator("realm", Owned<Authenticator>(new MockAuthenticator()); ``` When a test calls this, the test fixture will keep track of the set of realms that need to be unset, so that we can clean up in TearDown. Sound good? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1270) <https://reviews.apache.org/r/38000/#comment168914> Have you looked at gtest.hpp in libprocess? We have: AWAIT_EXPECT_RESPONSE_STATUS_EQ We should update that assertion to check both the code and status, not sure why we manually check both all over the place. 3rdparty/libprocess/src/tests/http_tests.cpp (line 1273) <https://reviews.apache.org/r/38000/#comment168917> No CHECKs in tests please :( Also, you don't need to do this AFAICT, you can just do the following above to simplify this: ``` EXPECT_CALL((*process.get()), handler1(_, Option<string>::none())); ``` Does that not work? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1281) <https://reviews.apache.org/r/38000/#comment168932> Let's avoid the "auth" abbreviation entirely, can you do a sweep? I also see it in the tests below. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1295 - 1297) <https://reviews.apache.org/r/38000/#comment168947> Why is this EXPECT_SOME_EQ instead of EXPECT_EQ? Aren't both of these options? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1305) <https://reviews.apache.org/r/38000/#comment168930> In the interest of making this more readable: s/"foo"/"principal"/ 3rdparty/libprocess/src/tests/http_tests.cpp (line 1309) <https://reviews.apache.org/r/38000/#comment168933> Why is there an underscore here? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1310 - 1311) <https://reviews.apache.org/r/38000/#comment168934> Do you need to capture? It looks like you can just place the value in the expectation: ``` EXPECT_CALL((*process.get()), handler1(_, "principal")); ``` Not sure if it can implicitly construct the option here, might need to do that explicitly. 3rdparty/libprocess/src/tests/http_tests.cpp (line 1316) <https://reviews.apache.org/r/38000/#comment168931> Can you just do "user:password" here? The comment seems sufficient to me 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1320 - 1322) <https://reviews.apache.org/r/38000/#comment168935> If you don't need the capture the option, can you use the AWAIT_EXPECT_RESPONSE_STATUS_EQ here? Ditto elsehwere 3rdparty/libprocess/src/tests/http_tests.cpp (line 1323) <https://reviews.apache.org/r/38000/#comment168945> If you have two options, you can just use EXPECT_EQ against the options directly: ``` EXPECT_EQ(authResult.principal, principal); ``` Although I think you don't need this per my other comment. 3rdparty/libprocess/src/tests/http_tests.cpp (line 1332) <https://reviews.apache.org/r/38000/#comment168936> This confused me a bit, how about: ``` // We satisfy the authentication futures in reverse // order. Libprocess should not re-order requests // when this occurs. ``` 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1339 - 1343) <https://reviews.apache.org/r/38000/#comment168937> Hm.. it looks like you're using the principals here to check that the requests arrive to the endpoint in the right order? How about avoiding the captures here and just specifying "principal1" and "principal2" in the expectation? Also, you're returning "1" and "2" but that's not checked at all? Did you mean to check that the responses below are ordered correctly? 3rdparty/libprocess/src/tests/http_tests.cpp (line 1359) <https://reviews.apache.org/r/38000/#comment168938> Ditto `username:password` here to be more direct, since you already have a comment above. 3rdparty/libprocess/src/tests/http_tests.cpp (line 1366) <https://reviews.apache.org/r/38000/#comment168939> newline here 3rdparty/libprocess/src/tests/http_tests.cpp (line 1369) <https://reviews.apache.org/r/38000/#comment168940> No auth abbreviations please :) 3rdparty/libprocess/src/tests/http_tests.cpp (line 1370) <https://reviews.apache.org/r/38000/#comment168942> Instead of "foo" and "bar", how about "principal1" and "principal2"? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1373 - 1375) <https://reviews.apache.org/r/38000/#comment168941> It's not clear why the sleep is here. If I had to guess, it looks like you wanted to ensure that the both authentication calls were made at this point? If so, we don't need a sleep for this. Let's try to avoid the sleep here if possible. 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1381 - 1387) <https://reviews.apache.org/r/38000/#comment168943> Can you use the other macro I mentioned earlier here as well? 3rdparty/libprocess/src/tests/http_tests.cpp (lines 1389 - 1390) <https://reviews.apache.org/r/38000/#comment168944> If you have two options, you can just use EXPECT_EQ against the options directly: ``` EXPECT_EQ(authResult1.principal, principal1); ``` Although I think you don't need this per my other comment. - Ben Mahler On Dec. 8, 2015, 2:38 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38000/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2015, 2:38 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till > Toenshoff. > > > Bugs: MESOS-3233 > https://issues.apache.org/jira/browse/MESOS-3233 > > > Repository: mesos > > > Description > ------- > > Adds functions which allow libprocess users to register HTTP authenticators. > Overloads `ProcesBase::route()` to allow for registering of authenticating > endpoints. > Includes tests. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/event.hpp > a03824c061c4a0eb865b163999a763635e56744c > 3rdparty/libprocess/include/process/process.hpp > 81c094414d4d5ac5eb593df2a6d14aaacb19a826 > 3rdparty/libprocess/src/authentication_router.hpp > 5777deafd7420324627802a7ab9da9aaa2b46825 > 3rdparty/libprocess/src/process.cpp > e93709d3bb4ac588457bb9331fc05ec5ab539f6d > 3rdparty/libprocess/src/tests/http_tests.cpp > 2de75ca1c7e224c36b534c368e7379dc158aa5bb > > Diff: https://reviews.apache.org/r/38000/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
