On Tue, Apr 22, 2025 at 02:27:02PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@xxxxxxxxx> writes:
>
> > We rely on "test-tool string-list" command to test the functionality of
> > the "string-list". However, as we have introduced clar test framework,
> > we'd better move the shell script into C program to improve speed and
> > readability.
> >
> > Create a new file "u-string-list.c" under "t/unit-tests", then update
> > the Makefile and "meson.build" to build the file. And let's first move
> > "test_split" into unit test and gradually convert the shell script into
> > C program.
> >
> > In order to create `string_list` easily by simply specifying strings in
> > the function call, create "t_vcreate_string_list_dup" and
> > "t_create_string_list_dup" functions to do above.
> >
> > Then port the shell script tests to C program and remove unused
> > "test-tool" code and tests.
> >
> > Signed-off-by: shejialuo <shejialuo@xxxxxxxxx>
> > ---
>
> This is the most interesting in the u-string-list patches, as it
> adds not just a moved test but adds supporting functions that are
> shared with test functions added in later steps.
>
> > diff --git a/t/unit-tests/u-string-list.c b/t/unit-tests/u-string-list.c
> > new file mode 100644
> > index 0000000000..0c148684ea
> > --- /dev/null
> > +++ b/t/unit-tests/u-string-list.c
> > @@ -0,0 +1,86 @@
> > +#include "unit-test.h"
> > +#include "string-list.h"
> > +
> > +static void t_check_string_list(struct string_list *list,
> > + struct string_list *expected_strings)
> > +{
> > + size_t expect_len = expected_strings->nr;
> > + cl_assert_equal_i(list->nr, expect_len);
> > + cl_assert(list->nr <= list->alloc);
> > + for (size_t i = 0; i < expect_len; i++)
> > + cl_assert_equal_s(list->items[i].string,
> > + expected_strings->items[i].string);
> > +}
>
> Perhaps call it "string_list_equal()" or something? "check" is a
> convenient name that can mean different kind of validation that is
> not limited to "is the actual answer identical to the expected one?"
>
Good idea.
> Wouldn't it be cleaner to read if you wrote it without an extra
> variable expect_len? The compiler would notice repeated reference
> of expected_strings->nr and optimize them away anyway, I would
> imagine.
>
Yeah, the compiler would definitely optimize this. Will update in the
next version.
> > +static void t_string_list_clear(struct string_list *list, int free_util)
> > +{
> > + string_list_clear(list, free_util);
> > + cl_assert_equal_p(list->items, NULL);
> > + cl_assert_equal_i(list->nr, 0);
> > + cl_assert_equal_i(list->alloc, 0);
> > +}
>
> Validating the result of clearing a list may be a good thing to do
> at least once in the test suite, but this is called from many places
> in other tests. Conceptually it feels kludgy to call this from
> other places where they should all just call string_list_clear(),
> like ...
>
I agree that we should not call `t_string_list_clear` in many places.
It's overkill.
> > +static void t_vcreate_string_list_dup(struct string_list *list,
> > + int free_util, va_list ap)
> > +{
> > + const char *arg;
> > +
> > + cl_assert(list->strdup_strings);
> > +
> > + t_string_list_clear(list, free_util);
>
> ... this place.
>
> > + while ((arg = va_arg(ap, const char *)))
> > + string_list_append(list, arg);
> > +}
>
> To put it differently, you could be calling t_string_list_append()
> in this loop, which would
>
> - remember list->nr
> - call string_list_append()
> - cl_assert_equal() to ensure that list->nr is one larger than
> the value we remembered upon entry to the function.
>
> which is not wrong per-se, but hopefully you'd agree that it is
> overkill. t_stirng_list_clear() is overkill in the same way.
>
You're right. I will improve this in the next version.
> > +void test_string_list__split(void)
> > +{
> > + struct string_list expected_strings = STRING_LIST_INIT_DUP;
> > +
> > + t_create_string_list_dup(&expected_strings, 0, "foo", "bar", "baz", NULL);
> > +...
> > + t_create_string_list_dup(&expected_strings, 0, "", "", NULL);
> > + t_string_list_split(":", ':', -1, &expected_strings);
> > +
> > + t_string_list_clear(&expected_strings, 0);
> > +}
>
> This is a good place to call t_string_list_clear(), just once in
> this script. All other callers are conceptually simpler to call
> string_list_clear(), as the point at their callsites is to clear
> after themselves, not about testing string_list_clear() works
> correctly.
>
> Thanks.
Thanks,
Jialuo
>
