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

[Christian Couder <christian.couder@gmail.com>]

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.

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

> > +int cmd_main(int argc, const char **argv) {
> > +     /* Run all character type tests */
> > +     TEST(test_ctype_isspace(), "isspace() works as we expect");
> > +     TEST(test_ctype_isdigit(), "isdigit() works as we expect");
> > +     TEST(test_ctype_isalpha(), "isalpha() works as we expect");
> > +     TEST(test_ctype_isalnum(), "isalnum() works as we expect");
> > +     TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect");
> > +     TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect");
> > +     TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect");
> > +     TEST(test_ctype_isascii(), "isascii() works as we expect");
> > +     TEST(test_ctype_islower(), "islower() works as we expect");
> > +     TEST(test_ctype_isupper(), "isupper() works as we expect");
> > +     TEST(test_ctype_iscntrl(), "iscntrl() works as we expect");
> > +     TEST(test_ctype_ispunct(), "ispunct() works as we expect");
> > +     TEST(test_ctype_isxdigit(), "isxdigit() works as we expect");
> > +     TEST(test_ctype_isprint(), "isprint() works as we expect");
> > +
> > +     return test_done();
> > +}
>
> As a practice to use the unit-tests framework, the patch looks OK.
> helper/test-ctype.c indeed is an oddball that runs once and checks
> everything it wants to check, for which the unit tests framework is
> much more suited.

Yeah, I agree.

> Let's see how others react and then queue.
>
> Thanks.