> On April 15, 2016, 1:22 p.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)
> 
> Greg Mann wrote:
>     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?

Fair enough


- Alexander


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


On April 15, 2016, 8: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, 8: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