From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59723) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brlQQ-0007qe-0H for qemu-devel@nongnu.org; Wed, 05 Oct 2016 08:35:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brlQJ-0003TM-Tv for qemu-devel@nongnu.org; Wed, 05 Oct 2016 08:35:52 -0400 From: Markus Armbruster References: <20160922203927.28241-1-marcandre.lureau@redhat.com> <20160922203927.28241-4-marcandre.lureau@redhat.com> <8760p7yv8n.fsf@dusky.pond.sub.org> <934105962.522999.1475662607447.JavaMail.zimbra@redhat.com> Date: Wed, 05 Oct 2016 14:35:44 +0200 In-Reply-To: <934105962.522999.1475662607447.JavaMail.zimbra@redhat.com> (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Wed, 5 Oct 2016 06:16:47 -0400 (EDT)") Message-ID: <87bmyzvuqn.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 3/3] tests: start generic qemu-qmp tests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , paolo bonzini , qemu-devel@nongnu.org, qemu-stable@nongnu.org Marc-Andr=C3=A9 Lureau writes: > Hi > > ----- Original Message ----- >> Marc-Andr=C3=A9 Lureau writes: >>=20 >> > These 2 tests exhibit two qmp bugs fixed by the previous patches. >> > >> > Signed-off-by: Marc-Andr=C3=A9 Lureau >> > Reviewed-by: Daniel P. Berrange >> > Reviewed-by: Eric Blake >> > --- >> > 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 >>=20 >> 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. >>=20 > > Not sure we have a strong conventions, but I renamed it after Daniel aske= d 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 =3D qmp("{'execute': 'object-add'," >> > + " 'arguments': { 'qom-type': 'memory-backend-ram', 'id': >> > 'ram1' } }"); >> > + g_assert_nonnull(ret); >> > + >> > + error =3D qdict_get_qdict(ret, "error"); >> > + klass =3D qdict_get_try_str(error, "class"); >> > + desc =3D qdict_get_try_str(error, "desc"); >> > + >> > + g_assert_cmpstr(klass, =3D=3D, "GenericError"); >> > + g_assert_cmpstr(desc, =3D=3D, "can't create backend with size 0"); >> > + >> > + QDECREF(ret); >> > +} >>=20 >> 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. >>=20 >> > + >> > +static void test_qom_set_without_value(void) >> > +{ >> > + QDict *ret, *error; >> > + const gchar *klass, *desc; >> > + >> > + ret =3D qmp("{'execute': 'qom-set'," >> > + " 'arguments': { 'path': '/machine', 'property': 'rtc-t= ime' >> > } }"); >> > + g_assert_nonnull(ret); >> > + >> > + error =3D qdict_get_qdict(ret, "error"); >> > + klass =3D qdict_get_try_str(error, "class"); >> > + desc =3D qdict_get_try_str(error, "desc"); >> > + >> > + g_assert_cmpstr(klass, =3D=3D, "GenericError"); >> > + g_assert_cmpstr(desc, =3D=3D, "Parameter 'value' is missing"); >> > + >> > + QDECREF(ret); >> > +} >>=20 >> 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 kee= p 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. >>=20 >> We have: >>=20 >> * QAPI schema tests: tests/qapi-schema/ >>=20 >> 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. >>=20 >> * tests/test-qmp-commands.c >>=20 >> This file provides three things: >>=20 >> - 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 >>=20 >> The file name became misleading when we added the third one. It >> should be split off. >>=20 >> * tests/test-qmp-event.c >>=20 >> QMP core event sending unit tests. >>=20=20=20 >> * 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 >>=20 >> As the recent bugs demonstrated, these tests have holes. Could use >> systematic review for coverage. >>=20 >> By the way, we should try to get rid of the non-strict QMP input >> visitor. >>=20 >> * tests/test-visitor-serialization.c >>=20 >> Tests an input / output visitor pair. >>=20 >> * 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. >>=20 >> 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. >>=20 >> 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 c= hecks. > > Furthermore, I hope it paves the way to more qmp checks, for when qemu qm= p commands are introduced (and are generic/simple enough to not need a sepe= rate 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. *=20 * 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?