Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qga.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 30c9643257..4edd35535d 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -227,6 +227,22 @@ static void test_qga_ping(gconstpointer fix)
     qobject_unref(ret);
 }
 
+static void test_qga_invalid_id(gconstpointer fix)
+{
+    /* FIXME "id" is ignored; it should be rejected */
+    const TestFixture *fixture = fix;
+    QDict *ret, *val;
+
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+
+    val = qdict_get_qdict(ret, "return");
+    g_assert(!qdict_haskey(val, "id"));
+
+    qobject_unref(ret);
+}
+
 static void test_qga_invalid_args(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
@@ -934,6 +950,7 @@ int main(int argc, char **argv)
     g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops);
     g_test_add_data_func("/qga/file-write-read", &fix, test_qga_file_write_read);
     g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time);
+    g_test_add_data_func("/qga/invalid-id", &fix, test_qga_invalid_id);
     g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd);
     g_test_add_data_func("/qga/invalid-args", &fix, test_qga_invalid_args);
     g_test_add_data_func("/qga/fsfreeze-status", &fix,
-- 
2.17.1

On 07/02/2018 11:21 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qga.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 30c9643257..4edd35535d 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -227,6 +227,22 @@ static void test_qga_ping(gconstpointer fix)
>       qobject_unref(ret);
>   }
>   
> +static void test_qga_invalid_id(gconstpointer fix)
> +{
> +    /* FIXME "id" is ignored; it should be rejected */

I might have written:

/* FIXME "id" is ignored; it should either be repeated in the reply, or 
rejected on input */

> +    const TestFixture *fixture = fix;
> +    QDict *ret, *val;
> +
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
> +    g_assert_nonnull(ret);
> +    qmp_assert_no_error(ret);
> +
> +    val = qdict_get_qdict(ret, "return");
> +    g_assert(!qdict_haskey(val, "id"));

This is documenting what currently happens, along with the FIXME showing 
that it is not ideal, so whether or not you improve the comment:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 07/02/2018 11:21 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/test-qga.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/tests/test-qga.c b/tests/test-qga.c
>> index 30c9643257..4edd35535d 100644
>> --- a/tests/test-qga.c
>> +++ b/tests/test-qga.c
>> @@ -227,6 +227,22 @@ static void test_qga_ping(gconstpointer fix)
>>       qobject_unref(ret);
>>   }
>>   +static void test_qga_invalid_id(gconstpointer fix)
>> +{
>> +    /* FIXME "id" is ignored; it should be rejected */
>
> I might have written:
>
> /* FIXME "id" is ignored; it should either be repeated in the reply,
> or rejected on input */

Sold.

>> +    const TestFixture *fixture = fix;
>> +    QDict *ret, *val;
>> +
>> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-ping', 'id': 1}");
>> +    g_assert_nonnull(ret);
>> +    qmp_assert_no_error(ret);
>> +
>> +    val = qdict_get_qdict(ret, "return");
>> +    g_assert(!qdict_haskey(val, "id"));
>
> This is documenting what currently happens, along with the FIXME
> showing that it is not ideal, so whether or not you improve the
> comment:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!