Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> 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.
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.
