----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66220/#review201180 -----------------------------------------------------------
src/tests/persistent_volume_tests.cpp Lines 453 (patched) <https://reviews.apache.org/r/66220/#comment282230> If I'm not mistaken, this test does the following steps: 1. Create a persistent volume 2. Launch a task to write a file in the volume 3. Grow the volume 4. Check if the file exists in the volume 5. Shrink the volume 6. Check if the file exists in the volume The majority of this test is to implement Step 1 and 2, which are already testsed in `AccessPersistentVolume` and not really relevent to the growing and shrinking functions. How about splitting this test into two tests, `GrowPersistentVolume` and `ShrinkPersistentVolume`, where the first one only does Step 1, 3, 4 and the second only does Step 1, 5, 6? The creation of the file can be done with a simple `os::write()` to the volume path instead of using a task. Also, I'm not sure if the file check is really necessary. If not, we can just check the existence of the volume path in Step 4 and 6. Furthermore, we could even choose not to check the existence of the volume path. In this case, we could make the persistent volume in `slaveFlags.resources` from the very beginning, and completely get rid of Step 4 and 6, and thus the whole test would be much easier to read. src/tests/persistent_volume_tests.cpp Lines 455-459 (patched) <https://reviews.apache.org/r/66220/#comment282229> Or alternatively, we can do the following: ``` class PersistentVolumeResizeTest : public PersistentVolumeTest {}; INSTANTIATE_TEST_CASE_P( DiskResource, /* Not sure if the same name can be used twice */ PersistentVolumeResizeTest, ::testing::Values( PersistentVolumeSourceType::NONE, PersistentVolumeSourceType::PATH)); TEST_P(PersistentVolumeResizeTest, GrowAndShrink) { ... } ``` And remove the conditional return. src/tests/persistent_volume_tests.cpp Lines 466 (patched) <https://reviews.apache.org/r/66220/#comment282228> The `roles` master flag is deprecated and there's no need to set it. src/tests/persistent_volume_tests.cpp Lines 519-570 (patched) <https://reviews.apache.org/r/66220/#comment282231> I feel the whole task launch doesn't need to be tested in this unit test. See my comments above. src/tests/persistent_volume_tests.cpp Lines 584 (patched) <https://reviews.apache.org/r/66220/#comment282234> Let's use all remaining free disk in the offer, so we can check that the consumed resource is indeed used and does not show up in the subsequent offer. If you use only 1G here, then there will be 1G free disk remaining and this would make the above check hard. src/tests/persistent_volume_tests.cpp Lines 597-603 (patched) <https://reviews.apache.org/r/66220/#comment282232> I agree with Greg on this. Let's not check the internal `ApplyOperationMessage`, but check the public behavior: make the framework wait for a subsequent offer and then check if the offer has the grown volume. Note that `Resources(offer.resources()).persistentVolumes().contains(grownVolume)` is not enough. We should check if the size meets the expectation. Also, let's check that the new offer doesn't contain the disk resource set up in `addition`. src/tests/persistent_volume_tests.cpp Lines 650-655 (patched) <https://reviews.apache.org/r/66220/#comment282233> Since the test checks if the subsequent offer contains the shrunk volume, there is no need to check the internal `ApplyOperationMessage`. src/tests/persistent_volume_tests.cpp Lines 677-678 (patched) <https://reviews.apache.org/r/66220/#comment282235> This check is not enough. We should check if the size of the persistent volume in the offer meets the expectation. Also, let's check that the freed disk resource shows up in the offer. - Chun-Hung Hsiao On April 11, 2018, 9:19 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66220/ > ----------------------------------------------------------- > > (Updated April 11, 2018, 9:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations. > > > Diffs > ----- > > src/tests/persistent_volume_tests.cpp > 4edf781711d9efdb994114aeb6289b6af750b87a > > > Diff: https://reviews.apache.org/r/66220/diff/4/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
