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

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

[Stefan Hajnoczi]

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 <armbru@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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 */
@@ -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();
-- 
1.8.4.2

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> 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 <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  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?

> @@ -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 <armbru@redhat.com>

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 <stefanha@redhat.com> 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 <armbru@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  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 <armbru@redhat.com>
> 


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

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

[Andreas Färber]

Am 18.12.2013 17:34, schrieb Stefan Hajnoczi:
> Use g_assert_cmpstr() instead of combining g_assert() and strcmp(3).

I vaguely remember that some such handy function was introduced only
after the minimum GLib version we require. Did you check on that?
But IIRC we already have higher requirements for qtest that for the
non-check code so it might not really matter...

Andreas

> 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 <armbru@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  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 */
> @@ -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();
> 


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

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

[Stefan Hajnoczi]

On Wed, Dec 18, 2013 at 07:31:48PM +0100, Andreas Färber wrote:
> Am 18.12.2013 17:34, schrieb Stefan Hajnoczi:
> > Use g_assert_cmpstr() instead of combining g_assert() and strcmp(3).
> 
> I vaguely remember that some such handy function was introduced only
> after the minimum GLib version we require. Did you check on that?
> But IIRC we already have higher requirements for qtest that for the
> non-check code so it might not really matter...

Yes, before using it I did git grep g_assert_cmpstr.  It's already used
in many other test cases like tests/check-qjson.c.

Stefan