Hi
On Thu, Aug 30, 2018 at 3:01 PM Markus Armbruster <arm...@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
> > test_object_add_without_props() tests a bug in qmp_object_add() we
> > fixed in commit e64c75a975. Sadly, we don't have systematic
> > object-add tests. This lone test can go into qmp-cmd-test for want of
> > a better home.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > tests/qmp-cmd-test.c | 31 +++++++++++++++++++++++++++++++
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> > index c5b70df974..3ba8f68476 100644
> > --- a/tests/qmp-cmd-test.c
> > +++ b/tests/qmp-cmd-test.c
> > @@ -19,6 +19,15 @@
> >
> > const char common_args[] = "-nodefaults -machine none";
> >
> > +static const char *get_error_class(QDict *resp)
> > +{
> > + QDict *error = qdict_get_qdict(resp, "error");
> > + const char *desc = qdict_get_try_str(error, "desc");
> > +
> > + g_assert(desc);
> > + return error ? qdict_get_try_str(error, "class") : NULL;
> > +}
> > +
> > /* Query smoke tests */
> >
> > static int query_error_class(const char *cmd)
>
> Copied from qmp-test.c. It should be factored out instead. Where to
> put it? libqtest.c isn't quite right, as the function could
> theoretically be useful in unit tests as well, but I guess it would do
> for now.
>
> Asserting presence of "desc" makes little sense outside qmp-test.c
> protocol tests, but it doesn't hurt, either.
>
> Grep shows more possible users in tests/drive_del-test.c and
> tests/test-qga.c.
ok
>
> > @@ -197,6 +206,24 @@ static void add_query_tests(QmpSchema *schema)
> > }
> > }
> >
> > +static void test_object_add_without_props(void)
> > +{
> > + QTestState *qts;
> > + QDict *ret;
>
> qmp-test.c and qmp-cmd-test.c commonly use @resp for the response.
ok
>
> > +
> > + qts = qtest_init(common_args);
> > +
> > + ret = qtest_qmp(qts,
> > + "{'execute': 'object-add', 'arguments':"
> > + " {'qom-type': 'memory-backend-ram', 'id': 'ram1' }
> > }");
> > + g_assert_nonnull(ret);
>
> What's wrong with g_assert(!ret)?
nothing wrong, but g_assert_nonnull is slightly more readable, both in
code and in error produced.
>
> > +
> > + g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> > +
> > + qobject_unref(ret);
>
> Permit me to digress.
>
> When you expect success, you check @resp like this:
>
> ret = qdict_get_qdict(resp, "return");
> ... laborously check @ret against expectations ...
>
> If you feel pedantically thorough, you can throw in
>
> g_assert(!qdict_haskey(resp, "error");
>
> When you expect failure, you check like this:
>
> error = qdict_get_qdict(resp, "error");
> class = qdict_get_try_str(error, "class");
> g_assert_cmpstr(class, ==, "GenericError");
>
> and perhaps
>
> g_assert(!qdict_haskey(resp, "return");
>
> get_error_class() saves a little typing in the failure case. It's still
> an awfully verbose overall, and the checking is full of holes more often
> than not. There's got to be a better way.
>
what about?
/**
* qmp_assert_error_class:
* @rsp: QMP response to check for error
* @class: an error class
*
* Assert the response has the given error class and discard @rsp.
*/
void qmp_assert_error_class(QDict *rsp, const char *class)
{
QDict *error = qdict_get_qdict(rsp, "error");
g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, class);
g_assert_nonnull(qdict_get_try_str(error, "desc"));
g_assert_null(qdict_get(error, "return"));
qobject_unref(rsp);
}
> > + qtest_quit(qts);
> > +}
> > +
> > int main(int argc, char *argv[])
> > {
> > QmpSchema schema;
> > @@ -206,6 +233,10 @@ int main(int argc, char *argv[])
> >
> > qmp_schema_init(&schema);
> > add_query_tests(&schema);
> > +
> > + qtest_add_func("qmp/object-add-without-props",
> > + test_object_add_without_props);
> > +
> > ret = g_test_run();
> >
> > qmp_schema_cleanup(&schema);
>
> May I have a TODO comment asking for coverage of generic object-add
> failure modes?
You mean checking for other kind of failures? ok
--
Marc-André Lureau
