Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Thu, Aug 30, 2018 at 3:05 PM Markus Armbruster <address@hidden> wrote:
>>
>> Marc-André Lureau <address@hidden> 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 <address@hidden>
>> > ---
>> >  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".