Re: [Qemu-devel] [PATCH] qdev-monitor-test: simplify using g_assert_cmpstr()

[Andreas Färber]

Am 18.12.2013 18:11, schrieb Markus Armbruster:
> Stefan Hajnoczi <address@hidden> writes:
> 
>> Use g_assert_cmpstr() instead of combining g_assert() and strcmp(3).
>> This simplifies the code since we no longer have to play games to
>> distinguish NULL from "" using "(null)".
>>
>> gcc extension haters will also be happy that ?: was dropped.
>>
>> Suggested-by: Markus Armbruster <address@hidden>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  tests/qdev-monitor-test.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qdev-monitor-test.c b/tests/qdev-monitor-test.c
>> index ba7f9cc..eefaab8 100644
>> --- a/tests/qdev-monitor-test.c
>> +++ b/tests/qdev-monitor-test.c
>> @@ -32,8 +32,9 @@ static void test_device_add(void)
>>                     "}}");
>>      g_assert(response);
>>      error = qdict_get_qdict(response, "error");
>> -    g_assert(!strcmp(qdict_get_try_str(error, "desc") ?: "",
>> -                     "Device needs media, but drive is empty"));
>> +    g_assert_cmpstr(qdict_get_try_str(error, "desc"),
>> +                    ==,
>> +                    "Device needs media, but drive is empty");
>>      QDECREF(response);
>>  
>>      /* Delete the drive */
> 
> Outside this patch's scope, but here goes anyway: why do we test the
> value of member desc?  Isn't that awfully fragile?
> 
> It broke once already, in Andreas's commit 75884af "virtio-blk: Convert
> to QOM realize".  Andreas, do you remember why you tossed the class ==
> GenericError check instead of the desc check?

/me points to bonzini, who rebased it on my behalf. :)

I do agree that testing error descriptions is fragile in that people may
not think of running make check on a trivial textual change, e.g. typo fix.

Andreas

> 
>> @@ -42,7 +43,7 @@ static void test_device_add(void)
>>                     "   \"command-line\": \"drive_del drive0\""
>>                     "}}");
>>      g_assert(response);
>> -    g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "(null)", 
>> ""));
>> +    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "");
>>      QDECREF(response);
>>  
>>      /* Try to re-add the drive.  This fails with duplicate IDs if a leaked
>> @@ -53,8 +54,7 @@ static void test_device_add(void)
>>                     "   \"command-line\": \"drive_add pci-addr=auto 
>> if=none,id=drive0\""
>>                     "}}");
>>      g_assert(response);
>> -    g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "",
>> -                     "OK\r\n"));
>> +    g_assert_cmpstr(qdict_get_try_str(response, "return"), ==, "OK\r\n");
>>      QDECREF(response);
>>  
>>      qtest_end();
> 
> Reviewed-by: Markus Armbruster <address@hidden>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg