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

[Junio C Hamano <gitster@pobox.com>]

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.