Re: [PATCH V4 4/5] util: strList unit tests

[Steven Sistare]

On 2/21/2024 8:19 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  tests/unit/meson.build    |  1 +
>>  tests/unit/test-strList.c | 80 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 81 insertions(+)
>>  create mode 100644 tests/unit/test-strList.c
>>
>> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
>> index 69f9c05..113d12e 100644
>> --- a/tests/unit/meson.build
>> +++ b/tests/unit/meson.build
>> @@ -35,6 +35,7 @@ tests = {
>>    'test-rcu-simpleq': [],
>>    'test-rcu-tailq': [],
>>    'test-rcu-slist': [],
>> +  'test-strList': [],
>>    'test-qdist': [],
>>    'test-qht': [],
>>    'test-qtree': [],
>> diff --git a/tests/unit/test-strList.c b/tests/unit/test-strList.c
>> new file mode 100644
>> index 0000000..49a1cfd
>> --- /dev/null
>> +++ b/tests/unit/test-strList.c
>> @@ -0,0 +1,80 @@
>> +/*
>> + * Copyright (c) 2022 - 2024 Oracle and/or its affiliates.
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/strList.h"
>> +
>> +static strList *make_list(int length)
>> +{
>> +    strList *head = 0, *list, **prev = &head;
>> +
>> +    while (length--) {
>> +        list = *prev = g_new0(strList, 1);
>> +        list->value = g_strdup("aaa");
>> +        prev = &list->next;
>> +    }
>> +    return head;
>> +}
>> +
>> +static void test_length(void)
>> +{
>> +    strList *list;
>> +    int i;
>> +
>> +    for (i = 0; i < 5; i++) {
>> +        list = make_list(i);
>> +        g_assert_cmpint(i, ==, QAPI_LIST_LENGTH(list));
>> +        qapi_free_strList(list);
>> +    }
>> +}
>> +
>> +struct {
>> +    const char *string;
>> +    const char *delim;
>> +    const char *args[5];
>> +} list_data[] = {
>> +    { 0, ",", { 0 } },
>> +    { "", ",", { 0 } },
>> +    { "a", ",", { "a", 0 } },
>> +    { "a,b", ",", { "a", "b", 0 } },
>> +    { "a,b,c", ",", { "a", "b", "c", 0 } },
>> +    { "first last", " ", { "first", "last", 0 } },
>> +    { "a:", ":", { "a", "", 0 } },
>> +    { "a::b", ":", { "a", "", "b", 0 } },
>> +    { ":", ":", { "", "", 0 } },
>> +    { ":a", ":", { "", "a", 0 } },
>> +    { "::a", ":", { "", "", "a", 0 } },
>> +};
> 
> NULL instead of 0, please.

ok.

>> +
>> +static void test_strv(void)
>> +{
>> +    int i, j;
>> +    const char **expect;
>> +    strList *list;
>> +    GStrv args;
> 
> I'd prefer char **argv;

ok.

>> +
>> +    for (i = 0; i < ARRAY_SIZE(list_data); i++) {
>> +        expect = list_data[i].args;
>> +        list = strList_from_string(list_data[i].string, list_data[i].delim);
>> +        args = strv_from_strList(list);
>> +        qapi_free_strList(list);
>> +        for (j = 0; expect[j] && args[j]; j++) {
> 
> Loop stops when either array element is null.  That's wrong, we need to
> exhaust both arrays: || instead of &&.

|| is not safe.  After one array is exhausted, [j] will be out of bounds for
the other.  The g_assert_null calls guarantee the arrays are the same length
and all elements have been compared.

- Steve

>> +            g_assert_cmpstr(expect[j], ==, args[j]);
>> +        }
>> +        g_assert_null(expect[j]);
>> +        g_assert_null(args[j]);
>> +        g_strfreev(args);
>> +    }
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    g_test_init(&argc, &argv, NULL);
>> +    g_test_add_func("/test-string/length", test_length);
>> +    g_test_add_func("/test-string/strv", test_strv);
>> +    return g_test_run();
>> +}
>