You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Adam B <ad...@mesosphere.io> on 2016/02/27 13:21:47 UTC
Re: Review Request 41790: Add tests for /weights endpoint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41790/#review121077
-----------------------------------------------------------
Sorry this took me forever to get to. Looks pretty good except for some indentation and some unnecessary lines.
I'd also like to see a couple of registrar tests, like AlexR suggested.
src/tests/dynamic_weights_tests.cpp (lines 71 - 74)
<https://reviews.apache.org/r/41790/#comment182667>
You can just use `process::http::Headers` instead.
src/tests/dynamic_weights_tests.cpp (line 108)
<https://reviews.apache.org/r/41790/#comment182674>
s/Req/Request/ (we don't like abbrev's)
src/tests/dynamic_weights_tests.cpp (line 113)
<https://reviews.apache.org/r/41790/#comment182675>
s/json/"%s"/ - no reaon to create a local variable for a string constant that isn't reused.
src/tests/dynamic_weights_tests.cpp (lines 134 - 150)
<https://reviews.apache.org/r/41790/#comment182676>
Seems like this should be an if/else, rather than always evaluating JSON::parse("[...]") even if the value's getting thrown away. Maybe something more like:
```Try<JSON::Value> expected;
if (weights == "") {
expected = "[..short..]";
} else if (weights == WEIGHTS) {
expected = "[...longer...]";
} else {
expected = Error("Unexpected weights string.");
}```
src/tests/dynamic_weights_tests.cpp (lines 215 - 219)
<https://reviews.apache.org/r/41790/#comment182677>
This seems like an arbitrary indentation. Either align with the parentheses (too far over in this case), or indent 4 spaces in from the previous line.
src/tests/dynamic_weights_tests.cpp (lines 215 - 219)
<https://reviews.apache.org/r/41790/#comment182678>
This seems like an arbitrary indentation. Either align with the parentheses (too far over in this case), or indent 4 spaces in from the previous line.
src/tests/dynamic_weights_tests.cpp (line 227)
<https://reviews.apache.org/r/41790/#comment182679>
s/filed/field/
src/tests/dynamic_weights_tests.cpp (lines 236 - 240)
<https://reviews.apache.org/r/41790/#comment182680>
Fix the indent again, everywhere.
src/tests/dynamic_weights_tests.cpp (line 247)
<https://reviews.apache.org/r/41790/#comment182681>
Soon this line won't be necessary, but we'll try to get your patch committed before Joseph's so he's the one who has to rebase, not you. :)
src/tests/dynamic_weights_tests.cpp (line 290)
<https://reviews.apache.org/r/41790/#comment182682>
Please add another Bad Request test for a non-numeric weight value, like "role1=two"
src/tests/dynamic_weights_tests.cpp (line 322)
<https://reviews.apache.org/r/41790/#comment182683>
s/empty (Only a space) role/empty role (only a space)/
src/tests/dynamic_weights_tests.cpp (line 400)
<https://reviews.apache.org/r/41790/#comment182687>
s/does not/not/
src/tests/dynamic_weights_tests.cpp (line 472)
<https://reviews.apache.org/r/41790/#comment182689>
You can remove the `UpdateWeightsWithImplicitRoles` test, since this test already tests implicit roles.
src/tests/dynamic_weights_tests.cpp (line 523)
<https://reviews.apache.org/r/41790/#comment182690>
This line should be unnecessary. If there are no credentials in the request, and authenticate_http=false (meaning authn is not required), then your request should be allowed through without authn.
In this setup, the master would still use its --credentials to authenticate any client that tries to use credentials.
src/tests/dynamic_weights_tests.cpp (lines 553 - 556)
<https://reviews.apache.org/r/41790/#comment182691>
These lines should be unnecessary if the ACLs are restrictive by default.
- Adam B
On Jan. 25, 2016, 7:47 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41790/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2016, 7:47 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add tests for /weights endpoint.
>
>
> Diffs
> -----
>
> src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6
> src/tests/dynamic_weights_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41790/diff/
>
>
> Testing
> -------
>
> Make and Make check successfully!
>
> ./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
> [==========] Running 11 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 11 tests from DynamicWeightsTest
> [ RUN ] DynamicWeightsTest.PutInvalidRequest
> [ OK ] DynamicWeightsTest.PutInvalidRequest (102 ms)
> [ RUN ] DynamicWeightsTest.ZeroWeight
> [ OK ] DynamicWeightsTest.ZeroWeight (38 ms)
> [ RUN ] DynamicWeightsTest.NegativeWeight
> [ OK ] DynamicWeightsTest.NegativeWeight (38 ms)
> [ RUN ] DynamicWeightsTest.MissingRole
> [ OK ] DynamicWeightsTest.MissingRole (43 ms)
> [ RUN ] DynamicWeightsTest.UnknownRole
> [ OK ] DynamicWeightsTest.UnknownRole (36 ms)
> [ RUN ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
> [ OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (44 ms)
> [ RUN ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles
> [ OK ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles (52 ms)
> [ RUN ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
> [ OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (40 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (32 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
> [ OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal (37 ms)
> [ RUN ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (30 ms)
> [----------] 11 tests from DynamicWeightsTest (492 ms total)
>
> [----------] Global test environment tear-down
> [==========] 11 tests from 1 test case ran. (503 ms total)
> [ PASSED ] 11 tests.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41790: Add tests for /weights endpoint.
Posted by Adam B <ad...@mesosphere.io>.
> On Feb. 27, 2016, 4:21 a.m., Adam B wrote:
> > Sorry this took me forever to get to. Looks pretty good except for some indentation and some unnecessary lines.
> > I'd also like to see a couple of registrar tests, like AlexR suggested.
>
> Yongqiao Wang wrote:
> Thanks Adam. I will add a couple of registrar tests later in another patches, is it OK?
Sure, it can go in a later patch, but let's make sure not to forget it.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41790/#review121077
-----------------------------------------------------------
On Feb. 27, 2016, 5:55 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41790/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2016, 5:55 a.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add tests for /weights endpoint.
>
>
> Diffs
> -----
>
> src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5
> src/tests/dynamic_weights_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41790/diff/
>
>
> Testing
> -------
>
> Make and Make check successfully!
>
> ./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
> [==========] Running 11 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 11 tests from DynamicWeightsTest
> [ RUN ] DynamicWeightsTest.PutInvalidRequest
> [ OK ] DynamicWeightsTest.PutInvalidRequest (85 ms)
> [ RUN ] DynamicWeightsTest.ZeroWeight
> [ OK ] DynamicWeightsTest.ZeroWeight (46 ms)
> [ RUN ] DynamicWeightsTest.NegativeWeight
> [ OK ] DynamicWeightsTest.NegativeWeight (49 ms)
> [ RUN ] DynamicWeightsTest.NonNumericWeight
> [ OK ] DynamicWeightsTest.NonNumericWeight (49 ms)
> [ RUN ] DynamicWeightsTest.MissingRole
> [ OK ] DynamicWeightsTest.MissingRole (50 ms)
> [ RUN ] DynamicWeightsTest.UnknownRole
> [ OK ] DynamicWeightsTest.UnknownRole (49 ms)
> [ RUN ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
> [ OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (45 ms)
> [ RUN ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
> [ OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (45 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (37 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
> [ OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal (36 ms)
> [ RUN ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (35 ms)
> [----------] 11 tests from DynamicWeightsTest (527 ms total)
>
> [----------] Global test environment tear-down
> [==========] 11 tests from 1 test case ran. (536 ms total)
> [ PASSED ] 11 tests.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41790: Add tests for /weights endpoint.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Feb. 27, 2016, 12:21 p.m., Adam B wrote:
> > Sorry this took me forever to get to. Looks pretty good except for some indentation and some unnecessary lines.
> > I'd also like to see a couple of registrar tests, like AlexR suggested.
Thanks Adam. I will add a couple of registrar tests later in another patches, is it OK?
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41790/#review121077
-----------------------------------------------------------
On Jan. 25, 2016, 3:47 p.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41790/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2016, 3:47 p.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add tests for /weights endpoint.
>
>
> Diffs
> -----
>
> src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6
> src/tests/dynamic_weights_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41790/diff/
>
>
> Testing
> -------
>
> Make and Make check successfully!
>
> ./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
> [==========] Running 11 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 11 tests from DynamicWeightsTest
> [ RUN ] DynamicWeightsTest.PutInvalidRequest
> [ OK ] DynamicWeightsTest.PutInvalidRequest (102 ms)
> [ RUN ] DynamicWeightsTest.ZeroWeight
> [ OK ] DynamicWeightsTest.ZeroWeight (38 ms)
> [ RUN ] DynamicWeightsTest.NegativeWeight
> [ OK ] DynamicWeightsTest.NegativeWeight (38 ms)
> [ RUN ] DynamicWeightsTest.MissingRole
> [ OK ] DynamicWeightsTest.MissingRole (43 ms)
> [ RUN ] DynamicWeightsTest.UnknownRole
> [ OK ] DynamicWeightsTest.UnknownRole (36 ms)
> [ RUN ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
> [ OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (44 ms)
> [ RUN ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles
> [ OK ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles (52 ms)
> [ RUN ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
> [ OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (40 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (32 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
> [ OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal (37 ms)
> [ RUN ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (30 ms)
> [----------] 11 tests from DynamicWeightsTest (492 ms total)
>
> [----------] Global test environment tear-down
> [==========] 11 tests from 1 test case ran. (503 ms total)
> [ PASSED ] 11 tests.
>
>
> Thanks,
>
> Yongqiao Wang
>
>
Re: Review Request 41790: Add tests for /weights endpoint.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Feb. 27, 2016, 12:21 p.m., Adam B wrote:
> > Sorry this took me forever to get to. Looks pretty good except for some indentation and some unnecessary lines.
> > I'd also like to see a couple of registrar tests, like AlexR suggested.
>
> Yongqiao Wang wrote:
> Thanks Adam. I will add a couple of registrar tests later in another patches, is it OK?
>
> Adam B wrote:
> Sure, it can go in a later patch, but let's make sure not to forget it.
I have logged a separated JIRA ticket #MESOS-4797 to trace this task.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41790/#review121077
-----------------------------------------------------------
On Feb. 27, 2016, 1:55 p.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41790/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2016, 1:55 p.m.)
>
>
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
>
>
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add tests for /weights endpoint.
>
>
> Diffs
> -----
>
> src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5
> src/tests/dynamic_weights_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41790/diff/
>
>
> Testing
> -------
>
> Make and Make check successfully!
>
> ./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
> [==========] Running 11 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 11 tests from DynamicWeightsTest
> [ RUN ] DynamicWeightsTest.PutInvalidRequest
> [ OK ] DynamicWeightsTest.PutInvalidRequest (85 ms)
> [ RUN ] DynamicWeightsTest.ZeroWeight
> [ OK ] DynamicWeightsTest.ZeroWeight (46 ms)
> [ RUN ] DynamicWeightsTest.NegativeWeight
> [ OK ] DynamicWeightsTest.NegativeWeight (49 ms)
> [ RUN ] DynamicWeightsTest.NonNumericWeight
> [ OK ] DynamicWeightsTest.NonNumericWeight (49 ms)
> [ RUN ] DynamicWeightsTest.MissingRole
> [ OK ] DynamicWeightsTest.MissingRole (50 ms)
> [ RUN ] DynamicWeightsTest.UnknownRole
> [ OK ] DynamicWeightsTest.UnknownRole (49 ms)
> [ RUN ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
> [ OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (45 ms)
> [ RUN ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
> [ OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (45 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (37 ms)
> [ RUN ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
> [ OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal (36 ms)
> [ RUN ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
> [ OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (35 ms)
> [----------] 11 tests from DynamicWeightsTest (527 ms total)
>
> [----------] Global test environment tear-down
> [==========] 11 tests from 1 test case ran. (536 ms total)
> [ PASSED ] 11 tests.
>
>
> Thanks,
>
> Yongqiao Wang
>
>