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

[Markus Armbruster]

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

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

Dan wrote "we're not 100% perfect, but the most common convention in
tests/ is to use 'test-' as the name prefix for files that contain test
suites eg test-qemu-qmp.c".  Actually true only for unit tests.

Of the 81 files that include libqtest.h, 15 are test infrastructure
(libqtest or libqos), 60 are named *-test.c, and 6 are named
differently.

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

In my defense, my unit test covers *all* visitor methods, while your QMP
test covers just the one used by qom-set.

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

Capturing this information in the tree is a good idea.
docs/qmp-spec.txt doesn't feel right, though: that file is about the
protocol, not about testing the implementation.  I guess the closest we
have in docs/ is docs/writing-qmp-commands.txt, but that isn't ideal,
either.  We could add tests/README.qmp or docs/qmp-implementation.txt.
To make the latter work, we'd have to cover more than just testing.

Regardless, the individual files should have decent file comments.
Let's start with that.  I'll see what I can do.

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

Yes, tests for QMP commands are nice to have, and once good examples
exist, we can more easily ask for them when people add commands.

Your patch is a start: it provides a home for QMP command tests that
don't want their own test program, and two simple tests people can
imitate.  It doesn't quite provide a good example, yet: the tests only
demonstrate two specific bugs, they don't really exercise the two
commands.

I'd be willing to take this as is with a suitable TODO comment
explaining where we want to go with this file.  Perhaps

/*
 * This program tests QMP commands that aren't interesting enough to
 * warrant their own test program.
 * 
 * TODO The tests we got here now aren't good examples, because they
 * don't really exercise the commands, but only demonstrate specific
 * bugs we've fixed.
 */

What do you think?