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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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.
> 

Not sure we have a strong conventions, but I renamed it after Daniel asked me 
to. qmp-test.c is fine for me too.

> > @@ -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?

Hmm, you add a narrowed "redundant" test and ask me whether we should keep 
mine? :)

> 
> 
> 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

That whole description could be a patch to docs/qmp-spec.txt

> 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?

I think this test would have helped to spot the regression when moving to 
qmp-dispatch, so I would like to have (at least some minimal) end-to-end checks.

Furthermore, I hope it paves the way to more qmp checks, for when qemu qmp 
commands are introduced (and are generic/simple enough to not need a seperate 
unit test).