Skip to content

Conversation

@meinaLi
Copy link
Contributor

@meinaLi meinaLi commented Oct 22, 2025

Automate case:
VIRT-300203 - Define&&Start vm with //disk/blockio[@discard_granularity] boundary values
Description: For discard_grabularity=0, from libvirt-11.7.0, it exists in vm dumpxml.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected test scenario naming for clearer test reporting and execution.
    • Scoped invalid discard_granularity error messages to specific test variants for more accurate feedback.
    • Added version-aware validation so behavior adapts to libvirt 11.7.0+ when discard_granularity is '0'.
    • Improved debug logging to surface the retrieved discard_granularity value during validation.

@coderabbitai
Copy link

coderabbitai bot commented Oct 22, 2025

Walkthrough

Test config moved variant-specific error messages from a boundary-level to per-variant. Test code adds debug logging, fixes two scenario name typos, and makes boundary scenario handling version-aware for discard_granularity == '0'.

Changes

Cohort / File(s) Summary
Configuration restructuring
libvirt/tests/cfg/virtual_disks/virtual_disks_discard_granularity.cfg
Moved define_error_msg and expected_attribute out of the top-level boundary_vm_start and into the specific variants (value_minus, value_max), making invalid-discard messaging variant-scoped.
Test logic & flow
libvirt/tests/src/virtual_disks/virtual_disks_discard_granularity.py
Added debug logging in check_vm_dumpxml when actual discard_granularity equals expected; corrected scenario names (failure_vm_tartfailure_vm_start, boundary_vm_tartboundary_vm_start); made boundary_vm_start set expected_attribute for discard_granularity '0' based on libvirt version (≥11.7.0 → true, else false).

Sequence Diagram(s)

sequenceDiagram
  participant Runner as Test Runner
  participant Scenario as boundary_vm_start
  participant LibvirtVer as libvirt_version
  participant Checker as check_vm_dumpxml
  Runner->>Scenario: invoke boundary_vm_start(params)
  Scenario->>LibvirtVer: version_compare(11,7,0)?
  alt discard_granularity == '0' and version >=11.7.0
    Scenario-->>Scenario: expected_attribute = True
  else discard_granularity == '0' and version <11.7.0
    Scenario-->>Scenario: expected_attribute = False
  else other values
    Scenario-->>Scenario: expected_attribute = params.get(...) or default
  end
  Scenario->>Checker: check_vm_dumpxml(expected_attribute)
  Checker->>Checker: retrieve actual discard_granularity
  alt actual == expected
    Checker-->>Runner: debug log "discard_granularity matches: <value>"
  else mismatch
    Checker-->>Runner: raise/report mismatch
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • chunfuwen

Poem

🐰 Hopping through tests at night,
I tuck loose errors out of sight,
I log a match, I fix a name,
A tiny hop — the tests reclaim! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Virtual disk: add new test scenario of discard_granularity" is directly related to the main changes in the changeset. According to the PR objectives, the PR automates test case VIRT-300203 and implements tests covering boundary values for discard_granularity, with special handling for version-specific behavior. The changes to both the configuration file and test file support this objective by reorganizing error handling and introducing new logic for boundary value testing. The title is concise, specific enough for teammates to understand the primary change without being overly detailed, and accurately reflects the core intent of the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a3e8d9 and 18fe5cd.

📒 Files selected for processing (2)
  • libvirt/tests/cfg/virtual_disks/virtual_disks_discard_granularity.cfg (1 hunks)
  • libvirt/tests/src/virtual_disks/virtual_disks_discard_granularity.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libvirt/tests/cfg/virtual_disks/virtual_disks_discard_granularity.cfg
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.11
  • GitHub Check: Python 3.8
🔇 Additional comments (3)
libvirt/tests/src/virtual_disks/virtual_disks_discard_granularity.py (3)

166-167: LGTM: Improved test observability.

Adding debug logging when the discard_granularity check passes helps with test debugging and confirms successful validation.


259-267: Excellent implementation of version-aware boundary testing.

This change correctly implements two improvements:

  1. Fixes the typo from "boundary_vm_tart" to "boundary_vm_start"
  2. Adds version-dependent logic for discard_granularity == '0', where libvirt 11.7.0+ preserves the attribute in dumpxml while earlier versions do not

The logic properly overrides the expected_attribute parameter based on the libvirt version when testing the zero boundary case, aligning with the PR objective.


212-212: Critical typo fix.

Corrects the scenario name from "failure_vm_tart" to "failure_vm_start". This fix ensures the invalid discard_granularity test scenario can actually be matched and executed.

Verify that the test configuration file uses the corrected scenario name:


Comment @coderabbitai help to get the list of available commands and usage tips.

VIRT-300203 - Define&&Start vm with //disk/blockio[@discard_granularity]
boundary values
Description: For discard_grabularity=0, from libvirt-11.7.0, it exists in vm dumpxml.

Signed-off-by: Meina Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant