[6.17 cp]add test libvirt image based provisioning

[amolpati30]

Adding new Libvirt image-based provisioning tests.
This test validates the complete workflow of provisioning a host on Libvirt compute resource using image-based provisioning method through the Satellite UI. It verifies that the host is successfully created, deployed.

Fixes - #20534

Parent pr - #19225

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The request.addfinalizer closure references host_fqdn, which is only defined later in the test; if the test fails before that assignment, the finalizer will raise a NameError—consider computing host_fqdn earlier or guarding the cleanup with a conditional.
• The finalizer deletes the Libvirt compute resource created by the module_cr_libvirt fixture; since this fixture is module-scoped and may be reused by other tests, it would be safer to move compute-resource cleanup into the fixture’s own teardown or avoid deleting it here.
• The hard-coded memory value 6442450944 in module_libvirt_compute_profile is a non-obvious magic number; consider expressing it as a calculation (e.g., 6 * 1024**3) or a named constant for readability.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `request.addfinalizer` closure references `host_fqdn`, which is only defined later in the test; if the test fails before that assignment, the finalizer will raise a `NameError`—consider computing `host_fqdn` earlier or guarding the cleanup with a conditional.
- The finalizer deletes the Libvirt compute resource created by the `module_cr_libvirt` fixture; since this fixture is module-scoped and may be reused by other tests, it would be safer to move compute-resource cleanup into the fixture’s own teardown or avoid deleting it here.
- The hard-coded memory value `6442450944` in `module_libvirt_compute_profile` is a non-obvious magic number; consider expressing it as a calculation (e.g., `6 * 1024**3`) or a named constant for readability.

## Individual Comments

### Comment 1
<location> `tests/foreman/ui/test_computeresource_libvirt.py:276-285` </location>
<code_context>
+        uuid=settings.libvirt.libvirt_image_path,
+    ).create()
+
+    @request.addfinalizer
+    def _finalize():
+        # Cleanup: Delete the image and compute resource after test execution.
+        sat.provisioning_cleanup(host_fqdn)
+        cr = sat.api.LibvirtComputeResource().search(
+            query={'search': f'name={module_cr_libvirt.id}'}
+        )
+        sat.api.Image(id=image.id, compute_resource=module_cr_libvirt.id).delete()
+        if cr:
+            sat.api.LibvirtComputeResource(id=cr[0].id).delete()
+
+    # Begin UI session to create and manage the host
</code_context>

<issue_to_address>
**issue (bug_risk):** Finalizer depends on `host_fqdn` being assigned, which can cause teardown errors if the test fails before that point.

Because `_finalize` closes over `host_fqdn` before it is assigned, any failure before `host_fqdn = f'{hostname}.{module_libvirt_provisioning_sat.domain.name}'` executes will cause an `UnboundLocalError` during teardown, obscuring the real test failure. Please either compute `host_fqdn` before defining the finalizer, or make the finalizer robust to `host_fqdn` not being set (e.g., by initializing a mutable container beforehand and only calling `provisioning_cleanup` when it has a value).
</issue_to_address>

### Comment 2
<location> `tests/foreman/ui/test_computeresource_libvirt.py:280-284` </location>
<code_context>
+    def _finalize():
+        # Cleanup: Delete the image and compute resource after test execution.
+        sat.provisioning_cleanup(host_fqdn)
+        cr = sat.api.LibvirtComputeResource().search(
+            query={'search': f'name={module_cr_libvirt.id}'}
+        )
+        sat.api.Image(id=image.id, compute_resource=module_cr_libvirt.id).delete()
+        if cr:
+            sat.api.LibvirtComputeResource(id=cr[0].id).delete()
+
</code_context>

<issue_to_address>
**suggestion (testing):** Per-test finalizer deletes a module-scoped compute resource fixture, which can interfere with other tests using the same fixture.

This test’s finalizer deletes the Libvirt compute resource provided by `module_cr_libvirt` (a module-scoped fixture). That means one test can tear down a resource that other tests in the module still need, leading to order-dependent or flaky failures.

Please move the compute resource cleanup into `module_cr_libvirt` itself (e.g., convert it to a `yield` fixture and delete the resource there), or otherwise ensure per-test cleanup does not remove shared module-scoped resources so fixture lifetimes stay consistent with their scope.

Suggested implementation:

```python
    @request.addfinalizer
    def _finalize():
        # Cleanup: Delete the image and host after test execution.
        sat.provisioning_cleanup(host_fqdn)

        # Delete the test-scoped image created for this test run.
        sat.api.Image(id=image.id, compute_resource=module_cr_libvirt.id).delete()

        # Delete the host and ensure it is gone.
        session.host.delete(name)
        assert not sat.api.Host().search(query={'search': f'name="{name}"'})

```

To fully implement the review comment and align resource lifetimes with fixture scopes, you should also:

1. Locate the `module_cr_libvirt` fixture (likely in this file or a nearby `conftest.py`).
2. Convert it into a `yield` fixture if it is not already, for example:

   ```python
   @pytest.fixture(scope="module")
   def module_cr_libvirt():
       cr = sat.api.LibvirtComputeResource(...).create()
       try:
           yield cr
       finally:
           # Module-scoped cleanup: delete the compute resource here
           sat.api.LibvirtComputeResource(id=cr.id).delete()
   ```

3. Remove any other per-test code paths that delete `LibvirtComputeResource` instances created by `module_cr_libvirt`, to avoid one test tearing down resources needed by others in the module.

These changes ensure that only module-level teardown deletes the module-scoped Libvirt compute resource, while per-test finalizers remain limited to test-scoped entities like hosts and images.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Per-test finalizer deletes a module-scoped compute resource fixture, which can interfere with other tests using the same fixture.

This test’s finalizer deletes the Libvirt compute resource provided by module_cr_libvirt (a module-scoped fixture). That means one test can tear down a resource that other tests in the module still need, leading to order-dependent or flaky failures.

Please move the compute resource cleanup into module_cr_libvirt itself (e.g., convert it to a yield fixture and delete the resource there), or otherwise ensure per-test cleanup does not remove shared module-scoped resources so fixture lifetimes stay consistent with their scope.

Suggested implementation:

    @request.addfinalizer
    def _finalize():
        # Cleanup: Delete the image and host after test execution.
        sat.provisioning_cleanup(host_fqdn)

        # Delete the test-scoped image created for this test run.
        sat.api.Image(id=image.id, compute_resource=module_cr_libvirt.id).delete()

        # Delete the host and ensure it is gone.
        session.host.delete(name)
        assert not sat.api.Host().search(query={'search': f'name="{name}"'})

To fully implement the review comment and align resource lifetimes with fixture scopes, you should also:

1. Locate the module_cr_libvirt fixture (likely in this file or a nearby conftest.py).

2. Convert it into a yield fixture if it is not already, for example:

   @pytest.fixture(scope="module")
   def module_cr_libvirt():
       cr = sat.api.LibvirtComputeResource(...).create()
       try:
           yield cr
       finally:
           # Module-scoped cleanup: delete the compute resource here
           sat.api.LibvirtComputeResource(id=cr.id).delete()

3. Remove any other per-test code paths that delete LibvirtComputeResource instances created by module_cr_libvirt, to avoid one test tearing down resources needed by others in the module.

These changes ensure that only module-level teardown deletes the module-scoped Libvirt compute resource, while per-test finalizers remain limited to test-scoped entities like hosts and images.

[amolpati30]

trigger: test-robottelo
pytest: tests/foreman/ui/test_computeresource_libvirt.py -k test_positive_image_provision_end_to_end
provisioning: true

[Satellite-QE]

PRT Result

Build Number: 13907
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_computeresource_libvirt.py -k test_positive_image_provision_end_to_end --external-logging --include-onprem-provisioning
Test Result : ====== 2 failed, 4 deselected, 33 warnings, 1 error in 1712.97s (0:28:32) ======