Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 2018-08-31 15:24, Marc-André Lureau wrote:
>> Hi
>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> On 2018-08-31 14:04, Markus Armbruster wrote:
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> 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.lureau@redhat.com>
>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  tests/qmp-test.c | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>>>>> index b347228..2b923f0 100644
>>>>> --- a/tests/qmp-test.c
>>>>> +++ b/tests/qmp-test.c
>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
>>>>>      qtest_quit(qs);
>>>>>  }
>>>>>
>>>>> +static void test_qom_set_without_value(void)
>>>>> +{
>>>>> +    QTestState *qts;
>>>>> +    QDict *resp;
>>>>> +
>>>>> +    qts = qtest_init(common_args);
>>>>> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>>>>> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
>>>>> +    g_assert_nonnull(resp);
>>>>> +    qmp_assert_error_class(resp, "GenericError");
>>>>> +    qtest_quit(qts);
>>>>> +}
>>>>> +
>>>>>  int main(int argc, char *argv[])
>>>>>  {
>>>>>      g_test_init(&argc, &argv, NULL);
>>>>> @@ -328,6 +341,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();
>>>>>  }
>>>>
>>>> I'm afraid you missed my objection to naming:
>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html
>>>
>>> Sorry about that, I was not on CC: for that series. I used the patches
>>> from v5 where Marc-André put me on CC:.
>>>
>>>> If you could work that into PULL v2, I'd be obliged.  If not, I'll have
>>>> to address it in a follow-up patch.
>>>
>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be
>>> away? ... so we've got plenty of time to sort this out anyway.
>>> Marc-André, could you send a new version of the patch?
>> 
>> Tbh, I don't care so much about the naming of the test, but (for once)
>> I respectfully don't think Markus suggestion is better.
>> 
>> The function checks "qom-set" without 'value' argument:
>> "qom-set-without-value", no brainer..

Nope, that's not what it tests.  It tests the visitor, the marshalling
code generator, and the QMP core handle a certain kind of invalid
arguments correctly.  It does not test qom-set.  I explained all that
already.

>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does.

I can accept "missing-any" or "missing-any-arg".  I object to any name
involving qom-set, because the test is not about qom-set at all.

If it was, then putting it in qmp-test.c would be wrong.

> Ok, then let's keep it this way. As I said, IMHO the current naming is
> not really bad, and I also don't have any suggestions for a perfect name
> right now.

We don't need a perfect name.  We need one that's not actively
misleading.