Robot Tests: Add basic snapshot testing by m-ildefons · Pull Request #2747 · harvester/tests

m-ildefons · 2026-07-03T11:24:24Z

Which issue(s) this PR fixes:

Issue #

What this PR does / why we need it:

Add basic tests for creating/deleting snapshots to the Robot framework based test suite.
This increases the coverage of the Robot framework based test suite.

Special notes for your reviewer:

Additional documentation or context

Add basic tests for creating/deleting snapshots to the Robot framework based test suite. Signed-off-by: Moritz Röhrich <[email protected]>

khushboo-rancher · 2026-07-08T19:12:23Z

This looks good to me.

cc: @albinsun (on automation duty)

Copilot

Pull request overview

Adds initial VM snapshot coverage to the Harvester Robot Framework suite by introducing a new regression test and a new snapshot “component + keywords” implementation backed by a Harvester CRD.

Changes:

Add a VM regression test that creates, waits for readiness, deletes, and waits for deletion of a VM snapshot.
Introduce a new snapshot library (component + CRD implementation + REST placeholder) and Robot keyword wrappers/resources to drive snapshot operations.
Extend constants with the virtualmachinebackups plural used by the snapshot CRD implementation.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
harvester_robot_tests/tests/regression/vm/test_vm_snapshots.robot	New Robot regression test for basic VM snapshot create/delete flow.
harvester_robot_tests/libs/snapshot/snapshot.py	Snapshot component delegating to CRD/REST implementation.
harvester_robot_tests/libs/snapshot/rest.py	REST strategy placeholder for snapshot operations.
harvester_robot_tests/libs/snapshot/crd.py	CRD-backed implementation for creating/deleting snapshots and waiting for states.
harvester_robot_tests/libs/snapshot/base.py	Abstract base interface for snapshot implementations.
harvester_robot_tests/libs/snapshot/init.py	Package export for `Snapshot`.
harvester_robot_tests/libs/keywords/snapshot_keywords.py	Robot keyword wrapper delegating snapshot operations to the Snapshot component.
harvester_robot_tests/libs/constant.py	Adds `VIRTUALMACHINEBACKUP_PLURAL` constant.
harvester_robot_tests/keywords/snapshot.resource	Robot resource keywords for snapshot operations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+            finally:
+                if time.time() > endtime:
+                    raise AssertionError(
+                        f"Snapshot {namespace}/{snapshot_name} not ready within {timeout}s"
+                    )
+                time.sleep(self.retry_interval)


- Apply test operation strategy from environment variable - Raise proper exceptions for non-yet-implemented REST strategy - Remove unnecessary VM lookup - Propagate error messages - Fix paths and imports - Remove superfluous List Snapshots keyword Signed-off-by: Moritz Röhrich <[email protected]>

albinsun

Hi, it looks for me, thank you.

Please also check those High severity comments by Copilot, the finally logic seems need to take care.

Move timeout check outside of the exception handling. This is ok, because the timeout checking was in a `finally` block, which would be executed only after returning from either the `try` or the `except` blocks, but on some return paths, we can omit the check. Namely, when there was an error and we abort anyways and when the wait was successful and the wait condition has been met already. All other paths out of the `try` and `except` blocks just continue the iteration of the for-loop, therefore don't need a `finally` block to execute. This is where we want to do the timeout check, so the timeout check is moved to just after the try/except blocks into the loop. Signed-off-by: Moritz Röhrich <[email protected]>

m-ildefons · 2026-07-15T12:27:17Z

I moved the contents of the finally block just to the bare loops. This lets the checks run when we don't exit the functions yet, but avoids running them when we exit anyways (due to exception or due to conditions met).
I believe this is a better solution that Copilot's state variable, as it achieves the same thing with less code.
Please take another look.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

+Suite Setup       Set up test environment
+Test Teardown    Cleanup test resources
+Suite Teardown   Common Suite Teardown
+
+
+*** Test Cases ***
+Test Basic VM Snapshot
+    [Tags]    coretest    p0    snapshot
+    [Documentation]    Test basic VM snapshot creation
+
+    # Generate unique name for vm, image and snapshot
+    ${suffix}=    Generate Unique Name
+    ${image_name}=    Set Variable    image-0-${suffix}
+    ${vm_name}=    Set Variable    vm-0-${suffix}
+    ${snapshot_name}=    Set Variable    snap-0-${suffix}
+
+    Given Image is available for VM creation    ${image_name}    ${OPENSUSE_IMAGE_URL}
+    When VM is created                 ${vm_name}        ${image_name}
+    Then VM should be running          ${vm_name}
+    When Snapshot is created           ${vm_name}        ${snapshot_name}
+    Then Snapshot should be ready      ${snapshot_name}
+    When Snapshot is deleted           ${snapshot_name}
+    Then Snapshot should be deleted    ${snapshot_name}


+            except ApiException as err:
+                if err.status == 404:
+                    return True
+                logging(f"error checing snapshot {namespace}/{snapshot_name}: {err}")
+


+        except ApiException as err:
+            logging(f"failed to create snapshot {namespace}/{snapshot_name}: {err}")
+            raise
+        self._wait_for_ready(namespace, snapshot_name)


+        except ApiException as err:
+            logging(f"failed to delete snapshot {namespace}/{snapshot_name}: {err}")
+            raise
+        self._wait_for_deleted(namespace, snapshot_name)


albinsun

lgtm

mergify · 2026-07-16T02:29:56Z

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

Queue this pull request

m-ildefons requested a review from khushboo-rancher July 3, 2026 11:24

m-ildefons self-assigned this Jul 3, 2026

m-ildefons force-pushed the wip/snapshots-robot branch 3 times, most recently from 2b42700 to 397ce5e Compare July 3, 2026 11:56

Robot Tests: Add basic snapshot testing

2e3e9aa

Add basic tests for creating/deleting snapshots to the Robot framework based test suite. Signed-off-by: Moritz Röhrich <[email protected]>

m-ildefons force-pushed the wip/snapshots-robot branch from 397ce5e to 2e3e9aa Compare July 3, 2026 11:57

khushboo-rancher requested a review from albinsun July 8, 2026 19:12

m-ildefons marked this pull request as ready for review July 8, 2026 20:37

albinsun requested a review from Copilot July 9, 2026 07:41

Copilot started reviewing on behalf of albinsun July 9, 2026 07:41 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

albinsun requested changes Jul 9, 2026

View reviewed changes

Comment thread harvester_robot_tests/keywords/snapshot.resource Outdated

Comment thread harvester_robot_tests/libs/snapshot/crd.py Outdated

Comment thread harvester_robot_tests/libs/snapshot/snapshot.py

m-ildefons requested a review from albinsun July 9, 2026 11:00

khushboo-rancher previously approved these changes Jul 9, 2026

View reviewed changes

albinsun reviewed Jul 15, 2026

View reviewed changes

Comment thread harvester_robot_tests/libs/snapshot/crd.py

m-ildefons dismissed khushboo-rancher’s stale review via 04160dc July 15, 2026 12:23

m-ildefons requested review from albinsun, Copilot and khushboo-rancher and removed request for Copilot July 15, 2026 12:23

m-ildefons requested a review from Copilot July 15, 2026 12:27

Copilot started reviewing on behalf of m-ildefons July 15, 2026 12:27 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

albinsun approved these changes Jul 16, 2026

View reviewed changes

khushboo-rancher approved these changes Jul 16, 2026

View reviewed changes

khushboo-rancher merged commit a780692 into harvester:main Jul 16, 2026
5 checks passed

Uh oh!

Conversation

m-ildefons commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Uh oh!

khushboo-rancher commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

albinsun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m-ildefons commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

albinsun left a comment

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Jul 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

m-ildefons commented Jul 3, 2026 •

edited

Loading

khushboo-rancher commented Jul 8, 2026 •

edited

Loading

albinsun left a comment •

edited

Loading