Re: [PATCH v6 11/13] t/unit-tests: convert strvec tests to use clar

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Patrick

On 20/08/2024 15:02, Patrick Steinhardt wrote:
> Convert the strvec tests to use the new clar unit testing framework.
> This is a first test balloon that demonstrates how the testing infra for
> clar-based tests looks like.
> 
> The tests are part of the "t/unit-tests/bin/unit-tests" binary. When
> running that binary, it generates TAP output:

It would be interesting to see a comparison between the current 
framework and clar of the output from a failing test - the TAP output 
for passing tests is pretty much the same regardless of the framework used.

>      # ./t/unit-tests/bin/unit-tests
>      TAP version 13
>      # start of suite 1: strvec
>      ok 1 - strvec::init
> [...] 
> The binary also supports some parameters that allow us to run only a
> subset of unit tests or alter the output:
> 
>      $ ./t/unit-tests/bin/unit-tests -h
>      Usage: ./t/unit-tests/bin/unit-tests [options]
> 
>      Options:
>        -sname        Run only the suite with `name` (can go to individual test name)
>        -iname        Include the suite with `name`
>        -xname        Exclude the suite with `name`
>        -v            Increase verbosity (show suite names)

The output above seems to include the suite name - are we running the 
tests with '-v' from our Makefile?

>        -q            Only report tests that had an error

This option is incompatible with TAP output. As we force TAP output we 
should find a way to stop displaying this help.

>        -Q            Quit as soon as a test fails
>        -t            Display results in tap format

We force TAP output by adding '-t' to argv in main() so this line is not 
very helpful

>        -l            Print suite names
>        -r[filename]  Write summary file (to the optional filename)

> diff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c
> [..]
> +#define check_strvec(vec, ...) \
> +	do { \
> +		const char *expect[] = { __VA_ARGS__ }; \
> +		cl_assert(ARRAY_SIZE(expect) > 0); \

As there are a lot occurrences of ARRAY_SIZE(expect) it is probably 
worth adding

	size_t expect_len = ARRAY_SIZE(expect);

above.

> +		cl_assert_equal_p(expect[ARRAY_SIZE(expect) - 1], NULL); \
> +		cl_assert_equal_i((vec)->nr, ARRAY_SIZE(expect) - 1); \
> +		cl_assert((vec)->nr <= (vec)->alloc); \

The conversion here loses the values of nr and alloc which is a shame as 
they would be useful when debugging a test failure.

> +		for (size_t i = 0; i < ARRAY_SIZE(expect); i++) \
> +			cl_assert_equal_s((vec)->v[i], expect[i]); \

The original test also printed the array index of the failing check. As 
the elements of the test vectors all seem to be unique that is less of a 
worry than if we had tests with repeating elements.

> +	} while (0)
> +
> +void test_strvec__init(void)
> +{
> +	struct strvec vec = STRVEC_INIT;

If we're rewriting the tests perhaps we can take the opportunity to add 
a blank line to each one after the variable declarations in accordance 
with our coding guidelines.

It might be a good opportunity to show the set-up and tear-down 
facilities in clar as well instead of repeating the initialization in 
each test.

Best Wishes

Phillip

> +	cl_assert_equal_p(vec.v, empty_strvec);
> +	cl_assert_equal_i(vec.nr, 0);
> +	cl_assert_equal_i(vec.alloc, 0);
> +}
> +
> +void test_strvec__dynamic_init(void)
> +{
> +	struct strvec vec;
> +	strvec_init(&vec);
> +	cl_assert_equal_p(vec.v, empty_strvec);
> +	cl_assert_equal_i(vec.nr, 0);
> +	cl_assert_equal_i(vec.alloc, 0);
> +}
> +
> +void test_strvec__clear(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_push(&vec, "foo");
> +	strvec_clear(&vec);
> +	cl_assert_equal_p(vec.v, empty_strvec);
> +	cl_assert_equal_i(vec.nr, 0);
> +	cl_assert_equal_i(vec.alloc, 0);
> +}
> +
> +void test_strvec__push(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +
> +	strvec_push(&vec, "foo");
> +	check_strvec(&vec, "foo", NULL);
> +
> +	strvec_push(&vec, "bar");
> +	check_strvec(&vec, "foo", "bar", NULL);
> +
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__pushf(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushf(&vec, "foo: %d", 1);
> +	check_strvec(&vec, "foo: 1", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__pushl(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	check_strvec(&vec, "foo", "bar", "baz", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__pushv(void)
> +{
> +	const char *strings[] = {
> +		"foo", "bar", "baz", NULL,
> +	};
> +	struct strvec vec = STRVEC_INIT;
> +
> +	strvec_pushv(&vec, strings);
> +	check_strvec(&vec, "foo", "bar", "baz", NULL);
> +
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__replace_at_head(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	strvec_replace(&vec, 0, "replaced");
> +	check_strvec(&vec, "replaced", "bar", "baz", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__replace_at_tail(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	strvec_replace(&vec, 2, "replaced");
> +	check_strvec(&vec, "foo", "bar", "replaced", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__replace_in_between(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	strvec_replace(&vec, 1, "replaced");
> +	check_strvec(&vec, "foo", "replaced", "baz", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__replace_with_substring(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", NULL);
> +	strvec_replace(&vec, 0, vec.v[0] + 1);
> +	check_strvec(&vec, "oo", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__remove_at_head(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	strvec_remove(&vec, 0);
> +	check_strvec(&vec, "bar", "baz", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__remove_at_tail(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	strvec_remove(&vec, 2);
> +	check_strvec(&vec, "foo", "bar", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__remove_in_between(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	strvec_remove(&vec, 1);
> +	check_strvec(&vec, "foo", "baz", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__pop_empty_array(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pop(&vec);
> +	check_strvec(&vec, NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__pop_non_empty_array(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> +	strvec_pop(&vec);
> +	check_strvec(&vec, "foo", "bar", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__split_empty_string(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_split(&vec, "");
> +	check_strvec(&vec, NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__split_single_item(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_split(&vec, "foo");
> +	check_strvec(&vec, "foo", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__split_multiple_items(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_split(&vec, "foo bar baz");
> +	check_strvec(&vec, "foo", "bar", "baz", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__split_whitespace_only(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_split(&vec, " \t\n");
> +	check_strvec(&vec, NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__split_multiple_consecutive_whitespaces(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	strvec_split(&vec, "foo\n\t bar");
> +	check_strvec(&vec, "foo", "bar", NULL);
> +	strvec_clear(&vec);
> +}
> +
> +void test_strvec__detach(void)
> +{
> +	struct strvec vec = STRVEC_INIT;
> +	const char **detached;
> +
> +	strvec_push(&vec, "foo");
> +
> +	detached = strvec_detach(&vec);
> +	cl_assert_equal_s(detached[0], "foo");
> +	cl_assert_equal_p(detached[1], NULL);
> +
> +	cl_assert_equal_p(vec.v, empty_strvec);
> +	cl_assert_equal_i(vec.nr, 0);
> +	cl_assert_equal_i(vec.alloc, 0);
> +
> +	free((char *) detached[0]);
> +	free(detached);
> +}
> diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c
> deleted file mode 100644
> index c4bac8fc91b..00000000000
> --- a/t/unit-tests/t-strvec.c
> +++ /dev/null
> @@ -1,211 +0,0 @@
> -#include "test-lib.h"
> -#include "strbuf.h"
> -#include "strvec.h"
> -
> -#define check_strvec(vec, ...) \
> -	do { \
> -		const char *expect[] = { __VA_ARGS__ }; \
> -		if (check_uint(ARRAY_SIZE(expect), >, 0) && \
> -		    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \
> -		    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \
> -		    check_uint((vec)->nr, <=, (vec)->alloc)) { \
> -			for (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \
> -				if (!check_str((vec)->v[i], expect[i])) { \
> -					test_msg("      i: %"PRIuMAX, \
> -						 (uintmax_t)i); \
> -					break; \
> -				} \
> -			} \
> -		} \
> -	} while (0)
> -
> -int cmd_main(int argc, const char **argv)
> -{
> -	if_test ("static initialization") {
> -		struct strvec vec = STRVEC_INIT;
> -		check_pointer_eq(vec.v, empty_strvec);
> -		check_uint(vec.nr, ==, 0);
> -		check_uint(vec.alloc, ==, 0);
> -	}
> -
> -	if_test ("dynamic initialization") {
> -		struct strvec vec;
> -		strvec_init(&vec);
> -		check_pointer_eq(vec.v, empty_strvec);
> -		check_uint(vec.nr, ==, 0);
> -		check_uint(vec.alloc, ==, 0);
> -	}
> -
> -	if_test ("clear") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_push(&vec, "foo");
> -		strvec_clear(&vec);
> -		check_pointer_eq(vec.v, empty_strvec);
> -		check_uint(vec.nr, ==, 0);
> -		check_uint(vec.alloc, ==, 0);
> -	}
> -
> -	if_test ("push") {
> -		struct strvec vec = STRVEC_INIT;
> -
> -		strvec_push(&vec, "foo");
> -		check_strvec(&vec, "foo", NULL);
> -
> -		strvec_push(&vec, "bar");
> -		check_strvec(&vec, "foo", "bar", NULL);
> -
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("pushf") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushf(&vec, "foo: %d", 1);
> -		check_strvec(&vec, "foo: 1", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("pushl") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		check_strvec(&vec, "foo", "bar", "baz", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("pushv") {
> -		const char *strings[] = {
> -			"foo", "bar", "baz", NULL,
> -		};
> -		struct strvec vec = STRVEC_INIT;
> -
> -		strvec_pushv(&vec, strings);
> -		check_strvec(&vec, "foo", "bar", "baz", NULL);
> -
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("replace at head") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		strvec_replace(&vec, 0, "replaced");
> -		check_strvec(&vec, "replaced", "bar", "baz", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("replace at tail") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		strvec_replace(&vec, 2, "replaced");
> -		check_strvec(&vec, "foo", "bar", "replaced", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("replace in between") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		strvec_replace(&vec, 1, "replaced");
> -		check_strvec(&vec, "foo", "replaced", "baz", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("replace with substring") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", NULL);
> -		strvec_replace(&vec, 0, vec.v[0] + 1);
> -		check_strvec(&vec, "oo", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("remove at head") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		strvec_remove(&vec, 0);
> -		check_strvec(&vec, "bar", "baz", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("remove at tail") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		strvec_remove(&vec, 2);
> -		check_strvec(&vec, "foo", "bar", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("remove in between") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		strvec_remove(&vec, 1);
> -		check_strvec(&vec, "foo", "baz", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("pop with empty array") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pop(&vec);
> -		check_strvec(&vec, NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("pop with non-empty array") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_pushl(&vec, "foo", "bar", "baz", NULL);
> -		strvec_pop(&vec);
> -		check_strvec(&vec, "foo", "bar", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("split empty string") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_split(&vec, "");
> -		check_strvec(&vec, NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("split single item") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_split(&vec, "foo");
> -		check_strvec(&vec, "foo", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("split multiple items") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_split(&vec, "foo bar baz");
> -		check_strvec(&vec, "foo", "bar", "baz", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("split whitespace only") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_split(&vec, " \t\n");
> -		check_strvec(&vec, NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("split multiple consecutive whitespaces") {
> -		struct strvec vec = STRVEC_INIT;
> -		strvec_split(&vec, "foo\n\t bar");
> -		check_strvec(&vec, "foo", "bar", NULL);
> -		strvec_clear(&vec);
> -	}
> -
> -	if_test ("detach") {
> -		struct strvec vec = STRVEC_INIT;
> -		const char **detached;
> -
> -		strvec_push(&vec, "foo");
> -
> -		detached = strvec_detach(&vec);
> -		check_str(detached[0], "foo");
> -		check_pointer_eq(detached[1], NULL);
> -
> -		check_pointer_eq(vec.v, empty_strvec);
> -		check_uint(vec.nr, ==, 0);
> -		check_uint(vec.alloc, ==, 0);
> -
> -		free((char *) detached[0]);
> -		free(detached);
> -	}
> -
> -	return test_done();
> -}