You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2018/09/19 13:12:12 UTC
***UNCHECKED*** Re: Review Request 68758: Added unit tests for
adding/updating invalid resource provider configs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68758/#review208762
-----------------------------------------------------------
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 508-511 (patched)
<https://reviews.apache.org/r/68758/#comment292906>
We don't seem to explicitly need this here. Is this something we could change in the fixture instead?
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 530 (patched)
<https://reviews.apache.org/r/68758/#comment292907>
Let's add a comment that `ResourceProviderInfo::storage` is required.
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 538-539 (patched)
<https://reviews.apache.org/r/68758/#comment292908>
Let's use `os::ls` instead (needs an additional include).
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 757 (patched)
<https://reviews.apache.org/r/68758/#comment292915>
`s/not allowed/rejected/`
Any other postconditions we could test for?
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 762-763 (patched)
<https://reviews.apache.org/r/68758/#comment292909>
Move to fixture? I see that this might add some value here nevertheless as we go through offer cycles, but the proper alternative would be to run with paused clock instead to remove the allocator behavior from the test runtime. Is this still impossible?
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 786 (patched)
<https://reviews.apache.org/r/68758/#comment292910>
Nit: would fit on a single line.
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 820 (patched)
<https://reviews.apache.org/r/68758/#comment292911>
Comment that `ResourceProviderInfo.storage` is needed.
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 826 (patched)
<https://reviews.apache.org/r/68758/#comment292914>
Can and should we also check that existing RP is not terminated?
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 830 (patched)
<https://reviews.apache.org/r/68758/#comment292912>
`os::ls`?
src/tests/agent_resource_provider_config_api_tests.cpp
Lines 830 (patched)
<https://reviews.apache.org/r/68758/#comment292913>
`os::ls`?
- Benjamin Bannier
On Sept. 19, 2018, 6:50 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68758/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2018, 6:50 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-9228
> https://issues.apache.org/jira/browse/MESOS-9228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added unit tests for adding/updating invalid resource provider configs.
>
>
> Diffs
> -----
>
> src/tests/agent_resource_provider_config_api_tests.cpp e6a68bae1a9e3e773ea45deae4951664ab81a857
>
>
> Diff: https://reviews.apache.org/r/68758/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68758: Added unit tests for adding/updating invalid
resource provider configs.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Sept. 19, 2018, 1:12 p.m., Benjamin Bannier wrote:
> > src/tests/agent_resource_provider_config_api_tests.cpp
> > Lines 508-511 (patched)
> > <https://reviews.apache.org/r/68758/diff/1/?file=2090232#file2090232line508>
> >
> > We don't seem to explicitly need this here. Is this something we could change in the fixture instead?
Fixed in r/68777.
> On Sept. 19, 2018, 1:12 p.m., Benjamin Bannier wrote:
> > src/tests/agent_resource_provider_config_api_tests.cpp
> > Lines 826 (patched)
> > <https://reviews.apache.org/r/68758/diff/1/?file=2090232#file2090232line826>
> >
> > Can and should we also check that existing RP is not terminated?
We can manipulate the clock similar to the `*Idempotent` tests, but I feel it a bit unnecessary here since that would make the test less concise and we already checked that the file doesn't change. If you still think we should do the check I can do that.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68758/#review208762
-----------------------------------------------------------
On Sept. 19, 2018, 4:50 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68758/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2018, 4:50 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-9228
> https://issues.apache.org/jira/browse/MESOS-9228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added unit tests for adding/updating invalid resource provider configs.
>
>
> Diffs
> -----
>
> src/tests/agent_resource_provider_config_api_tests.cpp e6a68bae1a9e3e773ea45deae4951664ab81a857
>
>
> Diff: https://reviews.apache.org/r/68758/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>