Re: [Qemu-devel] [PATCH 03/21] qmp-test: New, covering basic QMP protocol

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/23/2017 03:44 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  tests/Makefile.include |   5 +-
>>  tests/libqtest.c       |  17 ++++--
>>  tests/libqtest.h       |   8 +++
>>  tests/qmp-test.c       | 139 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 163 insertions(+), 6 deletions(-)
>>  create mode 100644 tests/qmp-test.c
>> 
>
>> +++ b/tests/libqtest.h
>> @@ -32,6 +32,14 @@ extern QTestState *global_qtest;
>>  QTestState *qtest_init(const char *extra_args);
>>  
>>  /**
>> + * qtest_init:
>
> Wrong name (too much copy-and-paste)

One of the several reasons why I detest this comment style.  I'll fix
it, of course.

>> + * @extra_args: other arguments to pass to QEMU.
>> + *
>> + * Returns: #QTestState instance.
>> + */
>> +QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
>> +
>> +/**
>>   * qtest_quit:
>>   * @s: #QTestState instance to operate on.
>>   *
>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> new file mode 100644
>> index 0000000..405e49e
>> --- /dev/null
>> +++ b/tests/qmp-test.c
>
>> +
>> +static void test_malformed(void)
>> +{
>> +    QDict *resp;
>> +
>> +    /* Not even a dictionary */
>> +    resp = qmp("null");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +    QDECREF(resp);
>> +
>
> Shall we test an array, integer, and/or boolean literal as well?

Not necessary for code coverage.  Wouldn't hurt, of course.

>> +    /* No "execute" key */
>> +    resp = qmp("{}");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +    QDECREF(resp);
>> +
>> +    /* "execute" isn't a string */
>> +    resp = qmp("{ 'execute': true }");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +    QDECREF(resp);
>> +
>> +    /* "arguments" isn't a dictionary */
>> +    resp = qmp("{ 'execute': 'no-such-cmd', 'arguments': [] }");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +    QDECREF(resp);
>> +
>> +    /* extra key */
>> +    resp = qmp("{ 'execute': 'no-such-cmd', 'extra': true }");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +    QDECREF(resp);
>
> Worth testing "{ 'arguments': {} }"?

Likewise.

>> +}
>> +
>> +static void test_qmp_protocol(void)
>> +{
>> +    QDict *resp, *q, *ret;
>> +    QList *capabilities;
>> +
>> +    global_qtest = qtest_init_without_qmp_handshake(common_args);
>> +
>> +    /* Test greeting */
>> +    resp = qmp_receive();
>> +    q = qdict_get_qdict(resp, "QMP");
>> +    g_assert(q);
>> +    test_version(qdict_get(q, "version"));
>> +    capabilities = qdict_get_qlist(q, "capabilities");
>> +    g_assert(capabilities && qlist_empty(capabilities));
>> +    QDECREF(resp);
>> +
>> +    /* Test valid command before handshake */
>> +    resp = qmp("{ 'execute': 'query-version' }");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
>> +    QDECREF(resp);
>> +
>> +    /* Test malformed commands before handshake */
>> +    test_malformed();
>> +
>> +    /* Test handshake */
>> +    resp = qmp("{ 'execute': 'qmp_capabilities' }");
>> +    ret = qdict_get_qdict(resp, "return");
>> +    g_assert(ret && !qdict_size(ret));
>> +    QDECREF(resp);
>> +
>> +    /* Test repeated handshake */
>> +    resp = qmp("{ 'execute': 'qmp_capabilities' }");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
>> +    QDECREF(resp);
>> +
>> +    /* Test valid command */
>> +    resp = qmp("{ 'execute': 'query-version' }");
>> +    test_version(qdict_get(resp, "return"));
>> +    QDECREF(resp);
>> +
>> +    /* Test malformed commands */
>> +    test_malformed();
>> +
>> +    /* Test 'id' */
>> +    resp = qmp("{ 'execute': 'query-name', 'id': 'cookie#1' }");
>> +    ret = qdict_get_qdict(resp, "return");
>> +    g_assert(ret);
>> +    g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, "cookie#1");
>> +    QDECREF(resp);
>> +
>> +    /* Test command failure with 'id' */
>> +    resp = qmp("{ 'execute': 'human-monitor-command', 'id': 2 }");
>> +    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +    g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
>> +    QDECREF(resp);
>
> id can be _any JSON_ (at one point, QMP crashed if id included a null
> literal); so maybe we should have more id tests such as "'id': null,",
> "'id': [ { }, true ]"
>
> But any tests at all is better than our previous state of none, so even
> if you don't add tests, but just fix the typo:
>
> Reviewed-by: Eric Blake <address@hidden>

I'm in a bit of a time squeeze, so I'll take your offer to just fix the
typo for now.

Thanks!