Thomas Huth <th...@redhat.com> writes:
> On 07/30/2018 08:32 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> writes:
>>
>>> On 07/27/2018 11:46 AM, Thomas Huth wrote:
>>>> On 07/27/2018 05:13 PM, Markus Armbruster wrote:
>>>>> qtest_qmp_discard_response(...) is shorthand for
>>>>> qobject_unref(qtest_qmp(...), except it's not actually shorter.
>>>>
>>>> But the latter is IMHO harder to read.
>>
>> Doing things sloppily looks a bit uglier now. That's a feature.
>>
>>> Maybe, but then it lends itself well to:
>>>
>>> QObject *rsp = qtest_qmp(...);
>>> qobject_unref(rsp);
>>>
>>> which is where you do insert tests for valid responses.
>>>
>>>> And it might be shorter in the compiled binary (one function call vs. two).
>>
>> I'd be quite sympathetic to this argument...
>>
>>> The size of the test binaries is not our biggest concern.
>>
>> ... outside tests/.
>>
>>>>> Moreover, the presence of these functions encourage sloppy testing.
>>>>
>>>> Shouldn't we then rather fix the tests to check for valid responses
>>>> instead of replacing this function with harder-to-read code?
>>
>> I'd welcome such patches, but this series is already pretty long.
>
> Then maybe rather drop this patch from this series, and fix the issues
> in a separate series instead?
Do you insist?
I fail to see how changing
qmp_discard_response("{ 'execute': 'system_reset' }");
to
qobject_unref(qmp("{ 'execute': 'system_reset' }"));
is so awful it would justify demanding I pause my work on libqtest to
first figure out which parts of ignored responses are worth checking,
then code up the checks.
Would you accept
rsp = qmp("{ 'execute': 'system_reset' }"));
qobject_unref(rsp);
?
If none of the above is acceptable to you, then I'll push the crap that
needs to go from libqtest into the crap-using tests, like this:
/* TODO actually test the results and get rid of this */
#define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__));
