Re: [PATCH 2/2 v2] KVM Test: Fix qmp_basic test failure in qmp-kvm-0.12.*

[Lucas Meneghel Rodrigues <lmr@redhat.com>]

On Mon, 2011-01-10 at 18:48 +0800, qzhou@redhat.com wrote:
> From: Qingtang Zhou <qzhou@redhat.com>
> 
> QMP in qemu-kvm-0.12.* has some difference from QMP in qemu-kvm-0.13.*.
> These difference cause 'qmp_basic' test fail while running on older qemu-kvm.
> 
> This patch will fix these failures, make 'qmp_basic' runs happily.

After talking to Luiz and Michael, we think it is a better idea instead
of handling all testing in qmp_basic, we should rather write a special
test for RHEL 6's qmp. Please write an alternate test qmp_simple_rhel6
or any better name you can find, and not change qmp_basic, OK?

As for the 1st patch on the series, we think it is unnecessary to add a
new property to the VM class only for this use case. I recreated the
patch moving the verification code only to qmp_basic and will apply it.

Thanks!

> Changelog from v1:
>  - fix errors in 'test_version' function, make sure qmp verrsion can be got
>    currectly in qemu-kvm-0.13.*
> 
> Signed-off-by: Qingtang Zhou <qzhou@redhat.com>
> ---
>  client/tests/kvm/tests/qmp_basic.py |  114 ++++++++++++++++++++++------------
>  1 files changed, 74 insertions(+), 40 deletions(-)
> 
> diff --git a/client/tests/kvm/tests/qmp_basic.py b/client/tests/kvm/tests/qmp_basic.py
> index 985ad15..11091da 100644
> --- a/client/tests/kvm/tests/qmp_basic.py
> +++ b/client/tests/kvm/tests/qmp_basic.py
> @@ -1,4 +1,4 @@
> -import kvm_test_utils
> +import kvm_test_utils, logging
>  from autotest_lib.client.common_lib import error
>  
>  def run_qmp_basic(test, params, env):
> @@ -29,6 +29,8 @@ def run_qmp_basic(test, params, env):
>          o Are all those check_*() functions really needed? Wouldn't a
>            specialized class (eg. a Response class) do better?
>      """
> +    qmp_version = []
> +
>      def fail_no_key(qmp_dict, key):
>          if not isinstance(qmp_dict, dict):
>              raise error.TestFail("qmp_dict is not a dict (it's '%s')" %
> @@ -49,21 +51,24 @@ def run_qmp_basic(test, params, env):
>          If any of these checks fails, error.TestFail is raised.
>          """
>          fail_no_key(qmp_dict, key)
> -        if not isinstance(qmp_dict[key], keytype):
> -            raise error.TestFail("'%s' key is not of type '%s', it's '%s'" %
> -                                 (key, keytype, type(qmp_dict[key])))
> +        if isinstance(qmp_dict[key], keytype):
> +            return True
> +
> +        logging.error("'%s' key is not of type '%s', it's '%s'",
> +                      key, keytype, type(qmp_dict[key]))
> +        return False
>  
> 
>      def check_key_is_dict(qmp_dict, key):
> -        check_dict_key(qmp_dict, key, dict)
> +        return check_dict_key(qmp_dict, key, dict)
>  
> 
>      def check_key_is_list(qmp_dict, key):
> -        check_dict_key(qmp_dict, key, list)
> +        return check_dict_key(qmp_dict, key, list)
>  
> 
>      def check_key_is_str(qmp_dict, key):
> -        check_dict_key(qmp_dict, key, unicode)
> +        return check_dict_key(qmp_dict, key, unicode)
>  
> 
>      def check_str_key(qmp_dict, keyname, value=None):
> @@ -76,11 +81,10 @@ def run_qmp_basic(test, params, env):
>      def check_key_is_int(qmp_dict, key):
>          fail_no_key(qmp_dict, key)
>          try:
> -            value = int(qmp_dict[key])
> +            int(qmp_dict[key])
>          except:
> -            raise error.TestFail("'%s' key is not of type int, it's '%s'" %
> -                                 (key, type(qmp_dict[key])))
> -
> +            return False
> +        return True
>  
>      def check_bool_key(qmp_dict, keyname, value=None):
>          check_dict_key(qmp_dict, keyname, bool)
> @@ -110,6 +114,7 @@ def run_qmp_basic(test, params, env):
>          @param classname: Expected error class name
>          @param datadict: Expected error data dictionary
>          """
> +        logging.debug("resp %s", str(resp))
>          check_key_is_dict(resp, "error")
>          check_key_is_str(resp["error"], "class")
>          if classname and resp["error"]["class"] != classname:
> @@ -129,9 +134,25 @@ def run_qmp_basic(test, params, env):
>          { "qemu": { "major": json-int, "minor": json-int, "micro": json-int }
>            "package": json-string }
>          """
> -        check_key_is_dict(version, "qemu")
> -        for key in [ "major", "minor", "micro" ]:
> -            check_key_is_int(version["qemu"], key)
> +        success = check_key_is_dict(version, "qemu")
> +        if success:
> +            for key in [ "major", "minor", "micro" ]:
> +                success = check_key_is_int(version["qemu"], key)
> +                if not success:
> +		    raise error.TestFail("'%s' key is not of type int, "
> +                                         "it's '%s'" %
> +					 (key, type(version["qemu"][key])))
> +
> +	        qmp_version.append(int(version["qemu"][key]))
> +
> +        else:
> +            success = check_key_is_str(version, "qemu")
> +            if not success:
> +                raise error.TestFail("'qemu' key is neither 'dict' nor 'str'")
> +            qmp_version.extend(map(int, version["qemu"].split('.')))
> +
> +        logging.debug("got qemu version %s", str(qmp_version))
> +
>          check_key_is_str(version, "package")
>  
> 
> @@ -224,8 +245,13 @@ def run_qmp_basic(test, params, env):
>          names must be detected.
>          """
>          resp = monitor.cmd_obj({ "execute": "eject", "foobar": True })
> -        check_error_resp(resp, "QMPExtraInputObjectMember",
> -                         { "member": "foobar" })
> +        if qmp_version[1] > 12:
> +            expected_error = "QMPExtraInputObjectMember"
> +            data_dict = {"member": "foobar"}
> +        else:
> +            expected_error = "MissingParameter"
> +            data_dict = {"name": "device"}
> +        check_error_resp(resp, expected_error, data_dict)
>  
> 
>      def test_bad_arguments_key_type(monitor):
> @@ -318,18 +344,37 @@ def run_qmp_basic(test, params, env):
>          command used doesn't matter much as QMP performs argument checking
>          _before_ calling the command.
>          """
> -        # stop doesn't take arguments
> -        resp = monitor.cmd_qmp("stop", { "foo": 1 })
> -        check_error_resp(resp, "InvalidParameter", { "name": "foo" })
> -
> -        # required argument omitted
> -        resp = monitor.cmd_qmp("screendump")
> -        check_error_resp(resp, "MissingParameter", { "name": "filename" })
> +        # qmp 0.12.* is different from 0.13.*:
> +        # 1. 'stop' command just return {} evenif stop have arguments.
> +        # 2. there is no 'screendump' command.
> +        # 3. argument isn't checked in 'device' command.
> +        # so skip these tests in qmp 0.12.*.
> +        if qmp_version[1] > 12:
> +            # stop doesn't take arguments
> +            resp = monitor.cmd_qmp("stop", { "foo": 1 })
> +            check_error_resp(resp, "InvalidParameter", { "name": "foo" })
> +
> +            # required argument omitted
> +            resp = monitor.cmd_qmp("screendump")
> +            check_error_resp(resp, "MissingParameter", { "name": "filename" })
> +
> +            # 'bar' is not a valid argument
> +            resp = monitor.cmd_qmp("screendump", { "filename": "outfile",
> +                                                   "bar": "bar" })
> +            check_error_resp(resp, "InvalidParameter", { "name": "bar"})
> +
> +            # filename argument must be a json-string
> +            for arg in [ {}, [], 1, True ]:
> +                resp = monitor.cmd_qmp("screendump", { "filename": arg })
> +                check_error_resp(resp, "InvalidParameterType",
> +                                 { "name": "filename", "expected": "string" })
> +
> +            # force argument must be a json-bool
> +            for arg in [ {}, [], 1, "foo" ]:
> +                resp = monitor.cmd_qmp("eject", { "force": arg, "device": "foo" })
> +                check_error_resp(resp, "InvalidParameterType",
> +                                 { "name": "force", "expected": "bool" })
>  
> -        # 'bar' is not a valid argument
> -        resp = monitor.cmd_qmp("screendump", { "filename": "outfile",
> -                                               "bar": "bar" })
> -        check_error_resp(resp, "InvalidParameter", { "name": "bar"})
>  
>          # test optional argument: 'force' is omitted, but it's optional, so
>          # the handler has to be called. Test this happens by checking an
> @@ -337,18 +382,6 @@ def run_qmp_basic(test, params, env):
>          resp = monitor.cmd_qmp("eject", { "device": "foobar" })
>          check_error_resp(resp, "DeviceNotFound")
>  
> -        # filename argument must be a json-string
> -        for arg in [ {}, [], 1, True ]:
> -            resp = monitor.cmd_qmp("screendump", { "filename": arg })
> -            check_error_resp(resp, "InvalidParameterType",
> -                             { "name": "filename", "expected": "string" })
> -
> -        # force argument must be a json-bool
> -        for arg in [ {}, [], 1, "foo" ]:
> -            resp = monitor.cmd_qmp("eject", { "force": arg, "device": "foo" })
> -            check_error_resp(resp, "InvalidParameterType",
> -                             { "name": "force", "expected": "bool" })
> -
>          # val argument must be a json-int
>          for arg in [ {}, [], True, "foo" ]:
>              resp = monitor.cmd_qmp("memsave", { "val": arg, "filename": "foo",
> @@ -377,8 +410,9 @@ def run_qmp_basic(test, params, env):
>          """
>          # We also call a HMP-only command, to be sure it will fail as expected
>          for cmd in [ "bar", "query-", "query-foo", "q", "help" ]:
> +            data_dict = { "name": cmd }
>              resp = monitor.cmd_qmp(cmd)
> -            check_error_resp(resp, "CommandNotFound", { "name": cmd })
> +            check_error_resp(resp, "CommandNotFound", data_dict)
>  
> 
>      vm = kvm_test_utils.get_living_vm(env, params.get("main_vm"))