Re: [PATCH v2 1/4] t/unit-tests: convert hashmap test to use clar test framework

[Patrick Steinhardt <ps@pks.im>]

On Sun, Feb 02, 2025 at 11:09:25AM +0000, phillip.wood123@gmail.com wrote:
> On 31/01/2025 22:14, Seyi Kuforiji wrote:
> > -	if (check(entry != NULL))
> > -		check_str(get_value(entry), "value3");
> > +	cl_assert(entry != NULL);
> > +	cl_assert_equal_s(get_value(entry), "value3");
> 
> Unfortunately cl_assert_equal_s() is not equivalent to check_str()
> because it does not handle NULL correctly. I think it would be very
> helpful to fix that upstream. The diff below shows a possible solution
> which avoids any ambiguity as to which pointer is NULL by only quoting
> non-NULL values. In the long run it would be good to fix
> cl_assert_equal_s() to properly quote control characters as check_str()
> does.

That's indeed something we should fix in clar.

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

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

Patrick