Hi
On Tue, Jul 17, 2018 at 3:15 PM, Markus Armbruster <arm...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
>> Hi
>>
>> On Tue, Jul 17, 2018 at 10:01 AM, Markus Armbruster <arm...@redhat.com>
>> wrote:
>>> Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
>>>> ---
>>>> 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?
>
> The line is broken in the middle of the second argument to qtest_qmp().
>
> Line breaks I'd like better:
>
> ret = qtest_qmp(qts,
> "{'execute': 'object-add',"
> " 'arguments': {'qom-type': 'memory-backend-ram',
> " 'id': 'ram1'}}");
>
> ret = qtest_qmp(qts,
> "{'execute': 'object-add',"
> " 'arguments': {"
> " 'qom-type': 'memory-backend-ram', 'id': 'ram1'}}");
>
> ret = qtest_qmp(qts,
> "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1'}}");
>
ok (I picked the last, they all look equally bad/good to me ;)
>>>> + 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
>
> Easy enough to drop :)
>
>>> 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.
>
> Let's see...
>
> 1. test_object_add_without_props() tests the bug fixed in commit
> e64c75a975 "qmp: fix object-add assert() without props"
>
> object-add's parameter @props is optional, qmp_object_add() fails to
> default it to {}, which user_creatable_add_type() requires. This is
> a bug in the command handler. It went unnoticed because object-add
> lacks systematic tests. You add one test case, which is welcome, but
> what we really want is at systematic coverage of all paths through
> qmp_object_add().
>
> Aside: we want that for every QMP command. We have it for next to
> none of them.
>
> Anyway, neither the bug nor its test are about the QMP protocol.
you are right
>
> 2. test_qom_set_without_value() tests the bug fixed in commit c489780203
> "qapi: Fix crash when 'any' or 'null' parameter is missing"
>
> The QMP input visitor neglects to check qmp_input_get_object() for
> failure. The bug is *not* in qom-set; qom-set is merely part of the
> reproducer. You're right, this case is 'more "protocol" related than
> command related.'
>
> We do have tests for the QMP input visitor. You fixed them to cover
> this case in the very next commit bce3035a44
> "tests/test-qmp-input-strict: Cover missing struct members". That's
> a unit test.
>
> Should we test it once more at the QMP level in a qtest? If yes,
> then we should test all the errors the visitor should report once
> there, shouldn't we? Mind, I'm not saying *you* should.
>
As you said commit bce3035a44 covers it at a lower/unit level. The QMP
test is the user level, and the original reproducer. Both have values,
even if they reach the same condition. Up to you if you prefer to drop
it.
>>>
>>> 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.
>
> Simpler, yes, but creating a new file isn't really hard, either.
>
> In current master, qmp-test.c has just under 500 lines. It's already
> one of the larger files in tests/. Certainly not too big, but I'd
> prefer to keep it focused.
>
> I defocused it some myself when I added the query smoke tests (commit
> e4a426e75e tests/qmp-test: Add generic, basic test of query commands).
> It was simpler to just use qmp-test. I shouldn't have.
>
> I'll prepare a patch to split qmp-test.c into a command-independent part
> (protocol, infrastructure) and a part about commands.
>
Fine with me, although it could be done after this commit is applied I suppose.
I will keep it in the series for now, let you decide how to handle it
thanks
--
Marc-André Lureau
