-
Notifications
You must be signed in to change notification settings - Fork 181
controller_functional: fix test for s390x #6671
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: master
Are you sure you want to change the base?
Conversation
'generic-ccw' was the standard device name in lszdev in the past but now it's been changed to actually list the virtio device type. However, this test might have to run on older versions still and the test expectation is only that the device is listed for the given device number. So, omit the device type restriction to check all devices. with s390-virtio, there are only virtio devices listed with lszdev (or passed through devices but they are not present here / per default). Signed-off-by: Sebastian Mitterle <[email protected]>
WalkthroughThe change modifies a device query command in a functional test file. Specifically, the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/controller/controller_functional.py (1)
442-442: LGTM – improved compatibility with newer lszdev versions.The change from
lszdev generic-ccw --columns IDtolszdev --columns IDappropriately addresses the evolution of lszdev output format. By removing the device-type restriction, the test remains functional with both older versions (reporting 'generic-ccw') and newer versions (reporting specific virtio device types).The broader query scope is safe for s390-virtio environments where lszdev inherently lists only virtio devices by default. The test correctly validates device presence by checking the device number (lines 447-451) rather than relying on the reported device type.
Optional: Consider adding a brief inline comment explaining why the device type is not specified, for future maintainers:
- cmd = 'lszdev --columns ID' + # Query all devices (no type filter) to support both old lszdev (generic-ccw) + # and new lszdev (reports specific virtio types like virtio-blk, virtio-net) + cmd = 'lszdev --columns ID'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/controller/controller_functional.py(1 hunks)
⏰ 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.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
'generic-ccw' was the standard device name in lszdev in the past but now it's been changed to actually list the virtio device type. However, this test might have to run on older versions still and the test expectation is only that the device is listed for the given device number. So, omit the device type restriction to check all devices. with s390-virtio, there are only virtio devices listed with lszdev (or passed through devices but they are not present here / per default).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.