mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 46792: Parameterized agent authorization tests by endpoint.
Date Fri, 29 Apr 2016 08:15:20 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message