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