Copilot commented on code in PR #13136:
URL: https://github.com/apache/cloudstack/pull/13136#discussion_r3305063361
##########
test/integration/smoke/test_deploy_vm_iso.py:
##########
@@ -149,9 +142,34 @@ def test_deploy_vm_from_iso(self):
diskofferingid=self.disk_offering.id,
hypervisor=self.hypervisor
)
+ self.cleanup.append(self.virtual_machine)
+
+ self.debug("VM created with ID: %s" % self.virtual_machine.id)
+
+ list_vm_response = VirtualMachine.list(
+ self.apiclient,
+ id=self.virtual_machine.id
+ )
- response = self.virtual_machine.getState(
+ self.assertEqual(
+ isinstance(list_vm_response, list),
+ True,
+ "Check list response returns a valid list"
+ )
+ vm_response = list_vm_response[0]
+ vm_state = self.virtual_machine.getState(
+ self.apiclient,
+ VirtualMachine.RUNNING
+ )
+
+ response = self.virtual_machine.getState(
self.apiclient,
VirtualMachine.RUNNING)
self.assertEqual(response[0], PASS, response[1])
+
Review Comment:
Line 165 is mis-indented (`response = ...` starts one space earlier than the
surrounding block), which will raise an `IndentationError` and prevent this
test module from importing/running. Also `getState()` is called twice (once
assigned to `vm_state` but never used). Please fix the indentation and keep a
single `getState()` call, asserting on its result.
##########
test/integration/smoke/test_deploy_vm_iso.py:
##########
@@ -129,16 +110,28 @@ def test_deploy_vm_from_iso(self):
domainid=self.account.domainid,
zoneid=self.zone.id
)
+ self.cleanup.append(self.iso)
+
+ self.debug("ISO created with ID: %s" % self.iso.id)
+ list_iso_response = Iso.list(
+ self.apiclient,
+ id=self.iso.id
+ )
+ while not isinstance(list_iso_response, list):
+ list_iso_response = Iso.list(
+ self.apiclient,
+ id=self.iso.id
+ )
Review Comment:
The `while not isinstance(list_iso_response, list): ...` loop can spin
indefinitely (and at 100% CPU) if `Iso.list()` keeps returning `None` (common
when the resource isn't visible yet) and there is no sleep/timeout. Since
`list_iso_response` isn't used afterwards, consider removing this polling;
otherwise, poll `isready`/availability with a bounded timeout and
`time.sleep()` between attempts.
##########
test/integration/smoke/test_deploy_vm_iso.py:
##########
@@ -149,9 +142,34 @@ def test_deploy_vm_from_iso(self):
diskofferingid=self.disk_offering.id,
hypervisor=self.hypervisor
)
+ self.cleanup.append(self.virtual_machine)
+
+ self.debug("VM created with ID: %s" % self.virtual_machine.id)
+
+ list_vm_response = VirtualMachine.list(
+ self.apiclient,
+ id=self.virtual_machine.id
+ )
- response = self.virtual_machine.getState(
+ self.assertEqual(
+ isinstance(list_vm_response, list),
+ True,
+ "Check list response returns a valid list"
+ )
Review Comment:
After asserting `list_vm_response` is a list, the code immediately indexes
`[0]` without verifying it’s non-empty. `VirtualMachine.list(..., id=...)` can
return an empty list transiently; this would raise `IndexError` and make the
test flaky. Add an assertion that `len(list_vm_response) != 0` (or retry with
timeout) before accessing the first element.
##########
test/integration/smoke/test_deploy_vm_iso.py:
##########
@@ -92,35 +84,24 @@ def setUp(self):
self.cleanup = [self.account]
return
- def tearDown(self):
- try:
- self.debug("Cleaning up the resources")
- cleanup_resources(self.apiclient, self.cleanup)
- self.debug("Cleanup complete!")
- except Exception as e:
- self.debug("Warning! Exception in tearDown: %s" % e)
-
@attr(
tags=[
"advanced",
"eip",
"advancedns",
"basic",
- "sg"],
- required_hardware="true")
+ "sg"
+ ],
+ required_hardware="true"
+ )
def test_deploy_vm_from_iso(self):
"""Test Deploy Virtual Machine from ISO
"""
# Validate the following:
- # 1. deploy VM using ISO
- # 2. listVM command should return the deployed VM. State of this VM
- # should be "Running".
- self.hypervisor = self.testClient.getHypervisorInfo()
- if self.hypervisor.lower() in ['lxc']:
- self.skipTest(
- "vm deploy from ISO feature is not supported on %s" %
- self.hypervisor.lower())
+ # 1. Create an ISO
+ # 2. Deploy a VM from the ISO
+ # 3. VM should be in 'Running' state
self.iso = Iso.create(
Review Comment:
The previous LXC hypervisor guard was removed. Other integration tests still
skip ISO-based VM deployment on LXC (e.g. `test_stopped_vm.py`), and attempting
this on LXC is expected to fail. Please restore the `if self.hypervisor.lower()
in ['lxc']:` + `self.skipTest(...)` check here to keep the test suite portable
across hypervisors.
--
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]
