Skip to content

Fix smoke tests due to change in behavior of restore VM #10583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Pearl1594 merged 4 commits into from
Apr 26, 2025
Merged

Fix smoke tests due to change in behavior of restore VM #10583

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b6edf3
Fix unit tests due to change in behavior of restore VM
Pearl1594 Mar 18, 2025
49d1d39
update numbering in comments
Pearl1594 Mar 26, 2025
04c901d
revert delete operations
Pearl1594 Apr 8, 2025
5997659
fix placement of start vm after refactoring
Pearl1594 Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 1 addition & 14 deletions test/integration/smoke/test_events_resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@
import os
import tempfile
import time
import unittest
import urllib.error
import urllib.parse
import urllib.request

from datetime import datetime

Expand Down Expand Up @@ -161,21 +157,12 @@ def test_01_events_resource(self):
virtual_machine.restore(self.apiclient)
time.sleep(self.services["sleep"])
virtual_machine.detach_volume(self.apiclient, volume)
volume.delete(self.apiclient)
self.cleanup.remove(volume)
ts = str(time.time())
virtual_machine.update(self.apiclient, displayname=ts)
virtual_machine.delete(self.apiclient)
self.cleanup.remove(virtual_machine)
account_network.update(self.apiclient, name=account_network.name + ts)
account_network.delete(self.apiclient)
self.cleanup.remove(account_network)
virtual_machine.start(self.apiclient)
Copy link
Contributor

@shwstppr shwstppr Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pearl1594 I'm not completely aware of the new behaviour but idea of the test was to do a bunch of operations related to VM, network, volume etc and then check if the events for them have resourceid an resourcetype associated for them. Can we update the test in a way that similar check can be done? With the current change it is checking only for VM action event.

Copy link
Contributor

@DaanHoogland DaanHoogland Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point and the point of such tests, but we are not checking for specific events anyway, just listing them. If the test is error prone, not specific and sensitive to environmental issues, I'd rather we simplify it and add more specific checks. Would you agree?

Copy link
Contributor Author

@Pearl1594 Pearl1594 Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shwstppr I removed the delete resource operations because it did not matter in terms of the events being generated. As the test doesn't check the type of event being generated for a resource, but rather just checking if events for a resource are generated. And the cleanup is happening via the teardown process.
Regarding the start operation, it was noticed that if a VM is restored - it loses its root disk, hence the test was failing on vmware during the deletion of the vm.

Copy link
Contributor

@shwstppr shwstppr Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pearl1594 Your change is correct in itself but I feel now we are verifying lesser set of actions. I would prefer if we can have only the change that fixes restore VM.
Like if earlier we were verifying 20 events, now we might only be doing 5 events.

Copy link
Contributor Author

@Pearl1594 Pearl1594 Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted the change, to keep only the relevant change to what this PR is attempting to fix. Thanks @shwstppr

account.update(self.apiclient, newname=account.name + ts)
account.disable(self.apiclient)
account.delete(self.apiclient)
self.cleanup.remove(account)
domain1.delete(self.apiclient)
self.cleanup.remove(domain1)

cmd = listEvents.listEventsCmd()
cmd.startdate = start_time
Expand Down
5 changes: 5 additions & 0 deletions test/integration/smoke/test_network_permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,11 @@ def test_04_deploy_vm_for_other_user_and_test_vm_operations(self):
self.exec_command("self.user_apiclient", command, expected=False)
self.exec_command("self.otheruser_apiclient", command, expected=True)

#22. Start VM before destroying, to recreate ROOT volume that was deleted as part of restore operation
command = """self.virtual_machine.start({apiclient})"""
self.exec_command("self.user_apiclient", command, expected=False)
self.exec_command("self.otheruser_apiclient", command, expected=True)

# 22. Destroy vm2, should succeed by vm owner
command = """self.virtual_machine.delete({apiclient}, expunge=False)"""
self.exec_command("self.user_apiclient", command, expected=False)
Expand Down
Loading