Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Thu, Aug 30, 2018 at 3:05 PM Markus Armbruster <arm...@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>>
>> > test_qom_set_without_value() is about a bug in infrastructure used by
>> > the QMP core, fixed in commit c489780203. We covered the bug in
>> > infrastructure unit tests (commit bce3035a44). I wrote that test
>> > earlier, to cover QMP level as well, the test could go into qmp-test.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> > ---
>> > tests/qmp-test.c | 18 ++++++++++++++++++
>> > 1 file changed, 18 insertions(+)
>> >
>> > diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> > index 4ae2245484..fdfe73b6d2 100644
>> > --- a/tests/qmp-test.c
>> > +++ b/tests/qmp-test.c
>> > @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>> > qtest_quit(qs);
>> > }
>> >
>> > +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);
>> > +}
>> > +
>> > int main(int argc, char *argv[])
>> > {
>> > g_test_init(&argc, &argv, NULL);
>> > @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
>> > qtest_add_func("qmp/protocol", test_qmp_protocol);
>> > qtest_add_func("qmp/oob", test_qmp_oob);
>> > qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>> > + qtest_add_func("qmp/qom-set-without-value",
>> > test_qom_set_without_value);
>> >
>> > return g_test_run();
>> > }
>>
>> What we're testing here is a missing 'any' parameter. The test should
>> be named accordingly. Perhaps:
>>
>> qtest_add_func("qmp/missing-any", test_missing_any);
>
> Eh, the inner visitor fix was about an 'any' parameter missing.
>
> But the test is more about about checking the behaviour of qom-set
> without 'value' argument. I would not rename it based on the internal
> bug that was fixed.
Missing arguments are caught by the QObject input visitor on behalf of
the generated command marshalling code. The command handler isn't
called then. Thus, qom-set doesn't get to behave! All the test really
tests is that
* the visitor behaves (redundant with test-qobject-input-visitor.c's
test_visitor_in_fail_struct_missing()),
* the generated marshalling code handles visitor failure (I guess that's
worth covering here),
* and the QMP core handles command failure (redundant with numerous
other tests, but covering it here once more won't hurt).
The test could just as well any other command with a mandatory argument
of type 'any', such as qom-set or object-add.
Same for arguments of inadmissible JSON type.
Having written all this, I now think the test should be named
"qmp/invalid-arg".
