Re: [PATCH 2/5] u-string-list: move "test_split" into "u-string-list.c"

> shejialuo <shejialuo@gmail.com> 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@gmail.com>
> > ---
> 
> 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?"
> 

> 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.
> 

> > +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 ...
> 

> > +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.
> 

> > +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.