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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 28, 2024, 13:16 UTC
Message-ID: <435e3840-ee46-4ca3-a525-747848da9930@gmail.com>
In-Reply-To: <1710e9f9ff75cee76be6d5c1311344c6564e3801.1724159966.git.ps@pks.im>

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(

Show 8 quoted lines

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

Show 27 quoted lines

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

Show 26 quoted lines

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