Re: [Qemu-stable] [Qemu-devel] [PATCH v4 3/3] tests: start generic qemu-qmp tests

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> These 2 tests exhibit two qmp bugs fixed by the previous patches.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Daniel P. Berrange <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  tests/test-qemu-qmp.c  | 69 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile.include |  2 ++
>  tests/.gitignore       |  1 +
>  3 files changed, 72 insertions(+)
>  create mode 100644 tests/test-qemu-qmp.c
>
> diff --git a/tests/test-qemu-qmp.c b/tests/test-qemu-qmp.c
> new file mode 100644
> index 0000000..9d05829
> --- /dev/null
> +++ b/tests/test-qemu-qmp.c

I guess you put -qemu- in the file name to indicate this is an
end-to-end QMP test rather than a unit test.  The existing convention
seems to be test-FOO.c for unit tests, and FOO-test.c for QTests,
i.e. tests talking to QEMU via libqtest.  So this should be called
qmp-test.c.

> @@ -0,0 +1,69 @@
> +/*
> + * QTest testcase for qemu qmp commands
> + *
> + * Copyright (c) 2016 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +
> +static void test_object_add_without_props(void)
> +{
> +    QDict *ret, *error;
> +    const gchar *klass, *desc;
> +
> +    ret = qmp("{'execute': 'object-add',"
> +              " 'arguments': { 'qom-type': 'memory-backend-ram', 'id': 
> 'ram1' } }");
> +    g_assert_nonnull(ret);
> +
> +    error = qdict_get_qdict(ret, "error");
> +    klass = qdict_get_try_str(error, "class");
> +    desc = qdict_get_try_str(error, "desc");
> +
> +    g_assert_cmpstr(klass, ==, "GenericError");
> +    g_assert_cmpstr(desc, ==, "can't create backend with size 0");
> +
> +    QDECREF(ret);
> +}

This is a test for a specific bug in a specific QMP command.  Adding a
test for a bug you fix is good practice.  The resulting suite of special
tests can complement more general tests.

> +
> +static void test_qom_set_without_value(void)
> +{
> +    QDict *ret, *error;
> +    const gchar *klass, *desc;
> +
> +    ret = qmp("{'execute': 'qom-set',"
> +              " 'arguments': { 'path': '/machine', 'property': 'rtc-time' } 
> }");
> +    g_assert_nonnull(ret);
> +
> +    error = qdict_get_qdict(ret, "error");
> +    klass = qdict_get_try_str(error, "class");
> +    desc = qdict_get_try_str(error, "desc");
> +
> +    g_assert_cmpstr(klass, ==, "GenericError");
> +    g_assert_cmpstr(desc, ==, "Parameter 'value' is missing");
> +
> +    QDECREF(ret);
> +}

This one is technically redundant with "[PATCH 2.5/3]
tests/test-qmp-input-strict: Cover missing struct members".  Does
testing the same bug end-to-end rather than narrowly add enough value to
earn its keep?


Let's take a step back and examine what QAPI/QMP tests we have, and
where we want to go.

We have:

* QAPI schema tests: tests/qapi-schema/

  This is a systematically constructed, comprehensive test suite.  I'm
  happy with it, we just need to continue maintaining it together with
  the QAPI generator scripts.

* tests/test-qmp-commands.c

  This file provides three things:

  - Stubs so we can test-compile and link the test-qmp-marshal.c
    generated for the positive QAPI schema tests in
    tests/qapi-schema/qapi-schema-test.json
  - A few rather basic QMP core command dispatch unit tests
  - QAPI deallocation unit tests

  The file name became misleading when we added the third one.  It
  should be split off.

* tests/test-qmp-event.c

  QMP core event sending unit tests.
  
* Visitor unit tests: tests/test-clone-visitor.c
  tests/test-opts-visitor.c tests/test-qmp-input-strict.c
  tests/test-qmp-input-visitor.c tests/test-qmp-output-visitor.c
  tests/test-string-input-visitor.c tests/test-string-output-visitor.c

  As the recent bugs demonstrated, these tests have holes.  Could use
  systematic review for coverage.

  By the way, we should try to get rid of the non-strict QMP input
  visitor.

* tests/test-visitor-serialization.c

  Tests an input / output visitor pair.

* Several non-QMP tests test QMP end-to-end by using it

Your patch adds a new file with end-to-end QMP tests.

End-to-end testing is probably the only practical way to test actual QMP
commands.  Your patch adds tests for two specific bugs you just fixed in
QMP commands.  I'm fine with adding such tests when we think they
provide enough value to earn their keep.

End-to-end testing can also be used to exercise the QMP core.  This
would test the same things as QMP unit tests, just at a higher level.
Do we want it anyway?

[...]