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