Copilot commented on code in PR #11945:
URL: https://github.com/apache/cloudstack/pull/11945#discussion_r2478634346
##########
test/integration/smoke/test_migration.py:
##########
@@ -124,54 +126,39 @@ def setUpClass(cls):
cls.api_client,
cls.test_data["isolated_network_offering"]
)
+ cls._cleanup.append(cls.network_offering_all)
# Enable Network offering
cls.network_offering_all.update(cls.api_client, state='Enabled')
cls.native_vpc_network_offering = NetworkOffering.create(
cls.api_client,
cls.test_data["nw_offering_isolated_vpc"],
conservemode=False)
+ cls._cleanup.append(cls.native_vpc_network_offering)
cls.native_vpc_network_offering.update(cls.api_client,
state='Enabled')
- cls._cleanup = [
- cls.service_offering,
- cls.network_offering_nouserdata,
- cls.network_offering_all,
- cls.native_vpc_network_offering
- ]
-
def setUp(self):
self.apiclient = self.testClient.getApiClient()
self.hypervisor = self.testClient.getHypervisorInfo()
self.dbclient = self.testClient.getDbConnection()
+ self.cleanup = []
Review Comment:
The `self.cleanup` list initialization should occur before the conditional
block at line 146 (`if not self.hypervisorNotSupported`), but in the current
implementation, it's placed correctly. However, moving the account appending
inside the conditional means resources created within the conditional are now
added to cleanup. This is correct, but ensure that any test methods that skip
due to `hypervisorNotSupported` won't encounter issues with an empty cleanup
list. The current implementation handles this properly since the parent's
`tearDown()` checks for the existence of the `cleanup` attribute with
`hasattr()`.
##########
test/integration/smoke/test_migration.py:
##########
@@ -223,6 +211,8 @@ def test_01_native_to_native_network_migration(self):
templateid=self.template.id,
zoneid=self.zone.id
)
+ self.cleanup.append(deployVmResponse)
+
Review Comment:
[nitpick] There's an extra blank line added after appending
`deployVmResponse` to the cleanup list (line 215), but no blank line after
other similar append operations (e.g., lines 194, 255, 271). For consistency,
either remove this blank line or add blank lines after all similar operations
throughout the file.
```suggestion
```
##########
test/integration/smoke/test_migration.py:
##########
@@ -277,13 +268,16 @@ def test_02_native_to_native_vpc_migration(self):
account=self.account.name,
domainid=self.account.domainid
)
+ self.cleanup.append(vpc)
+
Review Comment:
[nitpick] Similar to the previous comment, there's an extra blank line after
appending `vpc` to the cleanup list. This inconsistency in spacing should be
addressed to maintain uniform code style throughout the file.
```suggestion
```
--
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]
