Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c

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

On 27/12/2023 11:57, René Scharfe wrote:
> Am 27.12.23 um 11:57 schrieb Christian Couder:
>> On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Achu Luma <ach.lumap@gmail.com> writes:
>>
>>>> +/* Macro to test a character type */
>>>> +#define TEST_CTYPE_FUNC(func, string)                        \
>>>> +static void test_ctype_##func(void)                          \
>>>> +{                                                            \
>>>> +     int i;                                                  \
>>>> +     for (i = 0; i < 256; i++)                               \
>>>> +             check_int(func(i), ==, is_in(string, i));       \
>>>> +}
>>>
>>> Now, we let check_int() to do the checking for each and every byte
>>> value for the class.  check_int() uses different reporting and shows
>>> the problematic value in a way that is more verbose and at the same
>>> time is a less specific and harder to understand:
>>>
>>>                  test_msg("   left: %"PRIdMAX, a);
>>>                  test_msg("  right: %"PRIdMAX, b);
>>>
>>> But that is probably the price to pay to use a more generic
>>> framework, I guess.
>>
>> I have added Phillip and Josh in Cc: as they might have ideas about this.
> 
> You can write custom messages for custom tests using test_assert().

Another possibility is to do

	for (int i = 0; i < 256; i++) {
		if (!check_int(func(i), ==, is_in(string, i))
			test_msg("       i: %02x", i);
	}

To print the character code as well as the actual and expected return 
values of check_int(). The funny spacing is intended to keep the output 
aligned. I did wonder if we should be using

	check(func(i) == is_in(string, i))

instead of check_int() but I think it is useful to have the return value 
printed on error in case we start returning "53" instead of "1" for 
"true" [1]. With the extra test_msg() above we can now see if the test 
fails because of a mis-categorization or because func() returned a 
different non-zero value when we were expecting "1".

>> Also it might not be a big issue here, but when the new unit test
>> framework was proposed, I commented on the fact that "left" and
>> "right" were perhaps a bit less explicit than "actual" and "expected".

If people are worried about this then it would be possible to change the 
check_xxx() macros pass the stringified relational operator into the 
various check_xxx_loc() functions and then print "expected" and "actual" 
when the operator is "==" and "left" and "right" otherwise.

Best Wishes

Phillip

[1] As an aside I wonder if the ctype functions would make good test 
balloons for using _Bool by changing sane_istest() to be

#define sane_istest(x,mask) ((bool)(sane_ctype[(unsigned char)(x)] & 
(mask)))

so that we check casting to _Bool coerces non-zero values to "1"