-
Notifications
You must be signed in to change notification settings - Fork 1.3k
refactoring a few smoke tests #11748
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.
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
base: 4.22
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.22 #11748 +/- ##
============================================
+ Coverage 3.60% 17.60% +13.99%
- Complexity 0 15615 +15615
============================================
Files 445 5911 +5466
Lines 37628 530126 +492498
Branches 6947 64781 +57834
============================================
+ Hits 1356 93304 +91948
- Misses 36106 426319 +390213
- Partials 166 10503 +10337
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15229 |
| Snapshot.delete(snapshot, self.userapiclient, self.zone.id) | ||
| self.helper.verify_snapshot_copies(self.userapiclient, self.snapshot_id, [self.additional_zone.id]) | ||
| self.cleanup.append(snapshot) | ||
| self._cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn’t the snapshot to be removed from the cleanup list here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slavkap per test items should go in the cleanup list not in the _cleanup list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(snapshot) | |
| self.cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaanHoogland, the cleanup is left on purpose for the class because there is a required time.sleep() and I don't want to delay the tests execution with invoking it in each test case
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Outdated
Show resolved
Hide resolved
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Outdated
Show resolved
Hide resolved
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Outdated
Show resolved
Hide resolved
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Outdated
Show resolved
Hide resolved
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py
Outdated
Show resolved
Hide resolved
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors smoke tests by adding new test scenarios, improving cleanup management, and modifying storage pool configuration. The changes focus on improving test structure and adding coverage for virtual machine deployment scenarios with snapshots and volumes across multiple zones.
- Refactored storage pool tag handling to use existing tags instead of hardcoded values
- Added proper cleanup management by using
_cleanuplists for resources - Added new test methods for VM deployment scenarios from snapshots and volumes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py | Refactored storage pool tag configuration and added VM cleanup management |
| test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py | Added new test methods, improved cleanup handling, and added utility methods for VM deployment |
| test/integration/plugins/storpool/sp_util.py | Added new disk offering configuration and template definition with debug logging |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
test/integration/smoke/test_vm_lifecycle_with_snapshot_or_volume.py
Outdated
Show resolved
Hide resolved
| "displaytext": "Disk offering with tags", | ||
| "disksize":8, | ||
| "tags": "test-vm" | ||
| "tags": cls.zone_wide_storage.tags |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same potential AttributeError as above - cls.zone_wide_storage.tags may not exist or could be None.
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Show resolved
Hide resolved
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Show resolved
Hide resolved
vishesh92
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reviewed the PR. Same comments as Daan.
|
@slavkap Since this is for the 4.22.1 release, could you retarget the PR to the 4.22 branch? |
c90c4b8 to
147c0b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| self._cleanup.append(virtual_machine) | ||
| try: | ||
| ssh_client = virtual_machine.get_ssh_client() |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper adds the created VM to self._cleanup (class-level), which defers deletion until tearDownClass. Given this class already uses self.cleanup in tearDown() for test-scoped resources, consider using self.cleanup here too so each test cleans up its own VM and doesn't leave extra VMs around for subsequent tests in the same class.
|
|
||
| @skipTestIf("testsNotSupported") | ||
| @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false") | ||
| def test_07_take_snapshot_multi_zone_deply_vm_additional_zone(self): |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_07_take_snapshot_multi_zone_deply_vm_additional_zone has a typo in the test name ("deply" -> "deploy"). This makes the test name harder to search/understand and is inconsistent with the other test names in this file.
| def test_07_take_snapshot_multi_zone_deply_vm_additional_zone(self): | |
| def test_07_take_snapshot_multi_zone_deploy_vm_additional_zone(self): |
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Show resolved
Hide resolved
| logging.debug("new list %s" % new_list) | ||
| logging.debug("zone IDs %s" % zone_ids) | ||
| logging.debug("snapshot entries %s" % snapshot_entries) | ||
| if len(new_list) != len(zone_ids): | ||
| cls.fail("Undesired list snapshot size for multiple zones") | ||
| raise Exception("Undesired list snapshot size for multiple zones") |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StorPoolHelper.verify_snapshot_copies now raises exceptions for some failure cases, but it still calls cls.fail(...) earlier in the method when Snapshot.list doesn't return a list. Since StorPoolHelper is not a unittest.TestCase, cls.fail is not defined and that path will raise AttributeError instead of a clear test failure. Replace that remaining cls.fail usage with an exception/AssertionError for consistency and correct error reporting.
| ) | ||
| self._cleanup.append(virtual_machine) | ||
| try: | ||
| ssh_client = virtual_machine.get_ssh_client() |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper appends the created VM to self._cleanup (class-level), so it won't be deleted until tearDownClass. Since this test class already uses self.cleanup + tearDown() for per-test cleanup, consider adding the VM to self.cleanup instead to avoid accumulating running VMs across tests (unless you intentionally need the VM to persist across multiple test cases).
test/integration/plugins/storpool/test_snapshot_copy_on_primary_storage.py
Outdated
Show resolved
Hide resolved
| ) | ||
| self._cleanup.append(virtual_machine) | ||
| try: | ||
| ssh_client = virtual_machine.get_ssh_client() |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable ssh_client is not used.
| self.cleanup.append(self.virtual_machine) | ||
| try: | ||
| ssh_client = virtual_machine.get_ssh_client() | ||
| ssh_client = self.virtual_machine.get_ssh_client() |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable ssh_client is not used.
| self.cleanup.append(self.virtual_machine) | ||
| try: | ||
| ssh_client = virtual_machine.get_ssh_client() | ||
| ssh_client = self.virtual_machine.get_ssh_client() |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable ssh_client is not used.
| """ | ||
|
|
||
| snapshot = Snapshot.create(self.userapiclient,volume_id=self.volume.id, zoneids=[str(self.additional_zone.id)], usestoragereplication=True) | ||
| self._cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(snapshot) | |
| self.cleanup.append(snapshot) |
| """ | ||
|
|
||
| snapshot = Snapshot.create(self.userapiclient, volume_id=self.volume.id, zoneids=[str(self.additional_zone.id)], usestoragereplication=True) | ||
| self._cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(snapshot) | |
| self.cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaanHoogland, the cleanup is left on purpose for the class because there is a required time.sleep() and I don't want to delay the tests execution with invoking it in each test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the creation does need to happen in the test?
seems strangely asymmetrical and I would have to think if my mind can handle this… ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, if it is really needed please leave a don’t-do-this-at-home warning with it?
| """ | ||
|
|
||
| snapshot = Snapshot.create(self.userapiclient, volume_id=self.volume.id) | ||
| self._cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(snapshot) | |
| self.cleanup.append(snapshot) |
| Snapshot.delete(snapshot, self.userapiclient, self.zone.id) | ||
| self.helper.verify_snapshot_copies(self.userapiclient, self.snapshot_id, [self.additional_zone.id]) | ||
| self.cleanup.append(snapshot) | ||
| self._cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(snapshot) | |
| self.cleanup.append(snapshot) |
| service | ||
| ) | ||
| self.cleanup.append(self.disk_offering) | ||
| self._cleanup.append(self.disk_offering) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(self.disk_offering) | |
| self.cleanup.append(self.disk_offering) |
| """Test to take volume snapshot in multiple StorPool primary storages in diff zones and create a volume in one of the additional zones | ||
| """ | ||
| snapshot = Snapshot.create(self.userapiclient, volume_id=self.volume.id, zoneids=[str(self.additional_zone.id)], usestoragereplication=True) | ||
| self._cleanup.append(snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(snapshot) | |
| self.cleanup.append(snapshot) |
| volumeid=volume.id, | ||
| mode="basic", | ||
| ) | ||
| self._cleanup.append(virtual_machine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(virtual_machine) | |
| self.cleanup.append(virtual_machine) |
| snapshotid=snapshot.id, | ||
| mode="basic", | ||
| ) | ||
| self._cleanup.append(virtual_machine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._cleanup.append(virtual_machine) | |
| self.cleanup.append(virtual_machine) |
Description
Added few more tests scenarios and refactoring
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
ran the smoke tests