> 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 > >
