Re: [Qemu-devel] [PATCH v4 06/10] tests: add qmp/object-add-without-props test

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
> On Thu, Aug 30, 2018 at 3:01 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@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.lureau@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.

I beg to differ.  assert(!ret) is idiomatic C.

I file g_assert_nonnull() under "redundant crap GLib dumps into my
limited identifier memory just because it can", along with similar
beauties g_assert_false(), guint, gconstpointer, ...

>> > +
>> > +    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);
> }

Drawback: the assertion messages no longer point to the test that broke.
Do we care?

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

Yes.  Your commit message admits "we don't have systematic object-add
tests".  The TODO comment copies that to the code.  It only asks for
negative tests, because these are the only generic ones (I think), and
object-specific tests should go elsewhere.