On 02/14/2012 08:16 AM, guyanhua wrote: > This patch adds three test cases for "virsh version" command. > > Use three cases:(1) virsh version > (2) virsh version with a unexpected option > (3) virsh version with libvirtd service stop
Hi Gu, thanks for the test submission, some comments below: > Signed-off-by: Gu Yanhua <[email protected]> > --- > client/tests/libvirt/tests/virsh_version.py | 47 > +++++++++++++++++++++++++++ > 1 files changed, 47 insertions(+), 0 deletions(-) > create mode 100644 client/tests/libvirt/tests/virsh_version.py > > diff --git a/client/tests/libvirt/tests/virsh_version.py > b/client/tests/libvirt/tests/virsh_version.py > new file mode 100644 > index 0000000..e5bd252 > --- /dev/null > +++ b/client/tests/libvirt/tests/virsh_version.py > @@ -0,0 +1,47 @@ > +import re, os, logging, commands > +from autotest_lib.client.common_lib import utils, error > +from autotest_lib.client.virt import virt_vm, virt_utils, > virt_env_process, libvirt_vm > + > +def run_virsh_version(test, params, env): > + """ > + Test command: virsh version. > + """ > + > + def virsh_version(option): > + command = "virsh version %s" % option > + status, output = commands.getstatusoutput(command) > + logging.info("Output: %s", output) > + logging.info("Status: %d", status) > + return status ^ Rather than the commands API, I'd like to see here a call to utils.run, since it already is supposed to throw an exception if the command returns exit code != 0, so that you don't have to explicitly check for status and throw TestFail exceptions. > + > + case_num = params.get("case_num") > + > + libvirtd = "on" > + if params.has_key("libvirtd") == True: > + libvirtd = params.get("libvirtd") > + > + if case_num == "1": > + option = params.get("options") > + status = virsh_version(option) > + if status != 0: > + raise error.TestFail("Can not get the virsh version") > + > + elif case_num == "2": > + option = params.get("options") > + status = virsh_version(option) > + if status == 0: > + raise error.TestFail("Use virsh version command successfully with an > invalid option") > + > + elif case_num == "3": > + if libvirtd == "on": > + raise error.TestError("Libvirtd service still started!") ^ Service libvirtd still active sounds like a better error message, or better, a context message. Please verify how to use error.context in client/common_lib/error.py and client/virt/tests/boot.py. > + elif libvirtd == "off": > + libvirt_vm.libvirtd_stop() > + option = params.get("options") > + status = virsh_version(option) > + libvirt_vm.libvirtd_start() > + if status == 0: > + raise error.TestFail("Use virsh version command successfully with > libvirtd service stop") > + ^ It seems to me that all 3 blocks can be handled with a single logic block if check_libvirtd: if libvirtd_on: ... option = params.get("options") status = virs_version(option) if check_libvirtd: if libvirtd_on: So please look into combining the case logic into a single set of instructions. Thanks for the contribution, looking forward to see an updated version of the test, Lucas _______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
