> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 49
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line49>
> >
> >     Any reason why these shouldn't just go in authorization_tests.cpp?

I'm following the pattern established with `authorization_tests.cpp` vs 
`master_authorization_tests.cpp`: I'd consider the tests in 
`authorization_tests.cpp` the unit tests of the various actions and 
`master_authorization_tests.cpp` the integration tests, putting those actions 
into use. Because there wasn't an equivalent for agents yet, I created it.


> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, lines 110-114
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line110>
> >
> >     This seems wrong. You don't even bother to reset the authenticator 
> > after you're done?
> >     Why do you even need to unset the authenticator when you already set 
> > `slaveFlags.authenticate_http = false`?

Unfortunately the libprocess environment is shared between tests. An agent sets 
the libprocess-wide authenticator when initialized but doesn't unset it -- 
which is good, because otherwise tests instanciating multiple agents may not 
work correctly. As a consequence, the authenticator may still be set when this 
test case is started, forcing us to have to explicitly unset it here.


> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/slave/http.cpp, line 354
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350392#file1350392line354>
> >
> >     Forgive my lambda-ignorance here, but are you creating this locally 
> > scoped pointer just so that you can expose it to the lambda? Can you not 
> > reference `slave` directly?

I can't use `slave` directly because it's a member of `this`. We can't capture 
`this` in the lambda because it may be no longer known when the lambda is 
called, resulting in a segfault.


> On April 20, 2016, 10:35 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 119
> > <https://reviews.apache.org/r/46203/diff/9/?file=1350395#file1350395line119>
> >
> >     You shouldn't need to change these

I need to, because the agent would fail to start otherwise, see 
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L439


- Jan


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


On April 19, 2016, 2:08 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 2:08 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
>     https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md cd9733002a0940f44edd4e56593a0ca3fe9f77f5 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac 
>   src/authorizer/local/authorizer.cpp 
> c744a16041c2466d3839a37fbee2bf86887bf4e1 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/http.cpp 3908e33ed5b233387790276f6f5d884452087d2c 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Reply via email to