Fix KvmFileBasedStorageVmSnapshotStrategy unit test failures - #14135
Conversation
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14135 +/- ##
============================================
+ Coverage 3.71% 19.89% +16.18%
- Complexity 0 20142 +20142
============================================
Files 487 6371 +5884
Lines 41992 576829 +534837
Branches 7942 70627 +62685
============================================
+ Hits 1558 114768 +113210
- Misses 40208 449516 +409308
- Partials 226 12545 +12319
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
weizhouapache
left a comment
weizhouapache
left a comment
There was a problem hiding this comment.
lgtm
simulator CI tests have passed
|
Can we merge based on the ci build status? @weizhouapache . |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19212 |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, consistent with existing dependency-injection patterns in the class, and directly resolve the unit test setup mismatch without altering core snapshot logic.
Pull request overview
This PR addresses unit test failures in the KVM file-based storage VM snapshot strategy by updating the unit test setup to reflect current production dependencies and by aligning production code visibility to support test injection.
Changes:
- Update
KvmFileBasedStorageVmSnapshotStrategyTestto mock and injectInternalBackupServiceinto the strategy under test. - Adjust the unit test to use
VMSnapshotHelper.getVolumeSnapshotsAssociatedWithKvmDiskOnlyVmSnapshot(...)when selecting the root snapshot’s primary datastore for NVRAM cleanup. - Change
internalBackupServicefield visibility inKvmFileBasedStorageVmSnapshotStrategyto allow test injection.
File summaries
| File | Description |
|---|---|
| engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java | Fixes unit test wiring by mocking InternalBackupService and matching updated strategy behavior for root snapshot datastore selection. |
| engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java | Adjusts internalBackupService visibility to support injection in tests while preserving runtime behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
yes @Pearl1594 merging as simulator CI and packaging job results have verified the fix |
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 24.81% |
| Branch coverage | 19.01% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
Description
This PR fixes build failure on main
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?