From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53092) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brjG0-000460-NG for qemu-devel@nongnu.org; Wed, 05 Oct 2016 06:17:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brjFu-00048u-M9 for qemu-devel@nongnu.org; Wed, 05 Oct 2016 06:16:59 -0400 Date: Wed, 5 Oct 2016 06:16:47 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <934105962.522999.1475662607447.JavaMail.zimbra@redhat.com> In-Reply-To: <8760p7yv8n.fsf@dusky.pond.sub.org> References: <20160922203927.28241-1-marcandre.lureau@redhat.com> <20160922203927.28241-4-marcandre.lureau@redhat.com> <8760p7yv8n.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: Markus Armbruster Cc: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , qemu-devel@nongnu.org, paolo bonzini , qemu-stable@nongnu.org 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 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 =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-ti= me' > > } }"); > > + 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 keep = mine? :) >=20 >=20 > 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 > * 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 > 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 q= mp-dispatch, so I would like to have (at least some minimal) end-to-end che= cks. 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 sepera= te unit test).