DaanHoogland commented on code in PR #9546:
URL: https://github.com/apache/cloudstack/pull/9546#discussion_r1731068544
##########
test/integration/component/test_network_offering.py:
##########
@@ -1395,12 +1395,10 @@ def setUpClass(cls):
@classmethod
def tearDownClass(cls):
- try:
- #Cleanup resources used
- cleanup_resources(cls.apiclient, cls.cleanup)
- except Exception as e:
- raise Exception("Warning: Exception during cleanup : %s" % e)
- return
+ super(TestNOWithOnlySourceNAT, cls).tearDownClass()
Review Comment:
@Pearl1594 , the class level cleanup array should be called `_cleanup`, it
is now called `cleanup` and will be cleaned up at the instance level
`tearDown()` method I think. Better rename it.
class scope methods should use `_cleanup` and instance scope methods (i.e.
the test methods) should use `cleanup`
##########
test/integration/smoke/test_affinity_groups.py:
##########
@@ -107,6 +111,8 @@ def test_DeployVmAntiAffinityGroup(self):
affinitygroupnames=[self.ag.name]
)
+ self.cleanup.append(vm1)
+
Review Comment:
```suggestion
self.cleanup.append(vm1)
```
##########
test/integration/smoke/test_affinity_groups.py:
##########
@@ -183,6 +191,7 @@ def test_DeployVmAffinityGroup(self):
affinitygroupnames=[self.affinity.name]
)
+ self.cleanup.append(vm1)
Review Comment:
```suggestion
self.cleanup.append(vm1)
```
##########
test/integration/component/test_cpu_max_limits.py:
##########
@@ -229,8 +230,8 @@ def test_02_deploy_vm_account_limit_reached(self):
self.apiclient,
self.testdata["service_offering_multiple_cores"]
)
- # Adding to cleanup list after execution
self.cleanup.append(self.service_offering)
+ # Adding to cleanup list after execution
Review Comment:
```suggestion
```
##########
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:
maybe
```suggestion
cls._cleanup.append(cls.virtual_machine_1)
```
was meant?
##########
test/integration/smoke/test_backup_recovery_dummy.py:
##########
@@ -74,10 +76,8 @@ def setUpClass(cls):
@classmethod
def tearDownClass(cls):
+ super(TestDummyBackupAndRecovery, cls).tearDownClass()
try:
Review Comment:
```suggestion
try:
super(TestDummyBackupAndRecovery, cls).tearDownClass()
```
If we fail at cleaning entities we still want to cleanup the settings as
much as we can..
##########
test/integration/smoke/test_templates.py:
##########
@@ -560,6 +561,7 @@ def setUpClass(cls):
domainid=cls.account.domainid
)
cls._cleanup = [
+ cls.virtual_machine,
Review Comment:
again, it'll work but better adhere to the generic cleanup structure
##########
test/integration/smoke/test_scale_vm.py:
##########
@@ -211,7 +211,6 @@ def test_01_scale_vm(self):
serviceofferingid=self.small_offering.id,
mode=self.services["mode"]
)
Review Comment:
```suggestion
)
self.cleanup.append(self.virtual_machine)
```
the line below should not be deleted
##########
test/integration/smoke/test_affinity_groups.py:
##########
@@ -215,6 +224,8 @@ def test_DeployVmAffinityGroup(self):
serviceofferingid=self.service_offering.id,
affinitygroupnames=[self.affinity.name]
)
+
+ self.cleanup.append(vm2)
Review Comment:
```suggestion
self.cleanup.append(vm2)
```
##########
test/integration/smoke/test_affinity_groups.py:
##########
@@ -139,6 +145,8 @@ def test_DeployVmAntiAffinityGroup(self):
serviceofferingid=self.service_offering.id,
affinitygroupnames=[self.ag.name]
)
+
+ self.cleanup.append(vm2)
Review Comment:
```suggestion
self.cleanup.append(vm2)
```
##########
test/integration/smoke/test_vm_life_cycle.py:
##########
@@ -120,6 +120,12 @@ def setUpClass(cls):
mode=cls.services['mode']
)
+ cls.cleanup = [
+ cls.virtual_machine,
+ cls.service_offering,
+ cls.account
+ ]
Review Comment:
a `cleanup` at cls level does not seem right. There is already a
`cls._cleanup` and that should be used in this case.
##########
test/integration/smoke/test_direct_download.py:
##########
@@ -305,6 +305,7 @@ def
test_01_deploy_vm_from_direct_download_template_nfs_storage(self):
nfs_storage_offering =
self.createServiceOffering("TestNFSStorageDirectDownload", "shared", test_tag)
vm = self.deployVM(nfs_storage_offering)
+ self.cleanup.append(vm)
Review Comment:
this should have already happened in `deployVM()`
```suggestion
```
##########
test/integration/smoke/test_service_offerings.py:
##########
@@ -1014,6 +999,7 @@ def getHost(self, hostId=None):
)
cls._cleanup = [
+ cls.vm,
Review Comment:
this will work, but adhering to the structure of letting the parent class do
the cleanup won't work this way. Better refactor.
##########
test/integration/smoke/test_scale_vm.py:
##########
@@ -259,6 +258,7 @@ def test_01_scale_vm(self):
offering_data
)
self.cleanup.append(self.bigger_offering)
+ self.cleanup.append(self.virtual_machine)
Review Comment:
```suggestion
```
##########
test/integration/smoke/test_scale_vm.py:
##########
@@ -599,7 +599,7 @@ def test_04_scale_vm_with_user_account(self):
self.cleanup.append(self.bigger_offering)
else:
self.bigger_offering = self.big_offering
-
+ self.cleanup.append(self.virtual_machine_in_user_account)
Review Comment:
this seems out of place and the assignment and appending of
`self.big_offering` also looks iffy. Note that `self.big_offering` is
obfuscating `cls.big_offering` but `self.cleanup.big_offering` is not
obfuscating `cls._cleanup.big_offering` (and it shouldn't)
```suggestion
```
this line should move up to just below line 551
##########
test/integration/smoke/test_primary_storage.py:
##########
@@ -438,7 +432,6 @@ def setUpClass(cls):
cls.services["service_offerings"]["tiny"],
tags=cls.services["storage_tags"]["a"]
)
- cls._cleanup.append(cls.service_offering_1)
Review Comment:
shouldn't be removed
--
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-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
