use cleanup mechs on TestNetworkMigration #11945
Conversation
boring-cyborg
bot
added
component:integration-test
Python
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the test cleanup mechanism in test_migration.py to leverage the parent class's cleanup functionality. The changes move from a custom cleanup implementation to using the standard super() pattern inherited from cloudstackTestCase.
Key changes:
- Appending resources to cleanup lists immediately after creation instead of in bulk at the end
- Delegating cleanup to parent class methods via
super()calls - Initializing
self.cleanuplist at the beginning ofsetUp()method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| zoneid=self.zone.id | ||
| ) | ||
| self.cleanup.append(deployVmResponse) | ||
|
|
There was a problem hiding this comment.
[nitpick] There's an extra blank line added after appending deployVmResponse to the cleanup list (line 215), but no blank line after other similar append operations (e.g., lines 194, 255, 271). For consistency, either remove this blank line or add blank lines after all similar operations throughout the file.
| domainid=self.account.domainid | ||
| ) | ||
| self.cleanup.append(vpc) | ||
|
|
There was a problem hiding this comment.
[nitpick] Similar to the previous comment, there's an extra blank line after appending vpc to the cleanup list. This inconsistency in spacing should be addressed to maintain uniform code style throughout the file.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11945 +/- ##
============================================
- Coverage 16.18% 16.18% -0.01%
- Complexity 13298 13299 +1
============================================
Files 5657 5657
Lines 498470 498480 +10
Branches 60493 60493
============================================
- Hits 80660 80658 -2
- Misses 408830 408842 +12
Partials 8980 8980
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:
|
|
unfortunately |
The problem seems to be not in the test but in vlan allocation leakage in the system. This can be merged but the left over from the test will remain, the scenario is valid. |
|
As this is purely (test)cleanup, I don’t think this needs further testing , agree @vishesh92 @sudo87 @Pearl1594 ? |
Yes. Just need to wait for test results to ensure that teardown gets executed successfully. |
I don’t think there is much relevance in this, rerunning. |
DaanHoogland
force-pushed
the
ghi9026-testNetworkMigrationCleanup
branch
from
a080eb8 to
74235ec
Compare
|
@blueorangutan package |
|
@DaanHoogland 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 15755 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14828)
|
4 participants
Description
This PR…
Fixes: #9026 , but shows that there is a genuine issue in the vlan allocation management in the system.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?