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

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

Hi Patrick

On 20/08/2024 15:02, Patrick Steinhardt wrote:
> Convert the ctype tests to use the new clar unit testing framework.
> Introduce a new function `cl_failf()` 

This is a nice addition which somewhat mitigates the lack of an 
equivalent to test_msg() that adds addition context messages to test 
failures.

> that allows us to print a
> formatted error message, which we can use to point out which of the
> characters was classified incorrectly. This results in output like this
> on failure:
> 
>      # start of suite 1: ctype
>      ok 1 - ctype::isspace
>      not ok 2 - ctype::isdigit
>          ---
>          reason: |
>            Test failed.

"Test failed." is not the reason for the test failure

>            0x61 is classified incorrectly
>          at:
>            file: 't/unit-tests/ctype.c'
>            line: 38
>            function: 'test_ctype__isdigit'
>          ---

This is rather verbose compared to the current framework

# check "isdigit(i) == !!memchr("123456789", i, len)" failed at 
t/unit-tests/t-ctype.c:36
#    left: 1
#   right: 0
#       i: 0x30
not ok 2 - isdigit works

The current tests also shows which characters are expected to return 
true and distinguishes between the two possible failure modes which are 
(a) misclassification and (b) returning a non-zero integer other than 
'1' as "true". The new test output does not allow the person running the 
test to distinguish between these two failure modes.

> diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h
> index 66ec2387cc6..4c461defe16 100644
> --- a/t/unit-tests/unit-test.h
> +++ b/t/unit-tests/unit-test.h
> @@ -1,3 +1,10 @@
>   #include "git-compat-util.h"
>   #include "clar/clar.h"
>   #include "clar-decls.h"
> +#include "strbuf.h"
> +
> +#define cl_failf(fmt, ...) do { \
> +	char *desc = xstrfmt(fmt, __VA_ARGS__); \

In our current framework we avoid relying on the strbuf api and 
functions like xstrfmt() that use it as we want to be able to test 
strbuf.c with the framework. We'd be better with a macro wrapping a new 
function that uses a stack allocated buffer and p_snprintf() like 
clar__assert_equal(). That would allow us to upstream this change as well.

> +	clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \
> +	free(desc); \

This is leaked on failure due to the use of longjmp.

Best Wishes

Phillip

> +} while (0)