On 07/25/2012 04:04 AM, Yu Mingfei wrote:
> This patch adds test for virsh domname.
>
> Signed-off-by: Yu Mingfei <[email protected]>
> ---
> client/tests/libvirt/tests/virsh_domname.py | 83
> +++++++++++++++++++++++++++
> 1 files changed, 83 insertions(+), 0 deletions(-)
> create mode 100644 client/tests/libvirt/tests/virsh_domname.py
>
> diff --git a/client/tests/libvirt/tests/virsh_domname.py
> b/client/tests/libvirt/tests/virsh_domname.py
> new file mode 100644
> index 0000000..b2ef07f
> --- /dev/null
> +++ b/client/tests/libvirt/tests/virsh_domname.py
> @@ -0,0 +1,83 @@
> +import logging, time
> +from autotest.client.shared import utils, error
> +from autotest.client.virt import libvirt_vm
> +
> +
> +def run_virsh_domname(test, params, env):
> + """
> + Test command: virsh domname <id/uuid>.
> +
> + There are 26 testcases, list by its type:
> + 1) test valid options.(2)
> + 2) test invalid options.(6)
> + 3) test valid options with vm paused.(2)
> + 4) test invalid options with vm paused.(6)
> + 5) test valid options with vm shut_off.(2)
> + 6) test invalid options with vm shut_off.(6)
> + 7) test valid options with libvirtd stop.(2)
> + """
Instead of listing configured test cases here (which can quickly get out
of sync w/ config). We'd prefer a step-by-step summary of WHAT the test
module does. For example:
1) Put VM in initial state
2) Put libvirt service in initial state
3) Exercise virsh domname
4) Restore VM & libvirt state
> + vm_name = params.get("main_vm", "vm1")
> + vm = env.get_vm(params["main_vm"])
Suggestion for enhancement: Support testing on one or more active VMs
> + if not vm.is_alive():
> + vm.start()
> + time.sleep(5)
> +
Instead of arbitrary sleeps (which may/may not match reality), better to
rely on virt_env_process.py preprocess_vm() (which runs automatically
before the test module starts) for most of the above code. For example,
setting the Cartesian parameter 'start_vm = yes' will have the same
effect.
> + pre_vm_state = params.get("pre_vm_state", "running")
> + if pre_vm_state == "shut off":
> + vm.destroy()
> + time.sleep(2)
> + elif pre_vm_state == "paused":
> + vm.pause()
> + time.sleep(2)
I really like your idea of "pre_vm_state" functionality. It seems we
have a number of tests that depend on this and libvirt service state as
well. Instead of duplicating this code in every test module that needs
it, how about adding a more generic implementation into preprocess_vm()?
Off the top of my head, what about using the "start_vm" parameter where:
"yes" means the VM should be started (see also "restart_vm")
"no" means the VM should be shutdown
"paused" means the VM should be started but paused
Similar for the libvirt service, maybe add a preprocess_libvirt()
function to virt_env_process.py and a "libvirt_service" parameter? This
could replace the '#Prepare libvirtd status' code below (and in several
other tests as well).
> +
> + domid = vm.get_id().strip()
> + domuuid = vm.get_uuid().strip()
> +
> + #Prepare libvirtd status
> + libvirtd = params.get("libvirtd", "on")
> + if libvirtd == "off":
> + libvirt_vm.service_libvirtd_control("stop")
> +
> + #run test case
> + options_ref = params.get("options_ref", "id")
> + addition_status_error = params.get("addition_status_error", "no")
> + status_error = params.get("status_error", "no")
> + if options_ref == "id":
> + options_ref = domid
> + elif options_ref == "hex_id" and domid != "-":
> + options_ref = hex(int(domid))
> + elif options_ref == "uuid":
> + options_ref = domuuid
> + addition_status_error = "no"
> + elif options_ref == "invalid_uuid":
> + options_ref = params.get("invalid_uuid")
> + elif options_ref == "invalid_id":
> + options_ref = params.get("invalid_id")
> + elif options_ref == "invalid_param":
> + options_ref = "%s xyz" % domid
> + elif options_ref == "name":
> + options_ref = vm_name
This looks good to me for initial addition.
Suggestion for enhancement: Make the options format string directly
into a Cartesian parameter and eliminate all the logic above. This
allows for enhancements to test by changing configuration/variants.
Otherwise this big conditional will need to be updated every time new
testing requirements are added. For example:
variants:
...
- invalid_domid:
# %s will be replaced by a VM name
# %x will be replaced by 'domname_id' in hex format
# ...etc
domname_options_prefix = "foobar-%s-foobar"
domname_options = ""
domname_options_suffix = ""
> +
> + result = libvirt_vm.virsh_domname(options_ref, ignore_status=True,
> print_info=True)
> +
> + #Recover libvirtd service to start
> + if libvirtd == "off":
> + libvirt_vm.service_libvirtd_control("start")
> + addition_status_error = "yes"
> +
> + if pre_vm_state == "paused":
> + vm.resume()
> + time.sleep(2)
> +
> + if vm.is_alive():
> + vm.destroy()
> + time.sleep(2)
> +
IIRC, this also seems to be functionality shared by more than one
libvirt test. Perhaps this could also be re-factored into
postprocess_vm() and postprocess_libvirt() in virt_env_process.py ?
Even if not, my main contention here and with 'if not vm.is_alive()'
above is the arbitrary sleeps. So, if you're not comfortable enhancing
virt_env_process.py yet, maybe you could find a way to avoid the sleeps
(4.e.g. using virt_utils.wait_for() maybe?)
> + #check status_error
> + status_error = (status_error == "no") and (addition_status_error ==
> "no")
Why do we need two parameters for status_error and addition_status_error
if they are just logically and'ed together in test?
If it's necessary and can't be simplified into a single status_error
parameter, then some comments here would be helpful.
> + if status_error:
> + if result.exit_status != 0 or result.stdout.strip() != vm_name:
> + raise error.TestFail("Run failed because unexpected result.")
> + else:
> + if result.exit_status == 0 and result.stdout.strip() != vm_name:
> + raise error.TestFail("Run passed but result is unexpected.")
> --
> 1.7.1
>
>
> --
> Best Regards
> Yu Mingfei
>
Overall, the "flow" of this test module looks good. I really like it's
simplicity and style. Let me know if you'd like me to help on any of
the above suggestions. Thanks!
--
Chris Evich, RHCA, RHCE, RHCDS, RHCSS
Quality Assurance Engineer
e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
