> On April 15, 2016, 11:22 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, line 551
> > <https://reviews.apache.org/r/46260/diff/1/?file=1346281#file1346281line551>
> >
> >     Can you please add a test where authentication actually works? (for 
> > saftey)

I wonder if we really need this? I've written some previous tests along those 
lines (which test full endpoint functionality both with and without 
authentication), but after some conversations with bmahler recently I'm not 
sure this is a good idea. Since we don't currently use the authenticated 
`principal` at all in the endpoint handler, we really just need to test here 
that unauthenticated requests receive an `Unauthorized` response. The 
`HttpAuthenticationTest.Authenticated` test in libprocess already tests that a 
properly authenticated request will succeed in reaching the HTTP endpoint and 
passing the correct principal to the handler, so we don't need to test that 
again. Once authorization is enabled on this endpoint, it will make sense to 
have successfully authenticated test cases in order to test the authorization 
code, but at this point I think it would introduce unnecessary tests and 
increase the code maintenance burden. What do you think?


- Greg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46260/#review129098
-----------------------------------------------------------


On April 15, 2016, 6:58 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46260/
> -----------------------------------------------------------
> 
> (Updated April 15, 2016, 6:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4902
>     https://issues.apache.org/jira/browse/MESOS-4902
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The test `MetricsTest.SnapshotAuthenticationEnabled`
> is added to the libprocess tests in this patch.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp 
> b84dc8d858f58bc9f52b218b7153510417cf34c2 
> 
> Diff: https://reviews.apache.org/r/46260/diff/
> 
> 
> Testing
> -------
> 
> `sudo make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Reply via email to