DaanHoogland commented on code in PR #9546:
URL: https://github.com/apache/cloudstack/pull/9546#discussion_r1726971361
##########
test/integration/component/test_acl_listvolume.py:
##########
@@ -340,8 +340,19 @@ def setUpClass(cls):
cls.vm_a_volume = Volume.list(cls.apiclient,
virtualmachineid=cls.vm_a.id)
cls.cleanup = [
+ cls.vm_d1,
+ cls.vm_d1a,
+ cls.vm_d1b,
+ cls.vm_d11,
+ cls.vm_d11a,
+ cls.vm_d11b,
+ cls.vm_d111a,
+ cls.vm_d12a,
+ cls.vm_d12b,
+ cls.vm_d2,
+ cls.vm_a
cls.account_a,
- cls.service_offering,
+ cls.service_offering
Review Comment:
better have these added directly after the respective creates, otherwise no
cleaning will happen on exceptions.
##########
test/integration/component/test_cpu_max_limits.py:
##########
@@ -317,9 +316,11 @@ def test_04_deployVm__account_limit_reached(self):
self.debug("Deploying instance with account: %s" %
self.child_do_admin.name)
- self.createInstance(account=self.child_do_admin,
+ self.vm2 = self.createInstance(account=self.child_do_admin,
service_off=self.service_offering, api_client=api_client_admin)
-
+# Adding to cleanup list after execution
+ self.cleanup.append(self.vm2)
+ self.cleanup.append(self.service_offering)
Review Comment:
see above
##########
test/integration/smoke/test_console_endpoint.py:
##########
@@ -69,8 +69,8 @@ def setUpClass(cls):
)
cls._cleanup = [
- cls.service_offering,
cls.vm1,
+ cls.service_offering,
Review Comment:
here too, better append as you go and call super().tearDown()
##########
test/integration/smoke/test_primary_storage.py:
##########
@@ -479,6 +478,8 @@ def setUpClass(cls):
hypervisor=cls.hypervisor,
mode=cls.zone.networktype
)
+ cls._cleanup.append(cls.virtual_machine_1)
+ cls._cleanup.append(cls.service_offering_1)
Review Comment:
again, should not happen here, look at the tearDownClass implementation
##########
test/integration/smoke/test_affinity_groups.py:
##########
@@ -240,6 +251,14 @@ def test_DeployVmAffinityGroup(self):
self.assertEqual(host_of_vm1, host_of_vm2,
msg="Both VMs of affinity group %s are on different hosts" %
self.affinity.name)
+
+ def tearDown(self):
+ try:
+ # Clean up, terminate the created instance, volumes and snapshots
+ cleanup_resources(self.apiclient, self.cleanup)
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
Review Comment:
```suggestion
def tearDown(self):
super(<cls-name>, self).tearDown()
```
(replace \<cls-name\> !)
##########
test/integration/smoke/test_primary_storage.py:
##########
@@ -336,6 +335,7 @@ def test_01_add_primary_storage_disabled_host(self):
serviceofferingid=service_offering.id
)
self.cleanup.append(self.virtual_machine)
+ self.cleanup.append(service_offering)
Review Comment:
should not be happening here. You are probably doing this because the
teardown implementation is no good
##########
test/integration/component/test_network_offering.py:
##########
@@ -1449,14 +1449,16 @@ def test_create_network_with_snat(self):
self.debug("Deploying VM in account: %s on the network %s" %
(self.account.name, self.network.id))
# Spawn an instance in that network
- VirtualMachine.create(
+ self.vm1 = VirtualMachine.create(
self.apiclient,
self.services["virtual_machine"],
accountid=self.account.name,
domainid=self.account.domainid,
serviceofferingid=self.service_offering.id,
networkids=[str(self.network.id)]
)
+ self.cleanup.append(self.vm1)
+ self.cleanup.append(self.network)
Review Comment:
this should move up and the tearDownClass method implementation should call
super(..)
better clean the cleanup up ;)
cls._cleanup and self.cleanup
##########
test/integration/smoke/test_resource_accounting.py:
##########
@@ -222,6 +218,8 @@ def test_01_so_removal_resource_update(self):
self.debug("Deployed VM in account: %s, ID: %s" % (self.account.name,
vm_2.id))
self.cleanup.append(vm_2)
+ self.cleanup.append(self.service_offering_it_1)
+ self.cleanup.append(self.service_offering_it_2)
Review Comment:
nooooo, ;) look at the tearDown methods
##########
test/integration/smoke/test_backup_recovery_dummy.py:
##########
@@ -62,7 +62,7 @@ def setUpClass(cls):
cls.vm = VirtualMachine.create(cls.api_client, cls.services["small"],
accountid=cls.account.name,
domainid=cls.account.domainid,
serviceofferingid=cls.offering.id,
mode=cls.services["mode"])
- cls._cleanup = [cls.offering, cls.account]
+ cls._cleanup = [cls.vm, cls.offering, cls.account]
Review Comment:
scary, better append as you go
##########
test/integration/smoke/test_reset_vm_on_reboot.py:
##########
@@ -76,6 +76,7 @@ def setUpClass(cls):
mode=cls.services["mode"]
)
cls._cleanup = [
+ cls.virtual_machine
Review Comment:
scary
##########
test/integration/component/test_cpu_max_limits.py:
##########
@@ -242,8 +241,10 @@ def test_02_deploy_vm_account_limit_reached(self):
self.debug("Deploying instance with account: %s" %
self.child_do_admin.name)
- self.createInstance(account=self.child_do_admin,
+ self.vm1 = self.createInstance(account=self.child_do_admin,
service_off=self.service_offering, api_client=api_client_admin)
+ self.cleanup.append(self.vm1)
+ self.cleanup.append(self.service_offering)
Review Comment:
seems like this should happen inside the `self.createInstance(..)` call
##########
tools/marvin/setup.py:
##########
@@ -27,7 +27,7 @@
raise RuntimeError("python setuptools is required to build Marvin")
-VERSION = "4.20.0.0-SNAPSHOT"
+VERSION = "4.20.0.0"
Review Comment:
please remove this from your PR
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
