----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66220/#review201969 -----------------------------------------------------------
src/tests/persistent_volume_tests.cpp Lines 501-505 (patched) <https://reviews.apache.org/r/66220/#comment283579> ``` // Disk spaces will be merged if fixture parameter is `NONE`. Bytes totalBytes = GetParam() == NONE ? MegaBytes(4096) : MegaBytes(2048); ``` TBH I'm not a fan of conditioning on the fixture parameters. If a new fixture is created, this can be resolved by having only one disk in `getSlaveResources()` instead. But given the fact that we also do similar conditional branches in other tests, I can live with this. src/tests/persistent_volume_tests.cpp Lines 525-528 (patched) <https://reviews.apache.org/r/66220/#comment283588> Move this just before `acceptOffers` to make it close to its use. src/tests/persistent_volume_tests.cpp Lines 525-528 (patched) <https://reviews.apache.org/r/66220/#comment283590> Move this just before `acceptOffers` to make it close to its use. src/tests/persistent_volume_tests.cpp Lines 554-560 (patched) <https://reviews.apache.org/r/66220/#comment283589> How about merging these two into ``` EXPECT_TRUE(Resources(offer.resources()).contains( allocatedResources(Resources(volume) + addition, frameworkInfo.roles(0)))); ``` Nit for indentation: We indent for 4 more spaces when we open a parenthesis. src/tests/persistent_volume_tests.cpp Lines 554-560 (patched) <https://reviews.apache.org/r/66220/#comment283592> How about merging these two into the following, so the intention of this check is clear: verifying that the offer contains the resources we are going to use in the next ACCEPT call? ``` EXPECT_TRUE(Resources(offer.resources()).contains( allocatedResources(Resources(volume) + addition, frameworkInfo.roles(0)))); ``` Nit for indentation: We indent for 4 more spaces when we open a parenthesis. src/tests/persistent_volume_tests.cpp Lines 654-660 (patched) <https://reviews.apache.org/r/66220/#comment283593> ``` // Disk spaces will be merged if fixture parameter is `NONE`. Bytes totalBytes = GetParam() == NONE ? Megabytes(4096) : Megabytes(2048); ``` src/tests/persistent_volume_tests.cpp Lines 725-733 (patched) <https://reviews.apache.org/r/66220/#comment283596> We don't need this if we just use `Clock::settle()` later for synchronization, as I suggested. src/tests/persistent_volume_tests.cpp Lines 745-748 (patched) <https://reviews.apache.org/r/66220/#comment283597> We could replace this with just `Clock::settle()`. src/tests/persistent_volume_tests.cpp Lines 823 (patched) <https://reviews.apache.org/r/66220/#comment283599> This is not required. src/tests/persistent_volume_tests.cpp Lines 853-855 (patched) <https://reviews.apache.org/r/66220/#comment283603> This is essentially testing the same property as the last `EXPECT_FALSE` so we don't need it. src/tests/persistent_volume_tests.cpp Lines 865 (patched) <https://reviews.apache.org/r/66220/#comment283607> ``` // ... a `GROW_VOLUME` and ... ``` src/tests/persistent_volume_tests.cpp Lines 888 (patched) <https://reviews.apache.org/r/66220/#comment283608> ``` // Expect that the offer does not contain ... ``` Please drop this if I'm wrong. src/tests/persistent_volume_tests.cpp Lines 889-890 (patched) <https://reviews.apache.org/r/66220/#comment283602> ``` EXPECT_TRUE(Resources(offer.resources()).persistentVolumes().empty()); ``` src/tests/persistent_volume_tests.cpp Lines 966-968 (patched) <https://reviews.apache.org/r/66220/#comment283604> No need for this because of the last `EXPECT_FALSE`. src/tests/persistent_volume_tests.cpp Lines 978 (patched) <https://reviews.apache.org/r/66220/#comment283609> ``` // a `SHRINK_VOLUME` and ... ``` src/tests/persistent_volume_tests.cpp Lines 1001 (patched) <https://reviews.apache.org/r/66220/#comment283610> ``` // Expect that the offer does not contain ... ``` src/tests/persistent_volume_tests.cpp Lines 1002-1003 (patched) <https://reviews.apache.org/r/66220/#comment283605> ``` EXPECT_TRUE(Resources(offer.resources()).persistentVolumes().empty()); ``` - Chun-Hung Hsiao On April 19, 2018, 4:39 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66220/ > ----------------------------------------------------------- > > (Updated April 19, 2018, 4:39 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann. > > > Bugs: MESOS-4965 > https://issues.apache.org/jira/browse/MESOS-4965 > > > Repository: mesos > > > Description > ------- > > Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations. > > > Diffs > ----- > > src/tests/persistent_volume_tests.cpp > 4edf781711d9efdb994114aeb6289b6af750b87a > > > Diff: https://reviews.apache.org/r/66220/diff/7/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
