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




src/tests/slave_authorization_tests.cpp (line 65)
<https://reviews.apache.org/r/46792/#comment195007>

    Any reason you couldn't make this just a constant? As this isn't externally 
visible I see e.g., not lifetime problems.



src/tests/slave_authorization_tests.cpp (lines 71 - 80)
<https://reviews.apache.org/r/46792/#comment195012>

    I am not sure we want to document the behavior of certain tests with the 
fixture since the comment might go out of sync as tests are added/adjusted. 
Could you focus the discussion here on the behavior of the fixture and move 
test-specific notes and todos down to the tests?



src/tests/slave_authorization_tests.cpp (lines 148 - 150)
<https://reviews.apache.org/r/46792/#comment195008>

    Please call out that you need to do this since googletest cannot compose 
both type and value parameterized tests out of the box.
    
    The downside of doing this manually is that failures in `testCaseBody` will 
not mention the value of the "parameter" `endpoint`. Please update any checks 
and assertions in there to mention `endpoint` in case of failures.
    
    Note that there's also the possibility to shoehorn values into types, see 
e.g., 
https://groups.google.com/d/msg/googletestframework/3d8H5J5Isnw/RtXYy12SCSgJ, 
but I am not sure this wouldn't be perceived as trying too hard.



src/tests/slave_authorization_tests.cpp (lines 198 - 200)
<https://reviews.apache.org/r/46792/#comment195009>

    ditto.



src/tests/slave_authorization_tests.cpp (line 212)
<https://reviews.apache.org/r/46792/#comment195010>

    This comment seems redundant since it trivially reiterates what the code 
below says.


- Benjamin Bannier


On April 28, 2016, 7:24 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46792/
> -----------------------------------------------------------
> 
> (Updated April 28, 2016, 7:24 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently, only '/flags' endpoint is being tested by the
> `SlaveAuthorizationTest` test fixture. However, the same
> tests can be applied to any endpoint that consults
> authorizer for GET requests.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_authorization_tests.cpp 
> d3ab0835c8d2464a65f382087d914412dc573b44 
> 
> Diff: https://reviews.apache.org/r/46792/diff/
> 
> 
> Testing
> -------
> 
> On Mac OS 10.10.4:
> `make check`
> `GTEST_FILTER="*SlaveEndpointTest*:*SlaveAuthorizationTest*" 
> ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Reply via email to