Eric Blake <ebl...@redhat.com> writes:
> On 02/23/2017 03:44 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> 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 <ebl...@redhat.com>
I'm in a bit of a time squeeze, so I'll take your offer to just fix the
typo for now.
Thanks!
