From: phillip.wood123@gmail.com
Date: Mon, 03 Feb 2025 14:56:57 GMT
Subject: Re: [PATCH v2 1/4] t/unit-tests: convert hashmap test to use clar test framework
Message-ID: <288fbcf3-f6c1-46e4-8124-f4e566abe8af@gmail.com>
In-Reply-To: <Z6Bwr6hM54nu8nSS@pks.im>

Hi Patrick

On 03/02/2025 07:30, Patrick Steinhardt wrote:
> On Sun, Feb 02, 2025 at 11:09:25AM +0000, phillip.wood123@gmail.com wrote:
>> On 31/01/2025 22:14, Seyi Kuforiji wrote:
> 
>> diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
>> index d54e4553674..16f86c952f7 100644
>> --- a/t/unit-tests/clar/clar.c
>> +++ b/t/unit-tests/clar/clar.c
>> @@ -754,7 +754,12 @@ void clar__assert_equal(
>>                                   p_snprintf(buf, sizeof(buf), "'%s' != '%s' (at byte %d)",
>>                                           s1, s2, pos);
>>                           } else {
>> -                                p_snprintf(buf, sizeof(buf), "'%s' != '%s'", s1, s2);
>> +                                const char *q1 = s1 ? "'" : "";
>> +                                const char *q2 = s2 ? "'" : "";
>> +                                s1 = s1 ? s1 : "NULL";
>> +                                s2 = s2 ? s2 : "NULL";
>> +                                p_snprintf(buf, sizeof(buf), "%s%s%s != %s%s%s",
>> +                                           q1, s1, q1, q2, s2, q2);
>>                           }
>>                   }
>>           }
> 
> Would you mind creating an upstream pull request with these changes? I'm
> happy to review, and then we can update our embedded version of clar.

I've opened a PR at https://github.com/clar-test/clar/pull/114

>>>    	for (size_t i = 0; i < ARRAY_SIZE(query); i++) {
>>>    		entry = get_test_entry(map, query[i][0], ignore_case);
>>> -		if (check(entry != NULL))
>>> -			check_str(get_value(entry), query[i][1]);
>>> -		else
>>> -			test_msg("query key: %s", query[i][0]);
>>
>> It is a shame that we're removing all of the helpful debugging messages
>> from this test. It would be much nicer if we could keep them by using an
>> if statement and cl_failf() as we do in u-ctype.c
> 
> I honestly think that the debug messages don't add much and only add to
> the noise. You shouldn't ever see them, and if you do something is
> broken and you'll likely end up pulling out the debugger anyway. So I'm
> more in the camp of writing unit tests in a concise way rather than the
> needlessly-verbose style we previously had.

If I'm firing up the debugger I'd rather as much detail as I can about 
what went wrong so I can see where to set my breakpoints. Otherwise I 
need to waste time repeating the test to find out exactly what went 
wrong before I can make any progress. My experience with debugging our 
integration tests is that those tests that take care to print helpful 
diagnostic messages when they fail are a lot easier to debug than those 
that don't.

Best Wishes

Phillip