Re: [PATCH v6 02/13] t: import the clar unit testing framework

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

Hi Patrick

On 20/08/2024 15:02, Patrick Steinhardt wrote:
> Import the clar unit testing framework at commit 1516124 (Merge pull
> request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework
> will be wired up in subsequent commits.

I would be good to explain why we're doing this so we have a record in 
the project history. A summary of the advantages and disadvantages of 
using clar vs our current unit test framework would be helpful as well.

 > diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
 > [...]
 > +static void
 > +clar_parse_args(int argc, char **argv)
 > +{
 > [...]
> +		case 'q':
> +			_clar.report_errors_only = 1;
> +			break;

I think this option is incompatible with TAP output as when TAP output 
is selected the first error is printed as part of clar_print_ontest() 
and clar_print_errors() is a no-op. We should error out if it is given 
with '-t'
  > +void clar__fail(
> +	const char *file,
> +	const char *function,
> +	size_t line,
> +	const char *error_msg,
> +	const char *description,
> +	int should_abort)
> +{
> +	struct clar_error *error = calloc(1, sizeof(struct clar_error));

clar seems to take an inconsistent approach to memory allocation errors. 
Here "error" is de-referenced without checking if it is NULL and yet in 
other places there are checks for NULL after calling calloc() or strdup().

> +void clar__assert_equal(
> +	const char *file,
> +	const char *function,
> +	size_t line,
> +	const char *err,
> +	int should_abort,
> +	const char *fmt,
> +	...)
> +{
> +	va_list args;
> +	char buf[4096];
> +	int is_equal = 1;
> +
> +	va_start(args, fmt);
> +
> +	if (!strcmp("%s", fmt)) {
> +		const char *s1 = va_arg(args, const char *);
> +		const char *s2 = va_arg(args, const char *);
> +		is_equal = (!s1 || !s2) ? (s1 == s2) : !strcmp(s1, s2);
> +
> +		if (!is_equal) {
> +			if (s1 && s2) {
> +				int pos;
> +				for (pos = 0; s1[pos] == s2[pos] && s1[pos] && s2[pos]; ++pos)
> +					/* find differing byte offset */;
> +				p_snprintf(buf, sizeof(buf), "'%s' != '%s' (at byte %d)",
> +					s1, s2, pos);

s1 and s2 are passed to snprintf without checking if they are NULL. As 
'snprintf(buf, sizeof(buf), "%s", NULL)' is undefined we should check 
for NULL here. The use of variadic arguments means there is no type 
checking so if one is checking the contents of an strbuf and writes

     cl_assert_equal_s(buf, "expect");

instead of

     cl_assert_equal_s(buf->buf, "expect");

the compiler does not warn you. This is a regression compared to 
check_str(). We can address this by having cl_assert_equal_s() wrap a 
function that takes two strings. Another regression is that if the 
string contains control characters they are printed verbatim.

> diff --git a/t/unit-tests/clar/clar.h b/t/unit-tests/clar/clar.h
 > [...]
> +#define cl_assert_equal_i(i1,i2) clar__assert_equal(__FILE__,__func__,__LINE__,#i1 " != " #i2, 1, "%d", (int)(i1), (int)(i2))

The (int) casts here mean that we'll silently truncate longer arguments 
and suppress any compiler warnings about comparing incompatible types.

 > diff --git a/t/unit-tests/clar/clar/print.h 
b/t/unit-tests/clar/clar/print.h
 > [...]
> +static void clar_print_tap_ontest(const char *suite_name, const char *test_name, int test_number, enum cl_test_status status)
> +{
> +	const struct clar_error *error = _clar.last_report->errors;
> +
> +	(void)test_name;
> +	(void)test_number;
> +
> +	switch(status) {
> +	case CL_TEST_OK:
> +		printf("ok %d - %s::%s\n", test_number, suite_name, test_name);
> +		break;
> +	case CL_TEST_FAILURE:
> +		printf("not ok %d - %s::%s\n", test_number, suite_name, test_name);
> +
> +		printf("    ---\n");
> +		printf("    reason: |\n");
> +		printf("      %s\n", error->error_msg);
> +
> +		if (error->description)
> +			printf("      %s\n", error->description);
> +
> +		printf("    at:\n");
> +		printf("      file: '"); print_escaped(error->file); printf("'\n");
> +		printf("      line: %" PRIuZ "\n", error->line_number);
> +		printf("      function: '%s'\n", error->function);
> +		printf("    ---\n");

If a test calls cl_warning() only the message from the first call to 
that function is printed and any message from later calls to 
cl_warning() or clar__fail() is suppressed when TAP output is selected.

Best Wishes

Phillip