This is an automatically generated e-mail. To reply, visit:

Second part of my earlier review, this time I went throug the test code.

3rdparty/libprocess/src/tests/http_tests.cpp (lines 1214 - 1216)

    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)

    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)

    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)

    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)

    Have you looked at gtest.hpp in libprocess?
    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)

    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)

    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)

    Why is this EXPECT_SOME_EQ instead of EXPECT_EQ? Aren't both of these 

3rdparty/libprocess/src/tests/http_tests.cpp (line 1305)

    In the interest of making this more readable:

3rdparty/libprocess/src/tests/http_tests.cpp (line 1309)

    Why is there an underscore here?

3rdparty/libprocess/src/tests/http_tests.cpp (lines 1310 - 1311)

    Do you need to capture? It looks like you can just place the value in the 
    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)

    Can you just do "user:password" here? The comment seems sufficient to me

3rdparty/libprocess/src/tests/http_tests.cpp (lines 1320 - 1322)

    If you don't need the capture the option, can you use the 

3rdparty/libprocess/src/tests/http_tests.cpp (line 1323)

    If you have two options, you can just use EXPECT_EQ against the options 
    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)

    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)

    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)

    Ditto `username:password` here to be more direct, since you already have a 
comment above.

3rdparty/libprocess/src/tests/http_tests.cpp (line 1366)

    newline here

3rdparty/libprocess/src/tests/http_tests.cpp (line 1369)

    No auth abbreviations please :)

3rdparty/libprocess/src/tests/http_tests.cpp (line 1370)

    Instead of "foo" and "bar", how about "principal1" and "principal2"?

3rdparty/libprocess/src/tests/http_tests.cpp (lines 1373 - 1375)

    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)

    Can you use the other macro I mentioned earlier here as well?

3rdparty/libprocess/src/tests/http_tests.cpp (lines 1389 - 1390)

    If you have two options, you can just use EXPECT_EQ against the options 
    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

Reply via email to