-
Notifications
You must be signed in to change notification settings - Fork 170
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
Snapshot: add new auto case for snapshot-list #5835
Conversation
|
@nanli1 Can you help to review this PR? Thanks. |
bkxml = vmxml.copy() | ||
virsh_dargs = {"debug": True, "ignore_status": False} | ||
test_obj = snapshot_base.SnapshotTest(vm, test, params) | ||
libvirt_version.is_libvirt_feature_supported(params) |
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.
Version checking could me more forward ,maybe L68
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.
I think moving to L76 is more better after variant definition. So I've updated it to there.
virsh.snapshot_create_as(vm_name, "%s --disk-only" % snap_names[0], **virsh_dargs) | ||
test.log.info("TEST_SETUP: Prepare running guest and create a disk only snapshot.") | ||
if not vm.is_alive(): | ||
vm.start() |
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.
We could login vm to avoid some unpredictable issues
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.
Good point. I think it's a useful checkpoint. Finish to update now.
cmd = "virsh snapshot-list %s %s" % (vm_name, list_options) | ||
list_result = process.run(cmd, shell=True, ignore_status=False).stdout_text.split() |
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.
For virsh commands, I think it's better to reuse the functions in virsh module. Can you plz try virsh.command or virsh.snapshot_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.
Actually I've tried to use virsh.snapshot_list before. But it's hard to deal with the result to match the expected list. Maybe you can have more better solution to deal with the cmd result?
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.
Ok, can you plz tell me what the problem is? and can't 'virsh.command' be used?
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.
Thanks to your detailed introduction. I changed it from process.run to virsh.command and the cases can pass.
if list_dict.get(key) != value: | ||
test.fail("Get incorrect result %s, the expected result is %s" % | ||
(list_result, expected_dict)) | ||
test.log.debug("Get expected result for snapshot-list options %s" % |
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.log.debug("Get expected result for snapshot-list options %s" % | |
test.log.debug("Get expected result for snapshot-list options %s", |
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.
Updated.
if snap_name not in snap_list: | ||
test.fail("Snapshot %s doesn't exist" % snap_name) | ||
else: | ||
test.log.info("Snapshot %s is in expected snapshot list." % snap_name) |
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.
'%' should be ','.
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.
Updated.
Automate case VIRT-16603 - [Disk snapshot][External] Get guest snapshot list with option Signed-off-by: Meina Li <[email protected]>
Automate case VIRT-16603 - [Disk snapshot][External] Get guest snapshot list with option