[KVM_AUTOTEST][PATCH v1 0/3]: QMP basic test-suite

[KVM_AUTOTEST][PATCH v1 0/3]: QMP basic test-suite

[Luiz Capitulino]

This is an _initial_ work on having a full QMP test-suite in kvm-autotest,
but we start simple: the basic suite tests only the basic protocol
specification. More details in patch 3/3.

This is already very useful for QMP development and can be merged as is,
but there are two limitations that should be addressed in the near future:

    1. A number of QMP commands are executed before the QMP suite is run,
       which means that some problems can be caught by non-test code (ie.
       real usage).

       Ideally, the QMP suite should be run before any command is executed,
       so that we avoid catching QMP bugs in other test suites and/or in
       kvm-autotest non-test code.

    2. The qmp_capabilities command is run by the QMPMonitor class
       constructor. This makes it impossible for me to test it in the QMP
       suite.

       We have to find a way to create a new monitor object from the
       test suite.

Thanks.

[PATCH 1/3] QMPMonitor: Introduce the get_greeting() method

[Luiz Capitulino]

It returns the QMP's greeting message as sent by the monitor.

Please, note that this commit also changes the QMPMonitor's constructor
to store the full greeting message (it currently stores only its
contents).

This new method is going to be used by the QMP test-suite, which fully
checks the greeting message.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 client/tests/kvm/kvm_monitor.py |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
index 6bff07f..d77af31 100644
--- a/client/tests/kvm/kvm_monitor.py
+++ b/client/tests/kvm/kvm_monitor.py
@@ -408,7 +408,7 @@ class QMPMonitor(Monitor):
             while time.time() < end_time:
                 for obj in self._read_objects():
                     if "QMP" in obj:
-                        self._greeting = obj["QMP"]
+                        self._greeting = obj
                         break
                 if self._greeting:
                     break
@@ -597,6 +597,13 @@ class QMPMonitor(Monitor):
         self._lock.release()
 
 
+    def get_greeting(self):
+        """
+        Return QMP greeting message.
+        """
+        return self._greeting
+
+
     # Command wrappers
     # Note: all of the following functions raise exceptions in a similar manner
     # to cmd() and _get_command_output().
-- 
1.7.3.1.120.g38a18

[PATCH 2/3] QMPMonitor: Introduce the send() method and wrappers

[Luiz Capitulino]

This method directly sends data to the QMP monitor and returns its
response, without any kind of special treatment or sanity checking.

Two simple wrappers are also introduced: cmd_obj() and cmd_qmp(),
they provide some level of automation on building QMP commands.

All three methods are going to be used by the QMP test-suite.

NOTE: The methods send() and _get_command_output() are similar, but
      I'm out of ideas on how to properly refactor them.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 client/tests/kvm/kvm_monitor.py |   60 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
index d77af31..63201b4 100644
--- a/client/tests/kvm/kvm_monitor.py
+++ b/client/tests/kvm/kvm_monitor.py
@@ -604,6 +604,66 @@ class QMPMonitor(Monitor):
         return self._greeting
 
 
+    def send(self, data, timeout=20):
+        """
+        Send data to the QMP monitor and return its response.
+
+        @param data: Data to send
+        @param timeout: Time duration to wait for response
+        @return: QMP success or error response as a dictionary
+        @raise MonitorLockError: Raised if the lock cannot be acquired
+        @raise MonitorSendError: Raised if the command cannot be sent
+        @raise MonitorProtocolError: Raised if no response is received
+        """
+        # XXX: This method is similar to _get_command_output(), we should
+        # refactor it
+        if not self._acquire_lock(20):
+            raise MonitorLockError("Could not acquire exclusive lock to send "
+                                   "QMP command '%s'" % cmd)
+        try:
+            self._read_objects()
+            self._socket.sendall(data)
+            end_time = time.time() + timeout
+            while time.time() < end_time:
+                for obj in self._read_objects():
+                    if isinstance(obj, dict):
+                        if "return" in obj or "error" in obj:
+                            return obj
+                time.sleep(0.1)
+            else:
+                 raise MonitorProtocolError("Received no response (data: %s)"
+                                            % str(data))
+        except socket.error:
+            raise MonitorSendError("Could not send data '%s'" % str(data))
+        finally:
+            self._lock.release()
+
+
+    def cmd_obj(self, cmd_obj, timeout=20):
+        """
+        Take a Python object, transforms it in JSON and send the resulting
+        string to the QMP monitor. Return the monitor's response.
+
+        @param cmd_obj: Python object to transform in JSON
+        @param timeout: Time duration to wait for response
+        @return: QMP success or error response as a dictionary
+        @note: raise same exceptions as send()
+        """
+        return self.send(json.dumps(cmd_obj) + "\n", timeout)
+
+    def cmd_qmp(self, command, arguments=None, id=None, timeout=20):
+        """
+        Build a QMP command from the passed arguments, return the monitor's
+        response.
+
+        @param command: QMP command name
+        @param arguments: Arguments in the form of a Python dictionary
+        @param id: QMP command id
+        @return: QMP success or error response as a dictionary
+        @note: raise same exceptions as send_cmd()
+        """
+        return self.cmd_obj(self._build_cmd(command, arguments, id), timeout)
+
     # Command wrappers
     # Note: all of the following functions raise exceptions in a similar manner
     # to cmd() and _get_command_output().
-- 
1.7.3.1.120.g38a18

Re: [PATCH 2/3] QMPMonitor: Introduce the send() method and wrappers

[Lucas Meneghel Rodrigues]

On Fri, 2010-10-15 at 17:52 -0300, Luiz Capitulino wrote:
> This method directly sends data to the QMP monitor and returns its
> response, without any kind of special treatment or sanity checking.
> 
> Two simple wrappers are also introduced: cmd_obj() and cmd_qmp(),
> they provide some level of automation on building QMP commands.
> 
> All three methods are going to be used by the QMP test-suite.
> 
> NOTE: The methods send() and _get_command_output() are similar, but
>       I'm out of ideas on how to properly refactor them.

^ Rather we should probably refactor all the other commands in terms of
send() rather than _get_command_output(). This can be done on a later
patch, don't worry.

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  client/tests/kvm/kvm_monitor.py |   60 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
> index d77af31..63201b4 100644
> --- a/client/tests/kvm/kvm_monitor.py
> +++ b/client/tests/kvm/kvm_monitor.py
> @@ -604,6 +604,66 @@ class QMPMonitor(Monitor):
>          return self._greeting
>  
> 
> +    def send(self, data, timeout=20):
> +        """
> +        Send data to the QMP monitor and return its response.
> +
> +        @param data: Data to send
> +        @param timeout: Time duration to wait for response
> +        @return: QMP success or error response as a dictionary
> +        @raise MonitorLockError: Raised if the lock cannot be acquired
> +        @raise MonitorSendError: Raised if the command cannot be sent
> +        @raise MonitorProtocolError: Raised if no response is received
> +        """
> +        # XXX: This method is similar to _get_command_output(), we should
> +        # refactor it
> +        if not self._acquire_lock(20):
> +            raise MonitorLockError("Could not acquire exclusive lock to send "
> +                                   "QMP command '%s'" % cmd)
> +        try:
> +            self._read_objects()
> +            self._socket.sendall(data)
> +            end_time = time.time() + timeout
> +            while time.time() < end_time:
> +                for obj in self._read_objects():
> +                    if isinstance(obj, dict):
> +                        if "return" in obj or "error" in obj:
> +                            return obj
> +                time.sleep(0.1)
> +            else:
> +                 raise MonitorProtocolError("Received no response (data: %s)"
> +                                            % str(data))

^ Here we have an indentation with an extra space. As a suggestion to
avoid such problems, you can allways use utils/reindent.py (this path is
relative to the autotest source tree directory

utils/reindent.py client/tests/kvm/kvm_monitor.py

Also, in order to catch up little mistakes, I also suggest the pylint
wrapper that we have:

utils/run_pylint.py client/tests/kvm/kvm_monitor.py

Anyway, my pre-commit scripts run it, just pointed out because it's nice
to know those helper tools.

> +        except socket.error:
> +            raise MonitorSendError("Could not send data '%s'" % str(data))
> +        finally:
> +            self._lock.release()
> +
> +
> +    def cmd_obj(self, cmd_obj, timeout=20):
> +        """
> +        Take a Python object, transforms it in JSON and send the resulting
> +        string to the QMP monitor. Return the monitor's response.
> +
> +        @param cmd_obj: Python object to transform in JSON
> +        @param timeout: Time duration to wait for response
> +        @return: QMP success or error response as a dictionary
> +        @note: raise same exceptions as send()
> +        """
> +        return self.send(json.dumps(cmd_obj) + "\n", timeout)

^ Wouldn't be better to name this method obj_to_qmp ?

> +    def cmd_qmp(self, command, arguments=None, id=None, timeout=20):
> +        """
> +        Build a QMP command from the passed arguments, return the monitor's
> +        response.
> +
> +        @param command: QMP command name
> +        @param arguments: Arguments in the form of a Python dictionary
> +        @param id: QMP command id
> +        @return: QMP success or error response as a dictionary
> +        @note: raise same exceptions as send_cmd()
> +        """
> +        return self.cmd_obj(self._build_cmd(command, arguments, id), timeout)
> +
>      # Command wrappers
>      # Note: all of the following functions raise exceptions in a similar manner
>      # to cmd() and _get_command_output().

Re: [PATCH 2/3] QMPMonitor: Introduce the send() method and wrappers

[Luiz Capitulino]

On Mon, 18 Oct 2010 09:42:44 -0200
Lucas Meneghel Rodrigues <lmr@redhat.com> wrote:

> On Fri, 2010-10-15 at 17:52 -0300, Luiz Capitulino wrote:
> > This method directly sends data to the QMP monitor and returns its
> > response, without any kind of special treatment or sanity checking.
> > 
> > Two simple wrappers are also introduced: cmd_obj() and cmd_qmp(),
> > they provide some level of automation on building QMP commands.
> > 
> > All three methods are going to be used by the QMP test-suite.
> > 
> > NOTE: The methods send() and _get_command_output() are similar, but
> >       I'm out of ideas on how to properly refactor them.
> 
> ^ Rather we should probably refactor all the other commands in terms of
> send() rather than _get_command_output(). This can be done on a later
> patch, don't worry.
> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  client/tests/kvm/kvm_monitor.py |   60 +++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 60 insertions(+), 0 deletions(-)
> > 
> > diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
> > index d77af31..63201b4 100644
> > --- a/client/tests/kvm/kvm_monitor.py
> > +++ b/client/tests/kvm/kvm_monitor.py
> > @@ -604,6 +604,66 @@ class QMPMonitor(Monitor):
> >          return self._greeting
> >  
> > 
> > +    def send(self, data, timeout=20):
> > +        """
> > +        Send data to the QMP monitor and return its response.
> > +
> > +        @param data: Data to send
> > +        @param timeout: Time duration to wait for response
> > +        @return: QMP success or error response as a dictionary
> > +        @raise MonitorLockError: Raised if the lock cannot be acquired
> > +        @raise MonitorSendError: Raised if the command cannot be sent
> > +        @raise MonitorProtocolError: Raised if no response is received
> > +        """
> > +        # XXX: This method is similar to _get_command_output(), we should
> > +        # refactor it
> > +        if not self._acquire_lock(20):
> > +            raise MonitorLockError("Could not acquire exclusive lock to send "
> > +                                   "QMP command '%s'" % cmd)
> > +        try:
> > +            self._read_objects()
> > +            self._socket.sendall(data)
> > +            end_time = time.time() + timeout
> > +            while time.time() < end_time:
> > +                for obj in self._read_objects():
> > +                    if isinstance(obj, dict):
> > +                        if "return" in obj or "error" in obj:
> > +                            return obj
> > +                time.sleep(0.1)
> > +            else:
> > +                 raise MonitorProtocolError("Received no response (data: %s)"
> > +                                            % str(data))
> 
> ^ Here we have an indentation with an extra space. As a suggestion to
> avoid such problems, you can allways use utils/reindent.py (this path is
> relative to the autotest source tree directory
> 
> utils/reindent.py client/tests/kvm/kvm_monitor.py
> 
> Also, in order to catch up little mistakes, I also suggest the pylint
> wrapper that we have:
> 
> utils/run_pylint.py client/tests/kvm/kvm_monitor.py
> 
> Anyway, my pre-commit scripts run it, just pointed out because it's nice
> to know those helper tools.

I can do it, as this is likely going to need a respin.

> > +        except socket.error:
> > +            raise MonitorSendError("Could not send data '%s'" % str(data))
> > +        finally:
> > +            self._lock.release()
> > +
> > +
> > +    def cmd_obj(self, cmd_obj, timeout=20):
> > +        """
> > +        Take a Python object, transforms it in JSON and send the resulting
> > +        string to the QMP monitor. Return the monitor's response.
> > +
> > +        @param cmd_obj: Python object to transform in JSON
> > +        @param timeout: Time duration to wait for response
> > +        @return: QMP success or error response as a dictionary
> > +        @note: raise same exceptions as send()
> > +        """
> > +        return self.send(json.dumps(cmd_obj) + "\n", timeout)
> 
> ^ Wouldn't be better to name this method obj_to_qmp ?

I don't think so, 'obj_to_qmp' gives me the impression that the method's
input is going to get some transformation and then returned. IOW, I can't
tell from that name that a command is going to be sent.

> > +    def cmd_qmp(self, command, arguments=None, id=None, timeout=20):
> > +        """
> > +        Build a QMP command from the passed arguments, return the monitor's
> > +        response.
> > +
> > +        @param command: QMP command name
> > +        @param arguments: Arguments in the form of a Python dictionary
> > +        @param id: QMP command id
> > +        @return: QMP success or error response as a dictionary
> > +        @note: raise same exceptions as send_cmd()
> > +        """
> > +        return self.cmd_obj(self._build_cmd(command, arguments, id), timeout)
> > +
> >      # Command wrappers
> >      # Note: all of the following functions raise exceptions in a similar manner
> >      # to cmd() and _get_command_output().
> 
> 

[PATCH 3/3] Introduce QMP basic test-suite

[Luiz Capitulino]

This commit introduces a suite which checks that QMP conforms to its
specification (which is file QMP/qmp-spec.txt in QEMU's source tree).

It's important to note that this suite does _not_ do command or
asynchronous messages testing, as each command or asynchronous message
has its own specification, and thus should be tested separately.

This suite is limited to test that the basic protocol works as specified,
that is, to ensure that the greeting message, success and error responses
contain the basic information the spec says they do. Additionally,
several errors conditions at the protocol level are also tested.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 client/tests/kvm/tests/qmp_basic.py    |  374 ++++++++++++++++++++++++++++++++
 client/tests/kvm/tests_base.cfg.sample |    3 +
 2 files changed, 377 insertions(+), 0 deletions(-)
 create mode 100644 client/tests/kvm/tests/qmp_basic.py

diff --git a/client/tests/kvm/tests/qmp_basic.py b/client/tests/kvm/tests/qmp_basic.py
new file mode 100644
index 0000000..21198ef
--- /dev/null
+++ b/client/tests/kvm/tests/qmp_basic.py
@@ -0,0 +1,374 @@
+import kvm_test_utils
+from autotest_lib.client.common_lib import error
+
+def run_qmp_basic(test, params, env):
+    """
+    QMP Specification test-suite: this checks if the *basic* protocol conforms
+    to its specification, which is file QMP/qmp-spec.txt in QEMU's source tree.
+
+    IMPORTANT NOTES:
+
+        o Most tests depend heavily on QMP's error information (eg. classes),
+          this might have bad implications as the error interface is going to
+          change in QMP
+
+        o Command testing is *not* covered in this suite. Each command has its
+          own specification and should be tested separately
+
+        o We use the same terminology as used by the QMP specification,
+          specially with regard to JSON types (eg. a Python dict is called
+          a json-object)
+
+        o This is divided in sub test-suites, please check the bottom of this
+          file to check the order in which they are run
+
+    TODO:
+
+        o Finding which test failed is not as easy as it should be
+
+        o Are all those check_*() functions really needed? Wouldn't a
+          specialized class (eg. a Response class) do better?
+    """
+    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')" % type(qmp_dict))
+        if not key in qmp_dict:
+            raise error.TestFail("'%s' key doesn't exist in dict ('%s')"
+                                 % (key, str(qmp_dict)))
+
+    def check_dict_key(qmp_dict, key, keytype):
+        """
+        Performs the following checks on a QMP dict key:
+
+        1. qmp_dict is a dict
+        2. key exists in qmp_dict
+        3. key is of type keytype
+        
+        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])))
+
+    def check_key_is_dict(qmp_dict, key):
+        check_dict_key(qmp_dict, key, dict)
+
+    def check_key_is_list(qmp_dict, key):
+        check_dict_key(qmp_dict, key, list)
+
+    def check_key_is_str(qmp_dict, key):
+        check_dict_key(qmp_dict, key, unicode)
+
+    def check_str_key(qmp_dict, keyname, value=None):
+        check_dict_key(qmp_dict, keyname, unicode)
+        if value and value != qmp_dict[keyname]:
+            raise error.TestFail("'%s' key value '%s' should be '%s'"
+                                 % (keyname, str(qmp_dict[keyname]), str(value)))
+
+    def check_key_is_int(qmp_dict, key):
+        fail_no_key(qmp_dict, key)
+        try:
+            value = int(qmp_dict[key])
+        except:
+            raise error.TestFail("'%s' key is not of type int, it's '%s'"
+                                 % (key, type(qmp_dict[key])))
+
+    def check_bool_key(qmp_dict, keyname, value=None):
+        check_dict_key(qmp_dict, keyname, bool)
+        if value and value != qmp_dict[keyname]:
+            raise error.TestFail("'%s' key value '%s' should be '%s'"
+                                 % (keyname, str(qmp_dict[keyname]), str(value)))
+
+    def check_success_resp(resp, empty=False):
+        """
+        Check QMP OK response.
+
+        @param resp: QMP response
+        @param empty: if True, response should not contain data to return
+        """
+        check_key_is_dict(resp, "return")
+        if empty and len(resp["return"]) > 0:
+            raise error.TestFail("success response is not empty ('%s')"
+                                 % str(resp))
+
+    def check_error_resp(resp, classname=None, datadict=None):
+        """
+        Check QMP error response.
+
+        @param resp: QMP response
+        @param classname: Expected error class name
+        @param datadict: Expected error data dictionary
+        """
+        check_key_is_dict(resp, "error")
+        check_key_is_str(resp["error"], "class")
+        if classname and resp["error"]["class"] != classname:
+            raise error.TestFail("got error class '%s' expected '%s'"
+                                 % (resp["error"]["class"], classname))
+        check_key_is_dict(resp["error"], "data")
+        if datadict and resp["error"]["data"] != datadict:
+            raise error.TestFail("got data dict '%s' expected '%s'"
+                                 % (resp["error"]["data"], datadict))
+
+    def test_version(version):
+        """
+        Check the QMP greeting message version key which, according to QMP's
+        documentation, should be:
+
+        { "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)
+        check_key_is_str(version, "package")
+
+    def test_greeting(greeting):
+        check_key_is_dict(greeting, "QMP")
+        check_key_is_dict(greeting["QMP"], "version")
+        check_key_is_list(greeting["QMP"], "capabilities")
+
+    def greeting_suite(monitor):
+        """
+        Check the greeting message format, as described in the QMP
+        specfication section '2.2 Server Greeting'.
+
+        { "QMP": { "version": json-object, "capabilities": json-array } }
+        """
+        greeting = monitor.get_greeting()
+        test_greeting(greeting)
+        test_version(greeting["QMP"]["version"])
+
+
+    def json_parsing_errors_suite(monitor):
+        """
+        Check that QMP's parser is able to recover from parsing errors, please
+        check the JSON spec for more info on the JSON syntax (RFC 4627).
+        """
+        # We're quite simple right now and the focus is on parsing errors that
+        # have already biten us in the past.
+        #
+        # TODO: The following test-cases are missing:
+        #
+        #   - JSON numbers, strings and arrays
+        #   - More invalid characters or malformed structures
+        #   - Valid, but not obvious syntax, like zillion of spaces or
+        #     strings with unicode chars (different suite maybe?)
+        bad_json = []
+
+        # A JSON value MUST be an object, array, number, string, true, false,
+        # or null
+        #
+        # NOTE: QMP seems to ignore a number of chars, like: | and ?
+        bad_json.append(":")
+        bad_json.append(",")
+
+        # Malformed json-objects
+        #
+        # NOTE: sending only "}" seems to break QMP
+        # NOTE: Duplicate keys are accepted (should it?)
+        bad_json.append("{ \"execute\" }")
+        bad_json.append("{ \"execute\": \"query-version\", }")
+        bad_json.append("{ 1: \"query-version\" }")
+        bad_json.append("{ true: \"query-version\" }")
+        bad_json.append("{ []: \"query-version\" }")
+        bad_json.append("{ {}: \"query-version\" }")
+
+        for cmd in bad_json:
+            resp = monitor.send(cmd)
+            check_error_resp(resp, "JSONParsing")
+
+
+    def test_id_key(monitor):
+        """
+        Check that QMP's "id" key is correctly handled.
+        """
+        # The "id" key must be echoed back in error responses
+        id = "kvm-autotest"
+        resp = monitor.cmd_qmp("eject", { "foobar": True }, id=id)
+        check_error_resp(resp)
+        check_str_key(resp, "id", id)
+
+        # The "id" key must be echoed back in success responses
+        resp = monitor.cmd_qmp("query-status", id=id)
+        check_success_resp(resp)
+        check_str_key(resp, "id", id)
+
+        # The "id" key can be any json-object
+        for id in [ True, 1234, "string again!", [1, [], {}, True, "foo"],
+                    { "key": {} } ]:
+            resp = monitor.cmd_qmp("query-status", id=id)
+            check_success_resp(resp)
+            if resp["id"] != id:
+                raise error.TestFail("expected id '%s' but got '%s'"
+                                     % (str(id), str(resp["id"])))
+
+    def test_invalid_arg_key(monitor):
+        """
+        Currently, the only supported keys in the input object are: "execute",
+        "arguments" and "id". Although expansion is supported, invalid key
+        names must be detected.
+        """
+        resp = monitor.cmd_obj({ "execute": "eject", "foobar": True })
+        check_error_resp(resp, "QMPExtraInputObjectMember",
+                         { "member": "foobar" })
+
+    def test_bad_arguments_key_type(monitor):
+        """
+        The "arguments" key must be an json-object.
+        
+        We use the eject command to perform the tests, but that's a random
+        choice, any command that accepts arguments will do, as the command
+        doesn't get called.
+        """
+        for item in [ True, [], 1, "foo" ]:
+            resp = monitor.cmd_obj({ "execute": "eject", "arguments": item })
+            check_error_resp(resp, "QMPBadInputObjectMember",
+                             { "member": "arguments", "expected": "object" })
+
+    def test_bad_execute_key_type(monitor):
+        """
+        The "execute" key must be a json-string.
+        """
+        for item in [ False, 1, {}, [] ]:
+            resp = monitor.cmd_obj({ "execute": item })
+            check_error_resp(resp, "QMPBadInputObjectMember",
+                             { "member": "execute", "expected": "string" })
+
+    def test_no_execute_key(monitor):
+        """
+        The "execute" key must exist, we also test for some stupid parsing
+        errors.
+        """
+        for cmd in [ {}, { "execut": "qmp_capabilities" },
+                     { "executee": "qmp_capabilities" }, { "foo": "bar" }]:
+            resp = monitor.cmd_obj(cmd)
+            check_error_resp(resp) # XXX: check class and data dict?
+
+    def test_bad_input_obj_type(monitor):
+        """
+        The input object must be... an json-object.
+        """
+        for cmd in [ "foo", [], True, 1 ]:
+            resp = monitor.cmd_obj(cmd)
+            check_error_resp(resp, "QMPBadInputObject", { "expected":"object" })
+
+    def test_good_input_obj(monitor):
+        """
+        Basic success tests for issuing QMP commands.
+        """
+        # NOTE: We don't use the cmd_qmp() method here because the command
+        # object is in a 'random' order
+        resp = monitor.cmd_obj({ "execute": "query-version" })
+        check_success_resp(resp)
+
+        resp = monitor.cmd_obj({ "arguments": {}, "execute": "query-version" })
+        check_success_resp(resp)
+
+        id = "1234foo"
+        resp = monitor.cmd_obj({ "id": id, "execute": "query-version",
+                                 "arguments": {} })
+        check_success_resp(resp)
+        check_str_key(resp, "id", id)
+
+        # TODO: would be good to test simple argument usage, but we don't have
+        # a read-only command that accepts arguments.
+
+    def input_object_suite(monitor):
+        """
+        Check the input object format, as described in the QMP specfication
+        section '2.3 Issuing Commands'.
+
+        { "execute": json-string, "arguments": json-object, "id": json-value }
+        """
+        test_good_input_obj(monitor)
+        test_bad_input_obj_type(monitor)
+        test_no_execute_key(monitor)
+        test_bad_execute_key_type(monitor)
+        test_bad_arguments_key_type(monitor)
+        test_id_key(monitor)
+        test_invalid_arg_key(monitor)
+
+    def argument_checker_suite(monitor):
+        """
+        Check that QMP's argument checker is detecting all possible errors.
+
+        We use a number of different commands to perform the checks, but the
+        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" })
+
+        # '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
+        # error that is generated by the handler itself.
+        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 ba a json-int
+        for arg in [ {}, [], True, "foo" ]:
+            resp = monitor.cmd_qmp("memsave", { "val": arg, "filename": "foo",
+                                                "size": 10 })
+            check_error_resp(resp, "InvalidParameterType",
+                             { "name": "val", "expected": "int" })
+
+        # value argument must ba a json-number
+        for arg in [ {}, [], True, "foo" ]:
+            resp = monitor.cmd_qmp("migrate_set_speed", { "value": arg })
+            check_error_resp(resp, "InvalidParameterType",
+                             { "name": "value", "expected": "number" })
+
+        # qdev-type commands have their own argument checker, all QMP does
+        # is to skip its checking and pass arguments through. Check this
+        # works by providing invalid options to device_add and expecting
+        # an error message from qdev
+        resp = monitor.cmd_qmp("device_add", { "driver": "e1000","foo": "bar" })
+        check_error_resp(resp, "PropertyNotFound",
+                               {"device": "e1000", "property": "foo"})
+
+
+    def unknown_commands_suite(monitor):
+        """
+        Check that QMP handles unknown commands correctly.
+        """
+        # We also call a HMP-only command, to be sure it will fail as expected
+        for cmd in [ "bar", "query-", "query-foo", "q", "help" ]:
+            resp = monitor.cmd_qmp(cmd)
+            check_error_resp(resp, "CommandNotFound", { "name": cmd })
+
+
+    vm = kvm_test_utils.get_living_vm(env, params.get("main_vm"))
+
+    # Run all suites
+    greeting_suite(vm.monitor)
+    input_object_suite(vm.monitor)
+    argument_checker_suite(vm.monitor)
+    unknown_commands_suite(vm.monitor)
+    json_parsing_errors_suite(vm.monitor)
+
+    # check if QMP is still alive
+    if not vm.monitor.is_responsive():
+        raise error.TestFail('QEMU is not alive after QMP testing')
diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
index 769d750..2f6b077 100644
--- a/client/tests/kvm/tests_base.cfg.sample
+++ b/client/tests/kvm/tests_base.cfg.sample
@@ -456,6 +456,9 @@ variants:
             - fmt_raw:
                 image_format_stg = raw
 
+    - qmp_basic: install setup unattended_install.cdrom
+        type = qmp_basic
+
     - vlan:  install setup unattended_install.cdrom
         type = vlan
         # subnet should not be used by host
-- 
1.7.3.1.120.g38a18

Re: [PATCH 3/3] Introduce QMP basic test-suite

[Lucas Meneghel Rodrigues]

On Fri, 2010-10-15 at 17:52 -0300, Luiz Capitulino wrote:
> This commit introduces a suite which checks that QMP conforms to its
> specification (which is file QMP/qmp-spec.txt in QEMU's source tree).
> 
> It's important to note that this suite does _not_ do command or
> asynchronous messages testing, as each command or asynchronous message
> has its own specification, and thus should be tested separately.
> 
> This suite is limited to test that the basic protocol works as specified,
> that is, to ensure that the greeting message, success and error responses
> contain the basic information the spec says they do. Additionally,
> several errors conditions at the protocol level are also tested.

This is a very good test quality wise, thanks for the effort writing it!

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  client/tests/kvm/tests/qmp_basic.py    |  374 ++++++++++++++++++++++++++++++++
>  client/tests/kvm/tests_base.cfg.sample |    3 +
>  2 files changed, 377 insertions(+), 0 deletions(-)
>  create mode 100644 client/tests/kvm/tests/qmp_basic.py
> 
> diff --git a/client/tests/kvm/tests/qmp_basic.py b/client/tests/kvm/tests/qmp_basic.py
> new file mode 100644
> index 0000000..21198ef
> --- /dev/null
> +++ b/client/tests/kvm/tests/qmp_basic.py
> @@ -0,0 +1,374 @@
> +import kvm_test_utils
> +from autotest_lib.client.common_lib import error
> +
> +def run_qmp_basic(test, params, env):
> +    """
> +    QMP Specification test-suite: this checks if the *basic* protocol conforms
> +    to its specification, which is file QMP/qmp-spec.txt in QEMU's source tree.
> +
> +    IMPORTANT NOTES:
> +
> +        o Most tests depend heavily on QMP's error information (eg. classes),
> +          this might have bad implications as the error interface is going to
> +          change in QMP

^ I was thinking about this, perhaps in the future, when the error
interface does change, this test can be extended to do check the
capabilities of the monitor and act differently according to the results
of the check.

> +        o Command testing is *not* covered in this suite. Each command has its
> +          own specification and should be tested separately
> +
> +        o We use the same terminology as used by the QMP specification,
> +          specially with regard to JSON types (eg. a Python dict is called
> +          a json-object)
> +
> +        o This is divided in sub test-suites, please check the bottom of this
> +          file to check the order in which they are run
> +
> +    TODO:
> +
> +        o Finding which test failed is not as easy as it should be

^ What I usually do is to keep a failure dictionary/list that contains
all tests that failed at a given time, and allways append new errors to
it when they happen. In the end of the test, when raising
error.TestError, this contextual info can be added to the exception.

> +        o Are all those check_*() functions really needed? Wouldn't a
> +          specialized class (eg. a Response class) do better?

^ It is an interesting idea, we could wrap all the checks on a Response
class. Also, it seems to me that we could have a better implementation
for checking with assertions, such as:

if not isinstance(qmp_dict, dict):
    raise error.TestFail(...

assert isinstance(qmp_dict, dict), "qmp_dict is not a dict...

And during the actual test, run the tests in a try/except block that
traps AssertionErrors and turns them into error.TestFails

> +    """
> +    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')" % type(qmp_dict))
> +        if not key in qmp_dict:
> +            raise error.TestFail("'%s' key doesn't exist in dict ('%s')"
> +                                 % (key, str(qmp_dict)))
> +
> +    def check_dict_key(qmp_dict, key, keytype):
> +        """
> +        Performs the following checks on a QMP dict key:
> +
> +        1. qmp_dict is a dict
> +        2. key exists in qmp_dict
> +        3. key is of type keytype
> +        
> +        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])))
> +
> +    def check_key_is_dict(qmp_dict, key):
> +        check_dict_key(qmp_dict, key, dict)
> +
> +    def check_key_is_list(qmp_dict, key):
> +        check_dict_key(qmp_dict, key, list)
> +
> +    def check_key_is_str(qmp_dict, key):
> +        check_dict_key(qmp_dict, key, unicode)
> +
> +    def check_str_key(qmp_dict, keyname, value=None):
> +        check_dict_key(qmp_dict, keyname, unicode)
> +        if value and value != qmp_dict[keyname]:
> +            raise error.TestFail("'%s' key value '%s' should be '%s'"
> +                                 % (keyname, str(qmp_dict[keyname]), str(value)))
> +
> +    def check_key_is_int(qmp_dict, key):
> +        fail_no_key(qmp_dict, key)
> +        try:
> +            value = int(qmp_dict[key])
> +        except:
> +            raise error.TestFail("'%s' key is not of type int, it's '%s'"
> +                                 % (key, type(qmp_dict[key])))
> +
> +    def check_bool_key(qmp_dict, keyname, value=None):
> +        check_dict_key(qmp_dict, keyname, bool)
> +        if value and value != qmp_dict[keyname]:
> +            raise error.TestFail("'%s' key value '%s' should be '%s'"
> +                                 % (keyname, str(qmp_dict[keyname]), str(value)))
> +
> +    def check_success_resp(resp, empty=False):
> +        """
> +        Check QMP OK response.
> +
> +        @param resp: QMP response
> +        @param empty: if True, response should not contain data to return
> +        """
> +        check_key_is_dict(resp, "return")
> +        if empty and len(resp["return"]) > 0:
> +            raise error.TestFail("success response is not empty ('%s')"
> +                                 % str(resp))

^ Here a little detail in this format string operation: We try to follow
PEP 8 as much as possible so the % operator should be in the same line
as the initial string, ie, this should have been written as:

raise error.TestFail("success response is not empty ('%s')" %
                     str(resp))

Please consider this for all other similar instances of this construct.

> +    def check_error_resp(resp, classname=None, datadict=None):
> +        """
> +        Check QMP error response.
> +
> +        @param resp: QMP response
> +        @param classname: Expected error class name
> +        @param datadict: Expected error data dictionary
> +        """
> +        check_key_is_dict(resp, "error")
> +        check_key_is_str(resp["error"], "class")
> +        if classname and resp["error"]["class"] != classname:
> +            raise error.TestFail("got error class '%s' expected '%s'"
> +                                 % (resp["error"]["class"], classname))
> +        check_key_is_dict(resp["error"], "data")
> +        if datadict and resp["error"]["data"] != datadict:
> +            raise error.TestFail("got data dict '%s' expected '%s'"
> +                                 % (resp["error"]["data"], datadict))
> +
> +    def test_version(version):
> +        """
> +        Check the QMP greeting message version key which, according to QMP's
> +        documentation, should be:
> +
> +        { "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)
> +        check_key_is_str(version, "package")

^ Forgot to put 2 lines between the 2 methods

> +    def test_greeting(greeting):
> +        check_key_is_dict(greeting, "QMP")
> +        check_key_is_dict(greeting["QMP"], "version")
> +        check_key_is_list(greeting["QMP"], "capabilities")

^ Same here

> +    def greeting_suite(monitor):

^ About the suites: As they all get a monitor as a parameter, it seems
better to write them as classes, that get a monitor parameter on its
constructor, then all the suite functions are methods of that class.

> +        """
> +        Check the greeting message format, as described in the QMP
> +        specfication section '2.2 Server Greeting'.
> +
> +        { "QMP": { "version": json-object, "capabilities": json-array } }
> +        """
> +        greeting = monitor.get_greeting()
> +        test_greeting(greeting)
> +        test_version(greeting["QMP"]["version"])
> +
> +
> +    def json_parsing_errors_suite(monitor):
> +        """
> +        Check that QMP's parser is able to recover from parsing errors, please
> +        check the JSON spec for more info on the JSON syntax (RFC 4627).
> +        """
> +        # We're quite simple right now and the focus is on parsing errors that
> +        # have already biten us in the past.

^ Excellent, regression suites are usually a great way to tell whether
functionalities have regressed or not :)

> +        # TODO: The following test-cases are missing:
> +        #
> +        #   - JSON numbers, strings and arrays
> +        #   - More invalid characters or malformed structures
> +        #   - Valid, but not obvious syntax, like zillion of spaces or
> +        #     strings with unicode chars (different suite maybe?)
> +        bad_json = []
> +
> +        # A JSON value MUST be an object, array, number, string, true, false,
> +        # or null
> +        #
> +        # NOTE: QMP seems to ignore a number of chars, like: | and ?
> +        bad_json.append(":")
> +        bad_json.append(",")
> +
> +        # Malformed json-objects
> +        #
> +        # NOTE: sending only "}" seems to break QMP

^ Should we open a bug about it?

> +        # NOTE: Duplicate keys are accepted (should it?)

^ It most likely shouldn't.

> +        bad_json.append("{ \"execute\" }")
> +        bad_json.append("{ \"execute\": \"query-version\", }")
> +        bad_json.append("{ 1: \"query-version\" }")
> +        bad_json.append("{ true: \"query-version\" }")
> +        bad_json.append("{ []: \"query-version\" }")
> +        bad_json.append("{ {}: \"query-version\" }")
> +
> +        for cmd in bad_json:
> +            resp = monitor.send(cmd)
> +            check_error_resp(resp, "JSONParsing")
> +
> +
> +    def test_id_key(monitor):
> +        """
> +        Check that QMP's "id" key is correctly handled.
> +        """
> +        # The "id" key must be echoed back in error responses
> +        id = "kvm-autotest"
> +        resp = monitor.cmd_qmp("eject", { "foobar": True }, id=id)
> +        check_error_resp(resp)
> +        check_str_key(resp, "id", id)
> +
> +        # The "id" key must be echoed back in success responses
> +        resp = monitor.cmd_qmp("query-status", id=id)
> +        check_success_resp(resp)
> +        check_str_key(resp, "id", id)
> +
> +        # The "id" key can be any json-object
> +        for id in [ True, 1234, "string again!", [1, [], {}, True, "foo"],
> +                    { "key": {} } ]:
> +            resp = monitor.cmd_qmp("query-status", id=id)
> +            check_success_resp(resp)
> +            if resp["id"] != id:
> +                raise error.TestFail("expected id '%s' but got '%s'"
> +                                     % (str(id), str(resp["id"])))
> +
> +    def test_invalid_arg_key(monitor):
> +        """
> +        Currently, the only supported keys in the input object are: "execute",
> +        "arguments" and "id". Although expansion is supported, invalid key
> +        names must be detected.
> +        """
> +        resp = monitor.cmd_obj({ "execute": "eject", "foobar": True })
> +        check_error_resp(resp, "QMPExtraInputObjectMember",
> +                         { "member": "foobar" })
> +
> +    def test_bad_arguments_key_type(monitor):
> +        """
> +        The "arguments" key must be an json-object.
> +        
> +        We use the eject command to perform the tests, but that's a random
> +        choice, any command that accepts arguments will do, as the command
> +        doesn't get called.
> +        """
> +        for item in [ True, [], 1, "foo" ]:
> +            resp = monitor.cmd_obj({ "execute": "eject", "arguments": item })
> +            check_error_resp(resp, "QMPBadInputObjectMember",
> +                             { "member": "arguments", "expected": "object" })
> +
> +    def test_bad_execute_key_type(monitor):
> +        """
> +        The "execute" key must be a json-string.
> +        """
> +        for item in [ False, 1, {}, [] ]:
> +            resp = monitor.cmd_obj({ "execute": item })
> +            check_error_resp(resp, "QMPBadInputObjectMember",
> +                             { "member": "execute", "expected": "string" })
> +
> +    def test_no_execute_key(monitor):
> +        """
> +        The "execute" key must exist, we also test for some stupid parsing
> +        errors.
> +        """
> +        for cmd in [ {}, { "execut": "qmp_capabilities" },
> +                     { "executee": "qmp_capabilities" }, { "foo": "bar" }]:
> +            resp = monitor.cmd_obj(cmd)
> +            check_error_resp(resp) # XXX: check class and data dict?
> +
> +    def test_bad_input_obj_type(monitor):
> +        """
> +        The input object must be... an json-object.
> +        """
> +        for cmd in [ "foo", [], True, 1 ]:
> +            resp = monitor.cmd_obj(cmd)
> +            check_error_resp(resp, "QMPBadInputObject", { "expected":"object" })
> +
> +    def test_good_input_obj(monitor):
> +        """
> +        Basic success tests for issuing QMP commands.
> +        """
> +        # NOTE: We don't use the cmd_qmp() method here because the command
> +        # object is in a 'random' order
> +        resp = monitor.cmd_obj({ "execute": "query-version" })
> +        check_success_resp(resp)
> +
> +        resp = monitor.cmd_obj({ "arguments": {}, "execute": "query-version" })
> +        check_success_resp(resp)
> +
> +        id = "1234foo"
> +        resp = monitor.cmd_obj({ "id": id, "execute": "query-version",
> +                                 "arguments": {} })
> +        check_success_resp(resp)
> +        check_str_key(resp, "id", id)
> +
> +        # TODO: would be good to test simple argument usage, but we don't have
> +        # a read-only command that accepts arguments.
> +
> +    def input_object_suite(monitor):
> +        """
> +        Check the input object format, as described in the QMP specfication
> +        section '2.3 Issuing Commands'.
> +
> +        { "execute": json-string, "arguments": json-object, "id": json-value }
> +        """
> +        test_good_input_obj(monitor)
> +        test_bad_input_obj_type(monitor)
> +        test_no_execute_key(monitor)
> +        test_bad_execute_key_type(monitor)
> +        test_bad_arguments_key_type(monitor)
> +        test_id_key(monitor)
> +        test_invalid_arg_key(monitor)
> +
> +    def argument_checker_suite(monitor):
> +        """
> +        Check that QMP's argument checker is detecting all possible errors.
> +
> +        We use a number of different commands to perform the checks, but the
> +        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" })
> +
> +        # '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
> +        # error that is generated by the handler itself.
> +        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 ba a json-int

^ Typo in the comment

> +        for arg in [ {}, [], True, "foo" ]:
> +            resp = monitor.cmd_qmp("memsave", { "val": arg, "filename": "foo",
> +                                                "size": 10 })
> +            check_error_resp(resp, "InvalidParameterType",
> +                             { "name": "val", "expected": "int" })
> +
> +        # value argument must ba a json-number

^ Typo in the comment

> +        for arg in [ {}, [], True, "foo" ]:
> +            resp = monitor.cmd_qmp("migrate_set_speed", { "value": arg })
> +            check_error_resp(resp, "InvalidParameterType",
> +                             { "name": "value", "expected": "number" })
> +
> +        # qdev-type commands have their own argument checker, all QMP does
> +        # is to skip its checking and pass arguments through. Check this
> +        # works by providing invalid options to device_add and expecting
> +        # an error message from qdev
> +        resp = monitor.cmd_qmp("device_add", { "driver": "e1000","foo": "bar" })
> +        check_error_resp(resp, "PropertyNotFound",
> +                               {"device": "e1000", "property": "foo"})
> +
> +
> +    def unknown_commands_suite(monitor):
> +        """
> +        Check that QMP handles unknown commands correctly.
> +        """
> +        # We also call a HMP-only command, to be sure it will fail as expected
> +        for cmd in [ "bar", "query-", "query-foo", "q", "help" ]:
> +            resp = monitor.cmd_qmp(cmd)
> +            check_error_resp(resp, "CommandNotFound", { "name": cmd })
> +
> +
> +    vm = kvm_test_utils.get_living_vm(env, params.get("main_vm"))
> +
> +    # Run all suites
> +    greeting_suite(vm.monitor)
> +    input_object_suite(vm.monitor)
> +    argument_checker_suite(vm.monitor)
> +    unknown_commands_suite(vm.monitor)
> +    json_parsing_errors_suite(vm.monitor)
> +
> +    # check if QMP is still alive
> +    if not vm.monitor.is_responsive():
> +        raise error.TestFail('QEMU is not alive after QMP testing')
> diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> index 769d750..2f6b077 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -456,6 +456,9 @@ variants:
>              - fmt_raw:
>                  image_format_stg = raw
>  
> +    - qmp_basic: install setup unattended_install.cdrom
> +        type = qmp_basic
> +
>      - vlan:  install setup unattended_install.cdrom
>          type = vlan
>          # subnet should not be used by host

Re: [PATCH 3/3] Introduce QMP basic test-suite

[Luiz Capitulino]

On Mon, 18 Oct 2010 09:42:47 -0200
Lucas Meneghel Rodrigues <lmr@redhat.com> wrote:

> On Fri, 2010-10-15 at 17:52 -0300, Luiz Capitulino wrote:
> > This commit introduces a suite which checks that QMP conforms to its
> > specification (which is file QMP/qmp-spec.txt in QEMU's source tree).
> > 
> > It's important to note that this suite does _not_ do command or
> > asynchronous messages testing, as each command or asynchronous message
> > has its own specification, and thus should be tested separately.
> > 
> > This suite is limited to test that the basic protocol works as specified,
> > that is, to ensure that the greeting message, success and error responses
> > contain the basic information the spec says they do. Additionally,
> > several errors conditions at the protocol level are also tested.
> 
> This is a very good test quality wise, thanks for the effort writing it!

Thanks.

> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  client/tests/kvm/tests/qmp_basic.py    |  374 ++++++++++++++++++++++++++++++++
> >  client/tests/kvm/tests_base.cfg.sample |    3 +
> >  2 files changed, 377 insertions(+), 0 deletions(-)
> >  create mode 100644 client/tests/kvm/tests/qmp_basic.py
> > 
> > diff --git a/client/tests/kvm/tests/qmp_basic.py b/client/tests/kvm/tests/qmp_basic.py
> > new file mode 100644
> > index 0000000..21198ef
> > --- /dev/null
> > +++ b/client/tests/kvm/tests/qmp_basic.py
> > @@ -0,0 +1,374 @@
> > +import kvm_test_utils
> > +from autotest_lib.client.common_lib import error
> > +
> > +def run_qmp_basic(test, params, env):
> > +    """
> > +    QMP Specification test-suite: this checks if the *basic* protocol conforms
> > +    to its specification, which is file QMP/qmp-spec.txt in QEMU's source tree.
> > +
> > +    IMPORTANT NOTES:
> > +
> > +        o Most tests depend heavily on QMP's error information (eg. classes),
> > +          this might have bad implications as the error interface is going to
> > +          change in QMP
> 
> ^ I was thinking about this, perhaps in the future, when the error
> interface does change, this test can be extended to do check the
> capabilities of the monitor and act differently according to the results
> of the check.

Yeah, we'll need something like this.

> > +        o Command testing is *not* covered in this suite. Each command has its
> > +          own specification and should be tested separately
> > +
> > +        o We use the same terminology as used by the QMP specification,
> > +          specially with regard to JSON types (eg. a Python dict is called
> > +          a json-object)
> > +
> > +        o This is divided in sub test-suites, please check the bottom of this
> > +          file to check the order in which they are run
> > +
> > +    TODO:
> > +
> > +        o Finding which test failed is not as easy as it should be
> 
> ^ What I usually do is to keep a failure dictionary/list that contains
> all tests that failed at a given time, and allways append new errors to
> it when they happen. In the end of the test, when raising
> error.TestError, this contextual info can be added to the exception.

Looks like a good idea, but I think this should be part of the autotest API.

Even better, I really think that we should introduce the notion of test suite
and test cases and provide appropriate assert methods.

A good way to do this would be to use Python's unittest module, this would
bring a number of important features I miss in kvm-autotest:

 o test fixture
 o The notion of test suite and test case: helps keeping things separate and
   identifying which test has failed
 o Ability to run tests from the command-line
 o Good assert methods
 o Sane output

The problem with kvm-autotest output today is that, it's more about debugging
kvm-autotest then reporting appropriate test results.

I know that there are scripts to make a better report, but this looks like a
workaround to me. Take a look at C check's output:

Success:

~/src/qmp-unstable (mon-save-transfers-v0)/ ./check-qint 
Running suite(s): QInt test-suite
100%: Checks: 5, Failures: 0, Errors: 0
~/src/qmp-unstable (master)/

Failure:

~/src/qmp-unstable (master *)/ ./check-qint 
Running suite(s): QInt test-suite
80%: Checks: 5, Failures: 1, Errors: 0
check-qint.c:64:F:Public Interface:qint_get_int_test:0: Assertion 'qint_get_int(qi) == 2' failed
~/src/qmp-unstable (master *)/ 

Python's unittest module output is very similar.

I don't think we should have the exactly same output, but IMO the current one
is only useful to know that kvm-autotest is not stuck (which is bad from a
usability perspective).

> 
> > +        o Are all those check_*() functions really needed? Wouldn't a
> > +          specialized class (eg. a Response class) do better?
> 
> ^ It is an interesting idea, we could wrap all the checks on a Response
> class. Also, it seems to me that we could have a better implementation
> for checking with assertions, such as:
> 
> if not isinstance(qmp_dict, dict):
>     raise error.TestFail(...
> 
> assert isinstance(qmp_dict, dict), "qmp_dict is not a dict...
> 
> And during the actual test, run the tests in a try/except block that
> traps AssertionErrors and turns them into error.TestFails

Can we consider this a future improvement?

The current series is very useful as is and I'd like to get it merged ASAP.

> > +    """
> > +    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')" % type(qmp_dict))
> > +        if not key in qmp_dict:
> > +            raise error.TestFail("'%s' key doesn't exist in dict ('%s')"
> > +                                 % (key, str(qmp_dict)))
> > +
> > +    def check_dict_key(qmp_dict, key, keytype):
> > +        """
> > +        Performs the following checks on a QMP dict key:
> > +
> > +        1. qmp_dict is a dict
> > +        2. key exists in qmp_dict
> > +        3. key is of type keytype
> > +        
> > +        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])))
> > +
> > +    def check_key_is_dict(qmp_dict, key):
> > +        check_dict_key(qmp_dict, key, dict)
> > +
> > +    def check_key_is_list(qmp_dict, key):
> > +        check_dict_key(qmp_dict, key, list)
> > +
> > +    def check_key_is_str(qmp_dict, key):
> > +        check_dict_key(qmp_dict, key, unicode)
> > +
> > +    def check_str_key(qmp_dict, keyname, value=None):
> > +        check_dict_key(qmp_dict, keyname, unicode)
> > +        if value and value != qmp_dict[keyname]:
> > +            raise error.TestFail("'%s' key value '%s' should be '%s'"
> > +                                 % (keyname, str(qmp_dict[keyname]), str(value)))
> > +
> > +    def check_key_is_int(qmp_dict, key):
> > +        fail_no_key(qmp_dict, key)
> > +        try:
> > +            value = int(qmp_dict[key])
> > +        except:
> > +            raise error.TestFail("'%s' key is not of type int, it's '%s'"
> > +                                 % (key, type(qmp_dict[key])))
> > +
> > +    def check_bool_key(qmp_dict, keyname, value=None):
> > +        check_dict_key(qmp_dict, keyname, bool)
> > +        if value and value != qmp_dict[keyname]:
> > +            raise error.TestFail("'%s' key value '%s' should be '%s'"
> > +                                 % (keyname, str(qmp_dict[keyname]), str(value)))
> > +
> > +    def check_success_resp(resp, empty=False):
> > +        """
> > +        Check QMP OK response.
> > +
> > +        @param resp: QMP response
> > +        @param empty: if True, response should not contain data to return
> > +        """
> > +        check_key_is_dict(resp, "return")
> > +        if empty and len(resp["return"]) > 0:
> > +            raise error.TestFail("success response is not empty ('%s')"
> > +                                 % str(resp))
> 
> ^ Here a little detail in this format string operation: We try to follow
> PEP 8 as much as possible so the % operator should be in the same line
> as the initial string, ie, this should have been written as:
> 
> raise error.TestFail("success response is not empty ('%s')" %
>                      str(resp))
> 
> Please consider this for all other similar instances of this construct.

Will do and submit a v2.

> > +    def check_error_resp(resp, classname=None, datadict=None):
> > +        """
> > +        Check QMP error response.
> > +
> > +        @param resp: QMP response
> > +        @param classname: Expected error class name
> > +        @param datadict: Expected error data dictionary
> > +        """
> > +        check_key_is_dict(resp, "error")
> > +        check_key_is_str(resp["error"], "class")
> > +        if classname and resp["error"]["class"] != classname:
> > +            raise error.TestFail("got error class '%s' expected '%s'"
> > +                                 % (resp["error"]["class"], classname))
> > +        check_key_is_dict(resp["error"], "data")
> > +        if datadict and resp["error"]["data"] != datadict:
> > +            raise error.TestFail("got data dict '%s' expected '%s'"
> > +                                 % (resp["error"]["data"], datadict))
> > +
> > +    def test_version(version):
> > +        """
> > +        Check the QMP greeting message version key which, according to QMP's
> > +        documentation, should be:
> > +
> > +        { "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)
> > +        check_key_is_str(version, "package")
> 
> ^ Forgot to put 2 lines between the 2 methods

Will fix.

> > +    def test_greeting(greeting):
> > +        check_key_is_dict(greeting, "QMP")
> > +        check_key_is_dict(greeting["QMP"], "version")
> > +        check_key_is_list(greeting["QMP"], "capabilities")
> 
> ^ Same here
> 
> > +    def greeting_suite(monitor):
> 
> ^ About the suites: As they all get a monitor as a parameter, it seems
> better to write them as classes, that get a monitor parameter on its
> constructor, then all the suite functions are methods of that class.

I considered doing that, but then I thought that the best way would be
to have a 'real' notion of test suite and test cases, which misses in
kvm-autotest today.

> > +        """
> > +        Check the greeting message format, as described in the QMP
> > +        specfication section '2.2 Server Greeting'.
> > +
> > +        { "QMP": { "version": json-object, "capabilities": json-array } }
> > +        """
> > +        greeting = monitor.get_greeting()
> > +        test_greeting(greeting)
> > +        test_version(greeting["QMP"]["version"])
> > +
> > +
> > +    def json_parsing_errors_suite(monitor):
> > +        """
> > +        Check that QMP's parser is able to recover from parsing errors, please
> > +        check the JSON spec for more info on the JSON syntax (RFC 4627).
> > +        """
> > +        # We're quite simple right now and the focus is on parsing errors that
> > +        # have already biten us in the past.
> 
> ^ Excellent, regression suites are usually a great way to tell whether
> functionalities have regressed or not :)
> 
> > +        # TODO: The following test-cases are missing:
> > +        #
> > +        #   - JSON numbers, strings and arrays
> > +        #   - More invalid characters or malformed structures
> > +        #   - Valid, but not obvious syntax, like zillion of spaces or
> > +        #     strings with unicode chars (different suite maybe?)
> > +        bad_json = []
> > +
> > +        # A JSON value MUST be an object, array, number, string, true, false,
> > +        # or null
> > +        #
> > +        # NOTE: QMP seems to ignore a number of chars, like: | and ?
> > +        bad_json.append(":")
> > +        bad_json.append(",")
> > +
> > +        # Malformed json-objects
> > +        #
> > +        # NOTE: sending only "}" seems to break QMP
> 
> ^ Should we open a bug about it?

Will do, I forgot.

> 
> > +        # NOTE: Duplicate keys are accepted (should it?)
> 
> ^ It most likely shouldn't.

Yes, the JSON standard uses the SHOULD keyword for this, so this conforms
to the spec, we just need to be sure that this is what we want.

> 
> > +        bad_json.append("{ \"execute\" }")
> > +        bad_json.append("{ \"execute\": \"query-version\", }")
> > +        bad_json.append("{ 1: \"query-version\" }")
> > +        bad_json.append("{ true: \"query-version\" }")
> > +        bad_json.append("{ []: \"query-version\" }")
> > +        bad_json.append("{ {}: \"query-version\" }")
> > +
> > +        for cmd in bad_json:
> > +            resp = monitor.send(cmd)
> > +            check_error_resp(resp, "JSONParsing")
> > +
> > +
> > +    def test_id_key(monitor):
> > +        """
> > +        Check that QMP's "id" key is correctly handled.
> > +        """
> > +        # The "id" key must be echoed back in error responses
> > +        id = "kvm-autotest"
> > +        resp = monitor.cmd_qmp("eject", { "foobar": True }, id=id)
> > +        check_error_resp(resp)
> > +        check_str_key(resp, "id", id)
> > +
> > +        # The "id" key must be echoed back in success responses
> > +        resp = monitor.cmd_qmp("query-status", id=id)
> > +        check_success_resp(resp)
> > +        check_str_key(resp, "id", id)
> > +
> > +        # The "id" key can be any json-object
> > +        for id in [ True, 1234, "string again!", [1, [], {}, True, "foo"],
> > +                    { "key": {} } ]:
> > +            resp = monitor.cmd_qmp("query-status", id=id)
> > +            check_success_resp(resp)
> > +            if resp["id"] != id:
> > +                raise error.TestFail("expected id '%s' but got '%s'"
> > +                                     % (str(id), str(resp["id"])))
> > +
> > +    def test_invalid_arg_key(monitor):
> > +        """
> > +        Currently, the only supported keys in the input object are: "execute",
> > +        "arguments" and "id". Although expansion is supported, invalid key
> > +        names must be detected.
> > +        """
> > +        resp = monitor.cmd_obj({ "execute": "eject", "foobar": True })
> > +        check_error_resp(resp, "QMPExtraInputObjectMember",
> > +                         { "member": "foobar" })
> > +
> > +    def test_bad_arguments_key_type(monitor):
> > +        """
> > +        The "arguments" key must be an json-object.
> > +        
> > +        We use the eject command to perform the tests, but that's a random
> > +        choice, any command that accepts arguments will do, as the command
> > +        doesn't get called.
> > +        """
> > +        for item in [ True, [], 1, "foo" ]:
> > +            resp = monitor.cmd_obj({ "execute": "eject", "arguments": item })
> > +            check_error_resp(resp, "QMPBadInputObjectMember",
> > +                             { "member": "arguments", "expected": "object" })
> > +
> > +    def test_bad_execute_key_type(monitor):
> > +        """
> > +        The "execute" key must be a json-string.
> > +        """
> > +        for item in [ False, 1, {}, [] ]:
> > +            resp = monitor.cmd_obj({ "execute": item })
> > +            check_error_resp(resp, "QMPBadInputObjectMember",
> > +                             { "member": "execute", "expected": "string" })
> > +
> > +    def test_no_execute_key(monitor):
> > +        """
> > +        The "execute" key must exist, we also test for some stupid parsing
> > +        errors.
> > +        """
> > +        for cmd in [ {}, { "execut": "qmp_capabilities" },
> > +                     { "executee": "qmp_capabilities" }, { "foo": "bar" }]:
> > +            resp = monitor.cmd_obj(cmd)
> > +            check_error_resp(resp) # XXX: check class and data dict?
> > +
> > +    def test_bad_input_obj_type(monitor):
> > +        """
> > +        The input object must be... an json-object.
> > +        """
> > +        for cmd in [ "foo", [], True, 1 ]:
> > +            resp = monitor.cmd_obj(cmd)
> > +            check_error_resp(resp, "QMPBadInputObject", { "expected":"object" })
> > +
> > +    def test_good_input_obj(monitor):
> > +        """
> > +        Basic success tests for issuing QMP commands.
> > +        """
> > +        # NOTE: We don't use the cmd_qmp() method here because the command
> > +        # object is in a 'random' order
> > +        resp = monitor.cmd_obj({ "execute": "query-version" })
> > +        check_success_resp(resp)
> > +
> > +        resp = monitor.cmd_obj({ "arguments": {}, "execute": "query-version" })
> > +        check_success_resp(resp)
> > +
> > +        id = "1234foo"
> > +        resp = monitor.cmd_obj({ "id": id, "execute": "query-version",
> > +                                 "arguments": {} })
> > +        check_success_resp(resp)
> > +        check_str_key(resp, "id", id)
> > +
> > +        # TODO: would be good to test simple argument usage, but we don't have
> > +        # a read-only command that accepts arguments.
> > +
> > +    def input_object_suite(monitor):
> > +        """
> > +        Check the input object format, as described in the QMP specfication
> > +        section '2.3 Issuing Commands'.
> > +
> > +        { "execute": json-string, "arguments": json-object, "id": json-value }
> > +        """
> > +        test_good_input_obj(monitor)
> > +        test_bad_input_obj_type(monitor)
> > +        test_no_execute_key(monitor)
> > +        test_bad_execute_key_type(monitor)
> > +        test_bad_arguments_key_type(monitor)
> > +        test_id_key(monitor)
> > +        test_invalid_arg_key(monitor)
> > +
> > +    def argument_checker_suite(monitor):
> > +        """
> > +        Check that QMP's argument checker is detecting all possible errors.
> > +
> > +        We use a number of different commands to perform the checks, but the
> > +        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" })
> > +
> > +        # '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
> > +        # error that is generated by the handler itself.
> > +        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 ba a json-int
> 
> ^ Typo in the comment
> 
> > +        for arg in [ {}, [], True, "foo" ]:
> > +            resp = monitor.cmd_qmp("memsave", { "val": arg, "filename": "foo",
> > +                                                "size": 10 })
> > +            check_error_resp(resp, "InvalidParameterType",
> > +                             { "name": "val", "expected": "int" })
> > +
> > +        # value argument must ba a json-number
> 
> ^ Typo in the comment

Will fix.

> 
> > +        for arg in [ {}, [], True, "foo" ]:
> > +            resp = monitor.cmd_qmp("migrate_set_speed", { "value": arg })
> > +            check_error_resp(resp, "InvalidParameterType",
> > +                             { "name": "value", "expected": "number" })
> > +
> > +        # qdev-type commands have their own argument checker, all QMP does
> > +        # is to skip its checking and pass arguments through. Check this
> > +        # works by providing invalid options to device_add and expecting
> > +        # an error message from qdev
> > +        resp = monitor.cmd_qmp("device_add", { "driver": "e1000","foo": "bar" })
> > +        check_error_resp(resp, "PropertyNotFound",
> > +                               {"device": "e1000", "property": "foo"})
> > +
> > +
> > +    def unknown_commands_suite(monitor):
> > +        """
> > +        Check that QMP handles unknown commands correctly.
> > +        """
> > +        # We also call a HMP-only command, to be sure it will fail as expected
> > +        for cmd in [ "bar", "query-", "query-foo", "q", "help" ]:
> > +            resp = monitor.cmd_qmp(cmd)
> > +            check_error_resp(resp, "CommandNotFound", { "name": cmd })
> > +
> > +
> > +    vm = kvm_test_utils.get_living_vm(env, params.get("main_vm"))
> > +
> > +    # Run all suites
> > +    greeting_suite(vm.monitor)
> > +    input_object_suite(vm.monitor)
> > +    argument_checker_suite(vm.monitor)
> > +    unknown_commands_suite(vm.monitor)
> > +    json_parsing_errors_suite(vm.monitor)
> > +
> > +    # check if QMP is still alive
> > +    if not vm.monitor.is_responsive():
> > +        raise error.TestFail('QEMU is not alive after QMP testing')
> > diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
> > index 769d750..2f6b077 100644
> > --- a/client/tests/kvm/tests_base.cfg.sample
> > +++ b/client/tests/kvm/tests_base.cfg.sample
> > @@ -456,6 +456,9 @@ variants:
> >              - fmt_raw:
> >                  image_format_stg = raw
> >  
> > +    - qmp_basic: install setup unattended_install.cdrom
> > +        type = qmp_basic
> > +
> >      - vlan:  install setup unattended_install.cdrom
> >          type = vlan
> >          # subnet should not be used by host
> 
> 

Re: [PATCH 3/3] Introduce QMP basic test-suite

[Lucas Meneghel Rodrigues]

On Mon, 2010-10-18 at 12:55 -0200, Luiz Capitulino wrote:
> On Mon, 18 Oct 2010 09:42:47 -0200
> Lucas Meneghel Rodrigues <lmr@redhat.com> wrote:
> 
> > On Fri, 2010-10-15 at 17:52 -0300, Luiz Capitulino wrote:
> > > This commit introduces a suite which checks that QMP conforms to its
> > > specification (which is file QMP/qmp-spec.txt in QEMU's source tree).
> > > 
> > > It's important to note that this suite does _not_ do command or
> > > asynchronous messages testing, as each command or asynchronous message
> > > has its own specification, and thus should be tested separately.
> > > 
> > > This suite is limited to test that the basic protocol works as specified,
> > > that is, to ensure that the greeting message, success and error responses
> > > contain the basic information the spec says they do. Additionally,
> > > several errors conditions at the protocol level are also tested.
> > 
> > This is a very good test quality wise, thanks for the effort writing it!
> 
> Thanks.
> 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  client/tests/kvm/tests/qmp_basic.py    |  374 ++++++++++++++++++++++++++++++++
> > >  client/tests/kvm/tests_base.cfg.sample |    3 +
> > >  2 files changed, 377 insertions(+), 0 deletions(-)
> > >  create mode 100644 client/tests/kvm/tests/qmp_basic.py
> > > 
> > > diff --git a/client/tests/kvm/tests/qmp_basic.py b/client/tests/kvm/tests/qmp_basic.py
> > > new file mode 100644
> > > index 0000000..21198ef
> > > --- /dev/null
> > > +++ b/client/tests/kvm/tests/qmp_basic.py
> > > @@ -0,0 +1,374 @@
> > > +import kvm_test_utils
> > > +from autotest_lib.client.common_lib import error
> > > +
> > > +def run_qmp_basic(test, params, env):
> > > +    """
> > > +    QMP Specification test-suite: this checks if the *basic* protocol conforms
> > > +    to its specification, which is file QMP/qmp-spec.txt in QEMU's source tree.
> > > +
> > > +    IMPORTANT NOTES:
> > > +
> > > +        o Most tests depend heavily on QMP's error information (eg. classes),
> > > +          this might have bad implications as the error interface is going to
> > > +          change in QMP
> > 
> > ^ I was thinking about this, perhaps in the future, when the error
> > interface does change, this test can be extended to do check the
> > capabilities of the monitor and act differently according to the results
> > of the check.
> 
> Yeah, we'll need something like this.
> 
> > > +        o Command testing is *not* covered in this suite. Each command has its
> > > +          own specification and should be tested separately
> > > +
> > > +        o We use the same terminology as used by the QMP specification,
> > > +          specially with regard to JSON types (eg. a Python dict is called
> > > +          a json-object)
> > > +
> > > +        o This is divided in sub test-suites, please check the bottom of this
> > > +          file to check the order in which they are run
> > > +
> > > +    TODO:
> > > +
> > > +        o Finding which test failed is not as easy as it should be
> > 
> > ^ What I usually do is to keep a failure dictionary/list that contains
> > all tests that failed at a given time, and allways append new errors to
> > it when they happen. In the end of the test, when raising
> > error.TestError, this contextual info can be added to the exception.
> 
> Looks like a good idea, but I think this should be part of the autotest API.
> 
> Even better, I really think that we should introduce the notion of test suite
> and test cases and provide appropriate assert methods.
> 
> A good way to do this would be to use Python's unittest module, this would
> bring a number of important features I miss in kvm-autotest:
> 
>  o test fixture
>  o The notion of test suite and test case: helps keeping things separate and
>    identifying which test has failed
>  o Ability to run tests from the command-line
>  o Good assert methods
>  o Sane output
> 
> The problem with kvm-autotest output today is that, it's more about debugging
> kvm-autotest then reporting appropriate test results.
> 
> I know that there are scripts to make a better report, but this looks like a
> workaround to me. Take a look at C check's output:
> 
> Success:
> 
> ~/src/qmp-unstable (mon-save-transfers-v0)/ ./check-qint 
> Running suite(s): QInt test-suite
> 100%: Checks: 5, Failures: 0, Errors: 0
> ~/src/qmp-unstable (master)/
> 
> Failure:
> 
> ~/src/qmp-unstable (master *)/ ./check-qint 
> Running suite(s): QInt test-suite
> 80%: Checks: 5, Failures: 1, Errors: 0
> check-qint.c:64:F:Public Interface:qint_get_int_test:0: Assertion 'qint_get_int(qi) == 2' failed
> ~/src/qmp-unstable (master *)/ 
> 
> Python's unittest module output is very similar.
> 
> I don't think we should have the exactly same output, but IMO the current one
> is only useful to know that kvm-autotest is not stuck (which is bad from a
> usability perspective).

Ok, fair points. This is something that might need to be addressed at
the autotest level, so better thinking should be put into this. I don't
expect to see it fixed overnight precisely because this should be done
carefully.

> > 
> > > +        o Are all those check_*() functions really needed? Wouldn't a
> > > +          specialized class (eg. a Response class) do better?
> > 
> > ^ It is an interesting idea, we could wrap all the checks on a Response
> > class. Also, it seems to me that we could have a better implementation
> > for checking with assertions, such as:
> > 
> > if not isinstance(qmp_dict, dict):
> >     raise error.TestFail(...
> > 
> > assert isinstance(qmp_dict, dict), "qmp_dict is not a dict...
> > 
> > And during the actual test, run the tests in a try/except block that
> > traps AssertionErrors and turns them into error.TestFails
> 
> Can we consider this a future improvement?
> 
> The current series is very useful as is and I'd like to get it merged ASAP.

^ Absolutely, that is fine by me.

> > > +    """
> > > +    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')" % type(qmp_dict))
> > > +        if not key in qmp_dict:
> > > +            raise error.TestFail("'%s' key doesn't exist in dict ('%s')"
> > > +                                 % (key, str(qmp_dict)))
> > > +
> > > +    def check_dict_key(qmp_dict, key, keytype):
> > > +        """
> > > +        Performs the following checks on a QMP dict key:
> > > +
> > > +        1. qmp_dict is a dict
> > > +        2. key exists in qmp_dict
> > > +        3. key is of type keytype
> > > +        
> > > +        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])))
> > > +
> > > +    def check_key_is_dict(qmp_dict, key):
> > > +        check_dict_key(qmp_dict, key, dict)
> > > +
> > > +    def check_key_is_list(qmp_dict, key):
> > > +        check_dict_key(qmp_dict, key, list)
> > > +
> > > +    def check_key_is_str(qmp_dict, key):
> > > +        check_dict_key(qmp_dict, key, unicode)
> > > +
> > > +    def check_str_key(qmp_dict, keyname, value=None):
> > > +        check_dict_key(qmp_dict, keyname, unicode)
> > > +        if value and value != qmp_dict[keyname]:
> > > +            raise error.TestFail("'%s' key value '%s' should be '%s'"
> > > +                                 % (keyname, str(qmp_dict[keyname]), str(value)))
> > > +
> > > +    def check_key_is_int(qmp_dict, key):
> > > +        fail_no_key(qmp_dict, key)
> > > +        try:
> > > +            value = int(qmp_dict[key])
> > > +        except:
> > > +            raise error.TestFail("'%s' key is not of type int, it's '%s'"
> > > +                                 % (key, type(qmp_dict[key])))
> > > +
> > > +    def check_bool_key(qmp_dict, keyname, value=None):
> > > +        check_dict_key(qmp_dict, keyname, bool)
> > > +        if value and value != qmp_dict[keyname]:
> > > +            raise error.TestFail("'%s' key value '%s' should be '%s'"
> > > +                                 % (keyname, str(qmp_dict[keyname]), str(value)))
> > > +
> > > +    def check_success_resp(resp, empty=False):
> > > +        """
> > > +        Check QMP OK response.
> > > +
> > > +        @param resp: QMP response
> > > +        @param empty: if True, response should not contain data to return
> > > +        """
> > > +        check_key_is_dict(resp, "return")
> > > +        if empty and len(resp["return"]) > 0:
> > > +            raise error.TestFail("success response is not empty ('%s')"
> > > +                                 % str(resp))
> > 
> > ^ Here a little detail in this format string operation: We try to follow
> > PEP 8 as much as possible so the % operator should be in the same line
> > as the initial string, ie, this should have been written as:
> > 
> > raise error.TestFail("success response is not empty ('%s')" %
> >                      str(resp))
> > 
> > Please consider this for all other similar instances of this construct.
> 
> Will do and submit a v2.
> 
> > > +    def check_error_resp(resp, classname=None, datadict=None):
> > > +        """
> > > +        Check QMP error response.
> > > +
> > > +        @param resp: QMP response
> > > +        @param classname: Expected error class name
> > > +        @param datadict: Expected error data dictionary
> > > +        """
> > > +        check_key_is_dict(resp, "error")
> > > +        check_key_is_str(resp["error"], "class")
> > > +        if classname and resp["error"]["class"] != classname:
> > > +            raise error.TestFail("got error class '%s' expected '%s'"
> > > +                                 % (resp["error"]["class"], classname))
> > > +        check_key_is_dict(resp["error"], "data")
> > > +        if datadict and resp["error"]["data"] != datadict:
> > > +            raise error.TestFail("got data dict '%s' expected '%s'"
> > > +                                 % (resp["error"]["data"], datadict))
> > > +
> > > +    def test_version(version):
> > > +        """
> > > +        Check the QMP greeting message version key which, according to QMP's
> > > +        documentation, should be:
> > > +
> > > +        { "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)
> > > +        check_key_is_str(version, "package")
> > 
> > ^ Forgot to put 2 lines between the 2 methods
> 
> Will fix.
> 
> > > +    def test_greeting(greeting):
> > > +        check_key_is_dict(greeting, "QMP")
> > > +        check_key_is_dict(greeting["QMP"], "version")
> > > +        check_key_is_list(greeting["QMP"], "capabilities")
> > 
> > ^ Same here
> > 
> > > +    def greeting_suite(monitor):
> > 
> > ^ About the suites: As they all get a monitor as a parameter, it seems
> > better to write them as classes, that get a monitor parameter on its
> > constructor, then all the suite functions are methods of that class.
> 
> I considered doing that, but then I thought that the best way would be
> to have a 'real' notion of test suite and test cases, which misses in
> kvm-autotest today.

Ok, fair enough. So when you can, send me the v2 and I'll do a last
check, test it a bit and then get it upstream.

Thank you!