Re: [Qemu-devel] [PATCH 09/12] tests: add a few qemu-qmp tests

[Marc-André Lureau]

Hi

On Tue, Jul 17, 2018 at 10:01 AM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> These 2 tests exhibited two qmp bugs that were fixed in 2.7
>> (series from commit e64c75a9752c5d0fd64eb2e684c656a5ea7d03c6 to
>> commit 1382d4abdf9619985e4078e37e49e487cea9935e)
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  tests/qmp-test.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> index ceaf4a6789..084c5edff0 100644
>> --- a/tests/qmp-test.c
>> +++ b/tests/qmp-test.c
>> @@ -249,7 +249,39 @@ static void test_qmp_oob(void)
>>      recv_cmd_id(qts, "blocks-2");
>>      recv_cmd_id(qts, "err-2");
>>      cleanup_blocking_cmd();
>> +}
>> +
>> +static void test_object_add_without_props(void)
>> +{
>> +    QTestState *qts;
>> +    QDict *ret;
>> +
>> +    qts = qtest_init(common_args);
>> +
>> +    ret = qtest_qmp(qts, "{'execute': 'object-add',"
>> +          " 'arguments': { 'qom-type': 'memory-backend-ram', 'id': 'ram1' } 
>> }");
>
> Please break lines between arguments instead of within.  More of the
> same below.

Sorry I am not sure I understand what you want. It's broken between
argument "execute" and "arguments", how do you want to change it?

>
>> +    g_assert_nonnull(ret);
>> +
>> +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
>> +
>> +    qobject_unref(ret);
>> +    qtest_quit(qts);
>> +}
>> +
>> +static void test_qom_set_without_value(void)
>> +{
>> +    QTestState *qts;
>> +    QDict *ret;
>> +
>> +    qts = qtest_init(common_args);
>>
>> +    ret = qtest_qmp(qts, "{'execute': 'qom-set',"
>> +              " 'arguments': { 'path': '/machine', 'property': 'rtc-time' } 
>> }");
>> +    g_assert_nonnull(ret);
>> +
>> +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
>> +
>> +    qobject_unref(ret);
>>      qtest_quit(qts);
>>  }
>>
>> @@ -479,8 +511,13 @@ int main(int argc, char *argv[])
>>
>>      g_test_init(&argc, &argv, NULL);
>>
>> +    qtest_add_func("qmp/object-add-without-props",
>> +                   test_object_add_without_props);
>> +    qtest_add_func("qmp/qom-set-without-value",
>> +                   test_qom_set_without_value);
>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>>      qtest_add_func("qmp/oob", test_qmp_oob);
>> +
>>      qmp_schema_init(&schema);
>>      add_query_tests(&schema);
>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>> @@ -488,5 +525,6 @@ int main(int argc, char *argv[])
>>      ret = g_test_run();
>>
>>      qmp_schema_cleanup(&schema);
>> +
>>      return ret;
>>  }
>
> Is this hunk intentional?
>

no

> Taking a step back: the test cases look good, but is this file an
> appropriate home?  The file comment states it's about "QMP protocol test
> cases".  These test cases test commands, not the protocol.

Actually, the tests were written to test the qapi/qmp fixes mentionned
in commit message. So they were more "protocol" related than command
related.

>
> I figure test_qom_set_without_value() belongs to qom-test.c.
>
> test_object_add_without_props() could go into a memory backend test
> collection, or an object-add test collection.  Sadly, neither exists.
> We could have a qmp command test collection as a home of last resort.
> Temptation to just throw a few random test cases there instead of
> covering (a set of related) commands with a proper test case collection.
>
> As is, your patch turns qmp-test.c into such a home of last resort.  If
> that's what we want, we should update the file comment.  But I think I'd
> rather have a separate qmp-cmd-test.c.
>
> Thoughts?

It's somewhat blurry lines to me, it would be simpler to just use qmp-test.