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

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

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

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

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