Re: [PR] refactoring a few smoke tests [cloudstack]

["DaanHoogland (via GitHub)" <gi...@apache.org>]

DaanHoogland commented on code in PR #11748:
URL: https://github.com/apache/cloudstack/pull/11748#discussion_r2393901744


##########
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py:
##########
@@ -174,8 +179,8 @@ def test_02_copy_snapshot_multi_pools(self):
         self.snapshot_id = snapshot.id
         Snapshot.copy(self.userapiclient, self.snapshot_id, zone_ids=[str(self.additional_zone.id)], source_zone_id=self.zone.id, usestoragereplication=True)
         self.helper.verify_snapshot_copies(self.userapiclient, self.snapshot_id, [self.zone.id, self.additional_zone.id])
-        time.sleep(420)
-        Snapshot.delete(snapshot, self.userapiclient)
+
+        self._cleanup.append(snapshot)

Review Comment:
   this should move to just below the `create` .
   also, is the copy a separate entity and should it be in the cleanup list as well?



##########
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py:
##########
@@ -160,8 +166,7 @@ def test_01_take_snapshot_multi_zone(self):
         snapshot = Snapshot.create(self.userapiclient, volume_id=self.volume.id, zoneids=[str(self.additional_zone.id)], usestoragereplication=True)
         self.snapshot_id = snapshot.id
         self.helper.verify_snapshot_copies(self.userapiclient, self.snapshot_id, [self.zone.id, self.additional_zone.id])
-        time.sleep(420)
-        Snapshot.delete(snapshot, self.userapiclient)
+        self._cleanup.append(snapshot)

Review Comment:
   ```suggestion
           self._cleanup.append(snapshot)
           self.snapshot_id = snapshot.id
           self.helper.verify_snapshot_copies(self.userapiclient, self.snapshot_id, [self.zone.id, self.additional_zone.id])
   ```



##########
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py:
##########
@@ -249,7 +253,96 @@ def test_06_take_snapshot_multi_zone_create_template_additional_zone(self):
         self.template = self.helper.create_snapshot_template(self.userapiclient, self.services, self.snapshot_id, self.additional_zone.id)
         if self.additional_zone.id != self.template.zoneid:
             self.fail("Template from snapshot not created in the additional zone")
+        self._cleanup.append(snapshot)

Review Comment:
   this should move up a couple of lines to be just below the matching create



##########
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py:
##########
@@ -286,6 +280,7 @@ def deploy_vm_from_snapshot(self, snapshot):
                                                 snapshotid=snapshot.id,
                                                 mode="basic",
                                                 )
+        self._cleanup.append(virtual_machine)

Review Comment:
   ```suggestion
           self.cleanup.append(virtual_machine)
   ```
   maybe?



##########
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py:
##########
@@ -190,7 +195,7 @@ def test_03_take_snapshot_multi_pools_delete_single_zone(self):
         time.sleep(420)
         Snapshot.delete(snapshot, self.userapiclient, self.zone.id)
         self.helper.verify_snapshot_copies(self.userapiclient, self.snapshot_id, [self.additional_zone.id])
-        self.cleanup.append(snapshot)
+        self._cleanup.append(snapshot)

Review Comment:
   isn’t the snapshot to be removed from the cleanup list here?



##########
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py:
##########
@@ -311,6 +306,7 @@ def create_volume_from_snapshot_deploy_vm(self, snapshotid):
                                                 volumeid=volume.id,
                                                 mode="basic",
                                                 )
+        self._cleanup.append(virtual_machine)

Review Comment:
   ```suggestion
           self.cleanup.append(virtual_machine)
   ```
   ?
   (as it is an operation on self and not on cls)



##########
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py:
##########
@@ -227,13 +232,12 @@ def test_05_take_snapshot_multi_zone_create_volume_additional_zone(self):
                 self.apiclient,
                 service
             )
-            self.cleanup.append(self.disk_offering)
+            self._cleanup.append(self.disk_offering)
             disk_offering_id = self.disk_offering.id
 
         self.volume = Volume.create(self.userapiclient, {"diskname":"StorPoolDisk-1" }, snapshotid=self.snapshot_id, zoneid=self.zone.id, diskofferingid=disk_offering_id)
-        self.cleanup.append(self.volume)
-        time.sleep(420)
-        Snapshot.delete(snapshot, self.userapiclient)
+        self._cleanup.append(self.volume)
+        self._cleanup.append(snapshot)

Review Comment:
   why is this added at this point?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org