----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66227/#review201366 -----------------------------------------------------------
src/tests/api_tests.cpp Lines 3688 (patched) <https://reviews.apache.org/r/66227/#comment282564> Let's try to split the test into two so each test verifies one operation. Also, what's the reason of disabling the test on Windows? And do you think it is reasonable to have tests that leave `agent_id` unset, to expose the code path of validating the existence of `agent_id`? src/tests/api_tests.cpp Lines 3692-3695 (patched) <https://reviews.apache.org/r/66227/#comment282566> This is not needed. The overloaded `MasterAPITest::CreateMasterFlags()` already extends the allocation interval. src/tests/api_tests.cpp Lines 3700 (patched) <https://reviews.apache.org/r/66227/#comment282567> Since the API is not called XXXVolumes, let's just say ``` // ... use it in the API calls. ``` so need to break the line here. src/tests/api_tests.cpp Lines 3711-3719 (patched) <https://reviews.apache.org/r/66227/#comment282569> No need to add the `RESOURCE_PROVIDER` capability since this is set up by default in 1.6. In fact, we should remove it here so if the default capabilities are changed in the future, we will be able to capture the problem through this test. src/tests/api_tests.cpp Lines 3750 (patched) <https://reviews.apache.org/r/66227/#comment282570> Let's move this to the beginning of this test. src/tests/api_tests.cpp Lines 3766 (patched) <https://reviews.apache.org/r/66227/#comment282572> Not sure if this is necessary. Could you justify the need of this synchronization? src/tests/api_tests.cpp Lines 3768 (patched) <https://reviews.apache.org/r/66227/#comment282573> Is this necessary? src/tests/api_tests.cpp Lines 3787 (patched) <https://reviews.apache.org/r/66227/#comment282574> Do we need this? IIRC an event-based allocation will be triggered (and thus an offer will be sent) when a framework is added. src/tests/api_tests.cpp Lines 3791 (patched) <https://reviews.apache.org/r/66227/#comment282575> ``` Offer offer = offersAfterCreate->at(0); ``` src/tests/api_tests.cpp Lines 3801-3809 (patched) <https://reviews.apache.org/r/66227/#comment282576> Let's do the following for readability: ``` v1::master::Call::GrowVolume* growVolume = v1GrowVolumeCall.mutable_grow_volume(); growVolume->mutable_agent_id()->CopyFrom(evolve(slaveId)); growVolume->mutable_volume()->CopyFrom(evolve(volume)); growVolume->mutable_addition)->CopyFrom(evolve(addition)); ``` src/tests/api_tests.cpp Lines 3821 (patched) <https://reviews.apache.org/r/66227/#comment282578> No need for this since the clock is paused. src/tests/api_tests.cpp Lines 3834 (patched) <https://reviews.apache.org/r/66227/#comment282577> Again, I'm not convinced that this is required. src/tests/api_tests.cpp Lines 3836 (patched) <https://reviews.apache.org/r/66227/#comment282579> Do we need this? src/tests/api_tests.cpp Lines 3841 (patched) <https://reviews.apache.org/r/66227/#comment282580> ``` offer = offersAfterGrow->at(0); ``` src/tests/api_tests.cpp Lines 3859-3866 (patched) <https://reviews.apache.org/r/66227/#comment282581> ``` v1::master::Call::ShrinkVolume* shrinkVolume = v1ShrinkVolumeCall.mutable_shrink_volume(); shrinkVolume->... ``` src/tests/api_tests.cpp Lines 3878 (patched) <https://reviews.apache.org/r/66227/#comment282583> No need for this as the clock is paused. src/tests/api_tests.cpp Lines 3891 (patched) <https://reviews.apache.org/r/66227/#comment282582> Again, not sure if this is needed. src/tests/api_tests.cpp Lines 3893 (patched) <https://reviews.apache.org/r/66227/#comment282584> Also it seems to me that this is not needed. src/tests/api_tests.cpp Lines 3898 (patched) <https://reviews.apache.org/r/66227/#comment282585> ``` offer = offerAfterShrink->at(0); ``` src/tests/api_tests.cpp Lines 3903-3945 (patched) <https://reviews.apache.org/r/66227/#comment282586> If I'm not mistaken, the test teardown would clean up the work dir, and thus the persistent volume, so this is not required. Can you verify that? - Chun-Hung Hsiao On April 11, 2018, 9:21 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66227/ > ----------------------------------------------------------- > > (Updated April 11, 2018, 9:21 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann. > > > Bugs: MESOS-8747 > https://issues.apache.org/jira/browse/MESOS-8747 > > > Repository: mesos > > > Description > ------- > > Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API. > > > Diffs > ----- > > src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 > > > Diff: https://reviews.apache.org/r/66227/diff/4/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
