threads / patch / 60649

patchPort helper/test-ctype.c to unit-tests/t-ctype.c

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

## tl;dr

24 messages between Dec 21, 2023 and Jan 17, 2024. Diffs are folded; open one to read it.

replies: 23people: 7as markdown or json

Achu Luma· Dec 21, 2023, 23:15 UTC · lore

In the recent codebase update (8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework.

This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h).

The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org> 
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 Makefile               |  2 +-
 t/helper/test-ctype.c  | 70 --------------------------------------
 t/helper/test-tool.c   |  1 -
 t/helper/test-tool.h   |  1 -
 t/t0070-fundamental.sh |  4 ---
 t/unit-tests/t-ctype.c | 76 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 77 insertions(+), 77 deletions(-)
 delete mode 100644 t/helper/test-ctype.c
 create mode 100644 t/unit-tests/t-ctype.c
Show changes to 6 files +77 −77

Makefile, t/helper/test-ctype.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t0070-fundamental.sh, t/unit-tests/t-ctype.c

diff --git a/Makefile b/Makefile
index 88ba7a3c51..a4df48ba65 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-crontab.o
 TEST_BUILTINS_OBJS += test-csprng.o
-TEST_BUILTINS_OBJS += test-ctype.o
 TEST_BUILTINS_OBJS += test-date.o
 TEST_BUILTINS_OBJS += test-delta.o
 TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
-	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
-	rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
-	/*
-	 * We can't find NUL using strchr. Accept it as the first
-	 * character in the spec -- there are no empty classes.
-	 */
-	if (ch == '\0')
-		return ch == *s;
-	if (*s == '\0')
-		s++;
-	return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) {			\
-	int i;					\
-	for (i = 0; i < 256; i++) {		\
-		if (is_in(s, i) != t(i))	\
-			report_error(#t, i);	\
-	}					\
-	if (t(EOF))				\
-		report_error(#t, EOF);		\
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
-	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
-	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
-	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
-	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
-	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST_CLASS(isdigit, DIGIT);
-	TEST_CLASS(isspace, " \n\r\t");
-	TEST_CLASS(isalpha, LOWER UPPER);
-	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
-	TEST_CLASS(is_glob_special, "*?[\\");
-	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
-	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
-	TEST_CLASS(isascii, ASCII);
-	TEST_CLASS(islower, LOWER);
-	TEST_CLASS(isupper, UPPER);
-	TEST_CLASS(iscntrl, CNTRL);
-	TEST_CLASS(ispunct, PUNCT);
-	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
-	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
-	return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
 	{ "config", cmd__config },
 	{ "crontab", cmd__crontab },
 	{ "csprng", cmd__csprng },
-	{ "ctype", cmd__ctype },
 	{ "date", cmd__date },
 	{ "delta", cmd__delta },
 	{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__crontab(int argc, const char **argv);
 int cmd__csprng(int argc, const char **argv);
-int cmd__ctype(int argc, const char **argv);
 int cmd__date(int argc, const char **argv);
 int cmd__delta(int argc, const char **argv);
 int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'character classes (isspace, isalpha etc.)' '
-	test-tool ctype
-'
-
 test_expect_success 'mktemp to nonexistent directory prints filename' '
 	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
 	grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..41189ba9f9
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,76 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+	/*
+	 * We can't find NUL using strchr. Accept it as the first
+	 * character in the spec -- there are no empty classes.
+	 */
+	if (ch == '\0')
+		return ch == *s;
+	if (*s == '\0')
+		s++;
+	return !!strchr(s, ch);
+}
+
+/* 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)); 	\
+}
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+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();
+}

base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
-- 
2.42.0.windows.2
Junio C Hamano· Dec 26, 2023, 18:45 UTC · re: Achu Luma · lore

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

Achu Luma <ach.lumap@gmail.com> writes:
Show 15 quoted lines
> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> deleted file mode 100644
> index e5659df40b..0000000000
> --- a/t/helper/test-ctype.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -#include "test-tool.h"
> -
> -static int rc;
> -
> -static void report_error(const char *class, int ch)
> -{
> -	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> -	rc = 1;
> -}

So, if we have a is_foo() that characterises a byte that ought to be "foo" but gets miscategorised not to be "foo", we used to pinpoint exactly the byte value that was an issue. We did not do any early return ...

Show 10 quoted lines
> ...
> -#define TEST_CLASS(t,s) {			\
> -	int i;					\
> -	for (i = 0; i < 256; i++) {		\
> -		if (is_in(s, i) != t(i))	\
> -			report_error(#t, i);	\
> -	}					\
> -	if (t(EOF))				\
> -		report_error(#t, EOF);		\
> -}
... and reported for all errors in the "class".
Show 29 quoted lines
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> new file mode 100644
> index 0000000000..41189ba9f9
> --- /dev/null
> +++ b/t/unit-tests/t-ctype.c
> @@ -0,0 +1,76 @@
> +#include "test-lib.h"
> +
> +static int is_in(const char *s, int ch)
> +{
> +	/*
> +	 * We can't find NUL using strchr. Accept it as the first
> +	 * character in the spec -- there are no empty classes.
> +	 */
> +	if (ch == '\0')
> +		return ch == *s;
> +	if (*s == '\0')
> +		s++;
> +	return !!strchr(s, ch);
> +}
> +
> +/* 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.

Show 19 quoted lines
> +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.

Let's see how others react and then queue.
Thanks.
Christian Couder· Dec 27, 2023, 10:57 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Achu Luma <ach.lumap@gmail.com> writes:
Show 19 quoted lines
> > +/* 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".

Show 24 quoted lines
> > +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.
René Scharfe· Dec 27, 2023, 11:57 UTC · re: Christian Couder · lore

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

Am 27.12.23 um 11:57 schrieb Christian Couder:
Show 25 quoted lines
> 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().
> 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".
True.
Show 21 quoted lines
>>> +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.

The added repetition is a bit grating. With a bit of setup, loop unrolling and stringification you can retain the property of only having to mention the class name once. Demo patch below.

Show 5 quoted lines
>> 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.
Indeed: test-ctype does unit tests, so the unit test framework should
be a perfect fit.  It still feels a bit raw that this point, though,
but that's to be expected.
René
Show changes to t/unit-tests/t-ctype.c +47 −35
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
index 41189ba9f9..7903856cec 100644
--- a/t/unit-tests/t-ctype.c
+++ b/t/unit-tests/t-ctype.c
@@ -13,13 +13,23 @@ static int is_in(const char *s, int ch)
 	return !!strchr(s, ch);
 }

-/* 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)); 	\
+struct ctype {
+	const char *name;
+	const char *expect;
+	int actual[256];
+};
+
+static void test_ctype(const struct ctype *class)
+{
+	for (int i = 0; i < 256; i++) {
+		int expect = is_in(class->expect, i);
+		int actual = class->actual[i];
+		int res = test_assert(TEST_LOCATION(), class->name,
+				      actual == expect);
+		if (!res)
+			test_msg("%s classifies char %d (0x%02x) wrongly",
+				 class->name, i, i);
+	}
 }

 #define DIGIT "0123456789"
@@ -40,37 +50,39 @@ static void test_ctype_##func(void)				\
 	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
 	"\x7f"

-TEST_CTYPE_FUNC(isdigit, DIGIT)
-TEST_CTYPE_FUNC(isspace, " \n\r\t")
-TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
-TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
-TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
-TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
-TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
-TEST_CTYPE_FUNC(isascii, ASCII)
-TEST_CTYPE_FUNC(islower, LOWER)
-TEST_CTYPE_FUNC(isupper, UPPER)
-TEST_CTYPE_FUNC(iscntrl, CNTRL)
-TEST_CTYPE_FUNC(ispunct, PUNCT)
-TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
-TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+#define APPLY16(f, n) \
+	f(n + 0x0), f(n + 0x1), f(n + 0x2), f(n + 0x3), \
+	f(n + 0x4), f(n + 0x5), f(n + 0x6), f(n + 0x7), \
+	f(n + 0x8), f(n + 0x9), f(n + 0xa), f(n + 0xb), \
+	f(n + 0xc), f(n + 0xd), f(n + 0xe), f(n + 0xf)
+#define APPLY256(f) \
+	APPLY16(f, 0x00), APPLY16(f, 0x10), APPLY16(f, 0x20), APPLY16(f, 0x30),\
+	APPLY16(f, 0x40), APPLY16(f, 0x50), APPLY16(f, 0x60), APPLY16(f, 0x70),\
+	APPLY16(f, 0x80), APPLY16(f, 0x90), APPLY16(f, 0xa0), APPLY16(f, 0xb0),\
+	APPLY16(f, 0xc0), APPLY16(f, 0xd0), APPLY16(f, 0xe0), APPLY16(f, 0xf0),\
+
+#define CTYPE(name, expect) { #name, expect, { APPLY256(name) }  }

 int cmd_main(int argc, const char **argv) {
+	struct ctype classes[] = {
+		CTYPE(isdigit, DIGIT),
+		CTYPE(isspace, " \n\r\t"),
+		CTYPE(isalpha, LOWER UPPER),
+		CTYPE(isalnum, LOWER UPPER DIGIT),
+		CTYPE(is_glob_special, "*?[\\"),
+		CTYPE(is_regex_special, "$()*+.?[\\^{|"),
+		CTYPE(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"),
+		CTYPE(isascii, ASCII),
+		CTYPE(islower, LOWER),
+		CTYPE(isupper, UPPER),
+		CTYPE(iscntrl, CNTRL),
+		CTYPE(ispunct, PUNCT),
+		CTYPE(isxdigit, DIGIT "abcdefABCDEF"),
+		CTYPE(isprint, LOWER UPPER DIGIT PUNCT " "),
+	};
 	/* 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");
+	for (int i = 0; i < ARRAY_SIZE(classes); i++)
+		TEST(test_ctype(&classes[i]), "%s works", classes[i].name);

 	return test_done();
 }
Phillip Wood· Dec 27, 2023, 14:40 UTC · re: René Scharfe · lore

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

On 27/12/2023 11:57, René Scharfe wrote:
Show 28 quoted lines
> 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"
Junio C Hamano· Dec 27, 2023, 23:48 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 9 quoted lines
>> 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".
>
> True.
> ...
> The added repetition is a bit grating.  With a bit of setup, loop
> unrolling and stringification you can retain the property of only having
> to mention the class name once.  Demo patch below.
Nice.

This (and your mempool thing) being one of the early efforts to adopt the unit-test framework outside the initial set of sample tests, it is understandable that we might find what framework offers is still lacking. But at the same time, while the macro tricks demonstrated here are all amusing to read and admire, it feels a bit too much to expect that the test writers are willing to invent something like these every time they want to test.

Being a relatively faithful conversion of the original ctype tests, with its thorough enumeration of test samples and expected output, is what makes this test program require these macro tricks, and it does not have much to do with the features (or lack thereof) of the framework, I guess.

Show 18 quoted lines
> +struct ctype {
> +	const char *name;
> +	const char *expect;
> +	int actual[256];
> +};
> +
> +static void test_ctype(const struct ctype *class)
> +{
> +	for (int i = 0; i < 256; i++) {
> +		int expect = is_in(class->expect, i);
> +		int actual = class->actual[i];
> +		int res = test_assert(TEST_LOCATION(), class->name,
> +				      actual == expect);
> +		if (!res)
> +			test_msg("%s classifies char %d (0x%02x) wrongly",
> +				 class->name, i, i);
> +	}
>  }

Somehow, the "test_assert" does not seem to be adding much value here (i.e. we can do "res = (actual == expect)" there). Is this because we want to be able to report success, too?

    ... goes and looks at test_assert() ...

Ah, is it because we want to be able to "skip" (which pretends that the assert() was satisified). OK, but then the error reporting from it is redundant with our own test_msg().

Everything below this line was a fun read ;-)
Thanks.
Show 51 quoted lines
> ...
> +#define APPLY16(f, n) \
> +	f(n + 0x0), f(n + 0x1), f(n + 0x2), f(n + 0x3), \
> +	f(n + 0x4), f(n + 0x5), f(n + 0x6), f(n + 0x7), \
> +	f(n + 0x8), f(n + 0x9), f(n + 0xa), f(n + 0xb), \
> +	f(n + 0xc), f(n + 0xd), f(n + 0xe), f(n + 0xf)
> +#define APPLY256(f) \
> +	APPLY16(f, 0x00), APPLY16(f, 0x10), APPLY16(f, 0x20), APPLY16(f, 0x30),\
> +	APPLY16(f, 0x40), APPLY16(f, 0x50), APPLY16(f, 0x60), APPLY16(f, 0x70),\
> +	APPLY16(f, 0x80), APPLY16(f, 0x90), APPLY16(f, 0xa0), APPLY16(f, 0xb0),\
> +	APPLY16(f, 0xc0), APPLY16(f, 0xd0), APPLY16(f, 0xe0), APPLY16(f, 0xf0),\
> +
> +#define CTYPE(name, expect) { #name, expect, { APPLY256(name) }  }
>
>  int cmd_main(int argc, const char **argv) {
> +	struct ctype classes[] = {
> +		CTYPE(isdigit, DIGIT),
> +		CTYPE(isspace, " \n\r\t"),
> +		CTYPE(isalpha, LOWER UPPER),
> +		CTYPE(isalnum, LOWER UPPER DIGIT),
> +		CTYPE(is_glob_special, "*?[\\"),
> +		CTYPE(is_regex_special, "$()*+.?[\\^{|"),
> +		CTYPE(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"),
> +		CTYPE(isascii, ASCII),
> +		CTYPE(islower, LOWER),
> +		CTYPE(isupper, UPPER),
> +		CTYPE(iscntrl, CNTRL),
> +		CTYPE(ispunct, PUNCT),
> +		CTYPE(isxdigit, DIGIT "abcdefABCDEF"),
> +		CTYPE(isprint, LOWER UPPER DIGIT PUNCT " "),
> +	};
>  	/* 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");
> +	for (int i = 0; i < ARRAY_SIZE(classes); i++)
> +		TEST(test_ctype(&classes[i]), "%s works", classes[i].name);
>
>  	return test_done();
>  }
René Scharfe· Dec 28, 2023, 16:05 UTC · re: Junio C Hamano · lore

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

Am 28.12.23 um 00:48 schrieb Junio C Hamano:
Show 27 quoted lines
> René Scharfe <l.s.r@web.de> writes:
>
>>> 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".
>>
>> True.
>> ...
>> The added repetition is a bit grating.  With a bit of setup, loop
>> unrolling and stringification you can retain the property of only having
>> to mention the class name once.  Demo patch below.
>
> Nice.
>
> This (and your mempool thing) being one of the early efforts to
> adopt the unit-test framework outside the initial set of sample
> tests, it is understandable that we might find what framework offers
> is still lacking.  But at the same time, while the macro tricks
> demonstrated here are all amusing to read and admire, it feels a bit
> too much to expect that the test writers are willing to invent
> something like these every time they want to test.
>
> Being a relatively faithful conversion of the original ctype tests,
> with its thorough enumeration of test samples and expected output,
> is what makes this test program require these macro tricks, and it
> does not have much to do with the features (or lack thereof) of the
> framework, I guess.
*nod*
Show 29 quoted lines
>
>> +struct ctype {
>> +	const char *name;
>> +	const char *expect;
>> +	int actual[256];
>> +};
>> +
>> +static void test_ctype(const struct ctype *class)
>> +{
>> +	for (int i = 0; i < 256; i++) {
>> +		int expect = is_in(class->expect, i);
>> +		int actual = class->actual[i];
>> +		int res = test_assert(TEST_LOCATION(), class->name,
>> +				      actual == expect);
>> +		if (!res)
>> +			test_msg("%s classifies char %d (0x%02x) wrongly",
>> +				 class->name, i, i);
>> +	}
>>  }
>
> Somehow, the "test_assert" does not seem to be adding much value
> here (i.e. we can do "res = (actual == expect)" there).  Is this
> because we want to be able to report success, too?
>
>     ... goes and looks at test_assert() ...
>
> Ah, is it because we want to be able to "skip" (which pretends that
> the assert() was satisified).  OK, but then the error reporting from
> it is redundant with our own test_msg().

True, the test_msg() emits the old message here, but it doesn't have to report that the check failed anymore, because test_assert() already covers that part. It would only have to report the misclassified character and perhaps the expected result.

René
Taylor Blau· Jan 2, 2024, 18:55 UTC · re: Junio C Hamano · lore

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

On Tue, Dec 26, 2023 at 10:45:59AM -0800, Junio C Hamano wrote:
Show 76 quoted lines
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> > deleted file mode 100644
> > index e5659df40b..0000000000
> > --- a/t/helper/test-ctype.c
> > +++ /dev/null
> > @@ -1,70 +0,0 @@
> > -#include "test-tool.h"
> > -
> > -static int rc;
> > -
> > -static void report_error(const char *class, int ch)
> > -{
> > -	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> > -	rc = 1;
> > -}
>
> So, if we have a is_foo() that characterises a byte that ought to
> be "foo" but gets miscategorised not to be "foo", we used to
> pinpoint exactly the byte value that was an issue.  We did not do
> any early return ...
>
> > ...
> > -#define TEST_CLASS(t,s) {			\
> > -	int i;					\
> > -	for (i = 0; i < 256; i++) {		\
> > -		if (is_in(s, i) != t(i))	\
> > -			report_error(#t, i);	\
> > -	}					\
> > -	if (t(EOF))				\
> > -		report_error(#t, EOF);		\
> > -}
>
> ... and reported for all errors in the "class".
>
> > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> > new file mode 100644
> > index 0000000000..41189ba9f9
> > --- /dev/null
> > +++ b/t/unit-tests/t-ctype.c
> > @@ -0,0 +1,76 @@
> > +#include "test-lib.h"
> > +
> > +static int is_in(const char *s, int ch)
> > +{
> > +	/*
> > +	 * We can't find NUL using strchr. Accept it as the first
> > +	 * character in the spec -- there are no empty classes.
> > +	 */
> > +	if (ch == '\0')
> > +		return ch == *s;
> > +	if (*s == '\0')
> > +		s++;
> > +	return !!strchr(s, ch);
> > +}
> > +
> > +/* 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.

Perhaps I'm missing something here, since I haven't followed the unit-test effort very closely, but this check_int() macro feels like it might be overkill for what we're trying to do.

We know that the expected value is the result of is_in(string, i), so I wonder if we might benefit from having an "assert_equals()" that looks like:

    assert_equals(is_in(string, i), func(i));

Where we follow the usual convention of treating the first argument as the expected value, and the second as the actual value. Then we could format our error message to be more specific, like:

    test_msg("expected %d, got %d", expected, actual);

I think that this would be a little more readable, and still seems flexible enough to support the kind of thing that check_int(..., ==, ...) is after.

Show 24 quoted lines
> > +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.

As an aside, I don't think we need the "works as we expect" suffix in each test description. I personally would be fine with something like:

    TEST(test_ctype_isspace(), "isspace()");
    TEST(test_ctype_isdigit(), "isdigit()");
    ...
But don't feel strongly about it.

Thanks, Taylor

Achu Luma· Dec 30, 2023, 00:09 UTC · re: Achu Luma · lore

[Outreachy][PATCH v2] Port helper/test-ctype.c to unit-tests/t-ctype.c

In the recent codebase update(8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework.

This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h).

The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 In the revised patch, several improvements were made to the 
 TEST_CTYPE_FUNC macro. The updated version enhances error handling 
 by utilizing a direct comparison approach between the func and 
 is_in(string, i) functions across ASCII characters. Additionally, 
 the loop control variable i is locally scoped within the loop, ensuring 
 proper encapsulation. These changes streamline the comparison process 
 and clarify failure reporting. Special thanks to the invaluable reviews 
 by Junio, Phillip and René for their insightful feedback and thorough 
 review of the patch, contributing significantly to these changes.
 Makefile               |  2 +-
 t/helper/test-ctype.c  | 70 --------------------------------------
 t/helper/test-tool.c   |  1 -
 t/helper/test-tool.h   |  1 -
 t/t0070-fundamental.sh |  4 ---
 t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 78 insertions(+), 77 deletions(-)
 delete mode 100644 t/helper/test-ctype.c
 create mode 100644 t/unit-tests/t-ctype.c
Show changes to 6 files +78 −77

Makefile, t/helper/test-ctype.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t0070-fundamental.sh, t/unit-tests/t-ctype.c

diff --git a/Makefile b/Makefile
index 88ba7a3c51..a4df48ba65 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-crontab.o
 TEST_BUILTINS_OBJS += test-csprng.o
-TEST_BUILTINS_OBJS += test-ctype.o
 TEST_BUILTINS_OBJS += test-date.o
 TEST_BUILTINS_OBJS += test-delta.o
 TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
-	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
-	rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
-	/*
-	 * We can't find NUL using strchr. Accept it as the first
-	 * character in the spec -- there are no empty classes.
-	 */
-	if (ch == '\0')
-		return ch == *s;
-	if (*s == '\0')
-		s++;
-	return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) {			\
-	int i;					\
-	for (i = 0; i < 256; i++) {		\
-		if (is_in(s, i) != t(i))	\
-			report_error(#t, i);	\
-	}					\
-	if (t(EOF))				\
-		report_error(#t, EOF);		\
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
-	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
-	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
-	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
-	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
-	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST_CLASS(isdigit, DIGIT);
-	TEST_CLASS(isspace, " \n\r\t");
-	TEST_CLASS(isalpha, LOWER UPPER);
-	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
-	TEST_CLASS(is_glob_special, "*?[\\");
-	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
-	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
-	TEST_CLASS(isascii, ASCII);
-	TEST_CLASS(islower, LOWER);
-	TEST_CLASS(isupper, UPPER);
-	TEST_CLASS(iscntrl, CNTRL);
-	TEST_CLASS(ispunct, PUNCT);
-	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
-	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
-	return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
 	{ "config", cmd__config },
 	{ "crontab", cmd__crontab },
 	{ "csprng", cmd__csprng },
-	{ "ctype", cmd__ctype },
 	{ "date", cmd__date },
 	{ "delta", cmd__delta },
 	{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__crontab(int argc, const char **argv);
 int cmd__csprng(int argc, const char **argv);
-int cmd__ctype(int argc, const char **argv);
 int cmd__date(int argc, const char **argv);
 int cmd__delta(int argc, const char **argv);
 int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'character classes (isspace, isalpha etc.)' '
-	test-tool ctype
-'
-
 test_expect_success 'mktemp to nonexistent directory prints filename' '
 	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
 	grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..8a215d387a
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,77 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+	/*
+	 * We can't find NUL using strchr. Accept it as the first
+	 * character in the spec -- there are no empty classes.
+	 */
+	if (ch == '\0')
+		return ch == *s;
+	if (*s == '\0')
+		s++;
+	return !!strchr(s, ch);
+}
+
+/* Macro to test a character type */
+#define TEST_CTYPE_FUNC(func, string)				\
+static void test_ctype_##func(void)				\
+{								\
+	for (int i = 0; i < 256; i++) {				\
+        	if (!check_int(func(i), ==, is_in(string, i))) 	\
+        		test_msg("       i: %02x", i);		\
+        } 							\
+}
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+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();
+}
-- 
2.42.0.windows.2
Achu Luma· Jan 1, 2024, 10:40 UTC · re: Achu Luma · lore

[Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c

In the recent codebase update(8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework.

This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h).

The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()).

Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 Sorry for the poor description of the changes, maybe the following is better:
 In the revised patch we added a call to test_msg() suggested by Phillip to 
 print the character code. This helps us pinpoint exactly the byte value that 
 is an issue as suggested by Junio. We keep using check_int() as it allows us 
 to still see the actual and expected return which might help us in case func() 
 returned a different non-zero value when we were expecting "1". It is useful 
 to have the return value printed on error in case we start returning "53" 
 instead of "1" for "true".
 Makefile               |  2 +-
 t/helper/test-ctype.c  | 70 --------------------------------------
 t/helper/test-tool.c   |  1 -
 t/helper/test-tool.h   |  1 -
 t/t0070-fundamental.sh |  4 ---
 t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 78 insertions(+), 77 deletions(-)
 delete mode 100644 t/helper/test-ctype.c
 create mode 100644 t/unit-tests/t-ctype.c
Show changes to 6 files +78 −77

Makefile, t/helper/test-ctype.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t0070-fundamental.sh, t/unit-tests/t-ctype.c

diff --git a/Makefile b/Makefile
index 88ba7a3c51..a4df48ba65 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-crontab.o
 TEST_BUILTINS_OBJS += test-csprng.o
-TEST_BUILTINS_OBJS += test-ctype.o
 TEST_BUILTINS_OBJS += test-date.o
 TEST_BUILTINS_OBJS += test-delta.o
 TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
-	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
-	rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
-	/*
-	 * We can't find NUL using strchr. Accept it as the first
-	 * character in the spec -- there are no empty classes.
-	 */
-	if (ch == '\0')
-		return ch == *s;
-	if (*s == '\0')
-		s++;
-	return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) {			\
-	int i;					\
-	for (i = 0; i < 256; i++) {		\
-		if (is_in(s, i) != t(i))	\
-			report_error(#t, i);	\
-	}					\
-	if (t(EOF))				\
-		report_error(#t, EOF);		\
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
-	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
-	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
-	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
-	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
-	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST_CLASS(isdigit, DIGIT);
-	TEST_CLASS(isspace, " \n\r\t");
-	TEST_CLASS(isalpha, LOWER UPPER);
-	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
-	TEST_CLASS(is_glob_special, "*?[\\");
-	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
-	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
-	TEST_CLASS(isascii, ASCII);
-	TEST_CLASS(islower, LOWER);
-	TEST_CLASS(isupper, UPPER);
-	TEST_CLASS(iscntrl, CNTRL);
-	TEST_CLASS(ispunct, PUNCT);
-	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
-	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
-	return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
 	{ "config", cmd__config },
 	{ "crontab", cmd__crontab },
 	{ "csprng", cmd__csprng },
-	{ "ctype", cmd__ctype },
 	{ "date", cmd__date },
 	{ "delta", cmd__delta },
 	{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__crontab(int argc, const char **argv);
 int cmd__csprng(int argc, const char **argv);
-int cmd__ctype(int argc, const char **argv);
 int cmd__date(int argc, const char **argv);
 int cmd__delta(int argc, const char **argv);
 int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
-test_expect_success 'character classes (isspace, isalpha etc.)' '
-	test-tool ctype
-'
-
 test_expect_success 'mktemp to nonexistent directory prints filename' '
 	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
 	grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..8a215d387a
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,77 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+	/*
+	 * We can't find NUL using strchr. Accept it as the first
+	 * character in the spec -- there are no empty classes.
+	 */
+	if (ch == '\0')
+		return ch == *s;
+	if (*s == '\0')
+		s++;
+	return !!strchr(s, ch);
+}
+
+/* Macro to test a character type */
+#define TEST_CTYPE_FUNC(func, string)				\
+static void test_ctype_##func(void)				\
+{								\
+	for (int i = 0; i < 256; i++) {				\
+        	if (!check_int(func(i), ==, is_in(string, i))) 	\
+        		test_msg("       i: %02x", i);		\
+        } 							\
+}
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+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();
+}
-- 
2.42.0.windows.2
René Scharfe· Jan 1, 2024, 16:41 UTC · re: Achu Luma · lore

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

Am 01.01.24 um 11:40 schrieb Achu Luma:
Show 16 quoted lines
> In the recent codebase update(8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>

v1 and v2 had "Mentored-by: Christian Couder <chriscool@tuxfamily.org>". It's gone now; intentionally?

Show 9 quoted lines
> ---
>  Sorry for the poor description of the changes, maybe the following is better:
>  In the revised patch we added a call to test_msg() suggested by Phillip to
>  print the character code. This helps us pinpoint exactly the byte value that
>  is an issue as suggested by Junio. We keep using check_int() as it allows us
>  to still see the actual and expected return which might help us in case func()
>  returned a different non-zero value when we were expecting "1". It is useful
>  to have the return value printed on error in case we start returning "53"
>  instead of "1" for "true".
To illustrate, here are the changes between v1 and v2 in diff form:
--- >8 ---
Show changes to t/unit-tests/t-ctype.c +5 −4
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
index 41189ba9f9..8a215d387a 100644
--- a/t/unit-tests/t-ctype.c
+++ b/t/unit-tests/t-ctype.c
@@ -14,12 +14,13 @@ static int is_in(const char *s, int ch)
 }

 /* Macro to test a character type */
-#define TEST_CTYPE_FUNC(func, string)			\
+#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)); 	\
+	for (int i = 0; i < 256; i++) {				\
+        	if (!check_int(func(i), ==, is_in(string, i))) 	\
+        		test_msg("       i: %02x", i);		\
+        } 							\
 }

 #define DIGIT "0123456789"
--- >8 ---

v3 is the same as v2.

>
>  Makefile               |  2 +-
>  t/helper/test-ctype.c  | 70 --------------------------------------
>  t/helper/test-tool.c   |  1 -
>  t/helper/test-tool.h   |  1 -
>  t/t0070-fundamental.sh |  4 ---
>  t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 78 insertions(+), 77 deletions(-)
>  delete mode 100644 t/helper/test-ctype.c
>  create mode 100644 t/unit-tests/t-ctype.c
>
> diff --git a/Makefile b/Makefile
> index 88ba7a3c51..a4df48ba65 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
>  TEST_BUILTINS_OBJS += test-config.o
>  TEST_BUILTINS_OBJS += test-crontab.o
>  TEST_BUILTINS_OBJS += test-csprng.o
> -TEST_BUILTINS_OBJS += test-ctype.o
>  TEST_BUILTINS_OBJS += test-date.o
>  TEST_BUILTINS_OBJS += test-delta.o
>  TEST_BUILTINS_OBJS += test-dir-iterator.o
> @@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
>
>  UNIT_TEST_PROGRAMS += t-basic
>  UNIT_TEST_PROGRAMS += t-strbuf
> +UNIT_TEST_PROGRAMS += t-ctype
>  UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
>  UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
>  UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> deleted file mode 100644
> index e5659df40b..0000000000
> --- a/t/helper/test-ctype.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -#include "test-tool.h"
> -
> -static int rc;
> -
> -static void report_error(const char *class, int ch)
> -{
> -	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> -	rc = 1;
> -}
> -
> -static int is_in(const char *s, int ch)
> -{
> -	/*
> -	 * We can't find NUL using strchr. Accept it as the first
> -	 * character in the spec -- there are no empty classes.
> -	 */
> -	if (ch == '\0')
> -		return ch == *s;
> -	if (*s == '\0')
> -		s++;
> -	return !!strchr(s, ch);
> -}
> -
> -#define TEST_CLASS(t,s) {			\
> -	int i;					\
> -	for (i = 0; i < 256; i++) {		\
> -		if (is_in(s, i) != t(i))	\
> -			report_error(#t, i);	\
> -	}					\
> -	if (t(EOF))				\
> -		report_error(#t, EOF);		\
> -}
> -
> -#define DIGIT "0123456789"
> -#define LOWER "abcdefghijklmnopqrstuvwxyz"
> -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> -#define ASCII \
> -	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> -	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> -	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> -	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> -	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> -	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> -	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> -	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> -#define CNTRL \
> -	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> -	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> -	"\x7f"
> -
> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
> -{
> -	TEST_CLASS(isdigit, DIGIT);
> -	TEST_CLASS(isspace, " \n\r\t");
> -	TEST_CLASS(isalpha, LOWER UPPER);
> -	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
> -	TEST_CLASS(is_glob_special, "*?[\\");
> -	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
> -	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
> -	TEST_CLASS(isascii, ASCII);
> -	TEST_CLASS(islower, LOWER);
> -	TEST_CLASS(isupper, UPPER);
> -	TEST_CLASS(iscntrl, CNTRL);
> -	TEST_CLASS(ispunct, PUNCT);
> -	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
> -	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
> -
> -	return rc;
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 37ba996539..33b9501c21 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
>  	{ "config", cmd__config },
>  	{ "crontab", cmd__crontab },
>  	{ "csprng", cmd__csprng },
> -	{ "ctype", cmd__ctype },
>  	{ "date", cmd__date },
>  	{ "delta", cmd__delta },
>  	{ "dir-iterator", cmd__dir_iterator },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 8a1a7c63da..b72f07ded9 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
>  int cmd__config(int argc, const char **argv);
>  int cmd__crontab(int argc, const char **argv);
>  int cmd__csprng(int argc, const char **argv);
> -int cmd__ctype(int argc, const char **argv);
>  int cmd__date(int argc, const char **argv);
>  int cmd__delta(int argc, const char **argv);
>  int cmd__dir_iterator(int argc, const char **argv);
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index 487bc8d905..a4756fbab9 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
>  TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
> -test_expect_success 'character classes (isspace, isalpha etc.)' '
> -	test-tool ctype
> -'
> -
>  test_expect_success 'mktemp to nonexistent directory prints filename' '
>  	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
>  	grep "doesnotexist/test" err
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> new file mode 100644
> index 0000000000..8a215d387a
> --- /dev/null
> +++ b/t/unit-tests/t-ctype.c
> @@ -0,0 +1,77 @@
> +#include "test-lib.h"
> +
> +static int is_in(const char *s, int ch)
> +{
> +	/*
> +	 * We can't find NUL using strchr. Accept it as the first
> +	 * character in the spec -- there are no empty classes.
> +	 */
> +	if (ch == '\0')
> +		return ch == *s;
> +	if (*s == '\0')
> +		s++;
> +	return !!strchr(s, ch);
> +}
> +
> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string)				\
> +static void test_ctype_##func(void)				\
> +{								\
> +	for (int i = 0; i < 256; i++) {				\
> +        	if (!check_int(func(i), ==, is_in(string, i))) 	\
> +        		test_msg("       i: %02x", i);		\
> +        } 							\

This loop is indented with spaces followed by tabs.  The Git project
prefers indenting by tabs and in some cases tabs followed by spaces, but
not the other way array.  git am warns about such whitespace errors and
can actually fix them automatically, so I imagine this wouldn't be a
deal breaker.  But even if it seems picky, respecting the project's
preferences from the start reduces unnecessary friction.

The original test reported the number of a misclassified character
(basically its ASCII code) in both decimal and hexadecimal form.  This
code prints it only in hexadecimal, but without the prefix "0x".  A
casual reader could mistake hexadecimal numbers for decimal ones as a
result.  Printing only one suffices, but I think it's better to either
use decimal notation without any prefix or hexadecimal with the "0x"
prefix to avoid confusion.  There's no reason to be stingy with the
screen space here.

> +}
> +
> +#define DIGIT "0123456789"
> +#define LOWER "abcdefghijklmnopqrstuvwxyz"
> +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> +#define ASCII \
> +	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> +	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> +	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> +	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> +	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> +	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> +	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> +	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> +#define CNTRL \
> +	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> +	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> +	"\x7f"
> +
> +TEST_CTYPE_FUNC(isdigit, DIGIT)
> +TEST_CTYPE_FUNC(isspace, " \n\r\t")
> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
> +TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
> +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
> +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
> +TEST_CTYPE_FUNC(isascii, ASCII)
> +TEST_CTYPE_FUNC(islower, LOWER)
> +TEST_CTYPE_FUNC(isupper, UPPER)
> +TEST_CTYPE_FUNC(iscntrl, CNTRL)
> +TEST_CTYPE_FUNC(ispunct, PUNCT)
> +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
> +
> +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");

Each class (e.g. space or digit) is mentioned thrice here: When
declaring its function with TEST_CTYPE_FUNC, when calling said function
and again in the test description.  Adding a new class requires adding
two lines of code.  That's not too bad, but the original implementation
didn't require that repetition and adding a new class only required
adding a single line.

I mentioned this briefly in my review of v1 in
https://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/
and showed a way to define character classes without repeating their
names.  You don't have to follow that suggestion, but it would be nice
if you could give some feedback about it.

> +
> +	return test_done();
> +}

René
Junio C Hamano· Jan 2, 2024, 16:35 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 28 quoted lines
>> ...
>> +	for (int i = 0; i < 256; i++) {				\
>> +        	if (!check_int(func(i), ==, is_in(string, i))) 	\
>> +        		test_msg("       i: %02x", i);		\
>> +        } 							\
>
> This loop is indented with spaces followed by tabs.  The Git project
> prefers indenting by tabs and in some cases tabs followed by spaces, but
> not the other way array.  git am warns about such whitespace errors and
> can actually fix them automatically, so I imagine this wouldn't be a
> deal breaker.  But even if it seems picky, respecting the project's
> preferences from the start reduces unnecessary friction.
>
> The original test reported the number of a misclassified character
> (basically its ASCII code) in both decimal and hexadecimal form.  This
> code prints it only in hexadecimal, but without the prefix "0x".  A
> casual reader could mistake hexadecimal numbers for decimal ones as a
> result.  Printing only one suffices, but I think it's better to either
> use decimal notation without any prefix or hexadecimal with the "0x"
> prefix to avoid confusion.  There's no reason to be stingy with the
> screen space here.
> ...
> Each class (e.g. space or digit) is mentioned thrice here: When
> declaring its function with TEST_CTYPE_FUNC, when calling said function
> and again in the test description.  Adding a new class requires adding
> two lines of code.  That's not too bad, but the original implementation
> didn't require that repetition and adding a new class only required
> adding a single line.
Thanks for an excellent review.
Show 12 quoted lines
>
> I mentioned this briefly in my review of v1 in
> https://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/
> and showed a way to define character classes without repeating their
> names.  You don't have to follow that suggestion, but it would be nice
> if you could give some feedback about it.
>
>> +
>> +	return test_done();
>> +}
>
> René
Achu Luma· Jan 5, 2024, 16:14 UTC · re: Achu Luma · lore

[Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c

In the recent codebase update (8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework.

This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h).

The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: René Scharfe <l.s.r@web.de>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 The changes between version 3 and version 4 are the following:
 - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro.
 - A "0x"prefix has been added to avoid confusion between decimal and
   hexadecimal codes printed by test_msg().
 - The "Mentored-by:..." trailer has been restored.
 - "works as expected" has been reduced to just "works" as suggested by Taylor.
 - Some "Helped-by: ..." trailers have been added.
 - Some whitespace fixes have been made.
 Thanks to Junio, René, Phillip and Taylor for commenting on previous versions
 of this patch.
 Here is a diff between v3 and v4:
  @@ -14,15 +14,16 @@ static int is_in(const char *s, int ch)
  }
  /* Macro to test a character type */
 -#define TEST_CTYPE_FUNC(func, string)                          \
 -static void test_ctype_##func(void)                            \
 -{                                                              \
 -       for (int i = 0; i < 256; i++) {                         \
 -               if (!check_int(func(i), ==, is_in(string, i)))  \
 -                       test_msg("       i: %02x", i);          \
 -        }                                                      \
 +#define TEST_CTYPE_FUNC(func, string) \
 +static void test_ctype_##func(void) { \
 +       for (int i = 0; i < 256; i++) { \
 +               if (!check_int(func(i), ==, is_in(string, i))) \
 +                       test_msg("       i: 0x%02x", i); \
 +       } \
   }
 +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
 +
  #define DIGIT "0123456789"
  #define LOWER "abcdefghijklmnopqrstuvwxyz"
  #define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 @@ -58,20 +59,20 @@ TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
  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");
 +       TEST_CHAR_CLASS(isspace);
 +       TEST_CHAR_CLASS(isdigit);
 +       TEST_CHAR_CLASS(isalpha);
 +       TEST_CHAR_CLASS(isalnum);
 +       TEST_CHAR_CLASS(is_glob_special);
 +       TEST_CHAR_CLASS(is_regex_special);
 +       TEST_CHAR_CLASS(is_pathspec_magic);
 +       TEST_CHAR_CLASS(isascii);
 +       TEST_CHAR_CLASS(islower);
 +       TEST_CHAR_CLASS(isupper);
 +       TEST_CHAR_CLASS(iscntrl);
 +       TEST_CHAR_CLASS(ispunct);
 +       TEST_CHAR_CLASS(isxdigit);
 +       TEST_CHAR_CLASS(isprint);
 Makefile               |  2 +-
 t/helper/test-ctype.c  | 70 -------------------------------------
 t/helper/test-tool.c   |  1 -
 t/helper/test-tool.h   |  1 -
 t/t0070-fundamental.sh |  4 ---
 t/unit-tests/t-ctype.c | 78 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 79 insertions(+), 77 deletions(-)
 delete mode 100644 t/helper/test-ctype.c
 create mode 100644 t/unit-tests/t-ctype.c
Show changes to 6 files +79 −78

Makefile, t/helper/test-ctype.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t0070-fundamental.sh, t/unit-tests/t-ctype.c

diff --git a/Makefile b/Makefile
index 88ba7a3c51..a4df48ba65 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-crontab.o
 TEST_BUILTINS_OBJS += test-csprng.o
-TEST_BUILTINS_OBJS += test-ctype.o
 TEST_BUILTINS_OBJS += test-date.o
 TEST_BUILTINS_OBJS += test-delta.o
 TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%

 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
-	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
-	rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
-	/*
-	 * We can't find NUL using strchr. Accept it as the first
-	 * character in the spec -- there are no empty classes.
-	 */
-	if (ch == '\0')
-		return ch == *s;
-	if (*s == '\0')
-		s++;
-	return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) {			\
-	int i;					\
-	for (i = 0; i < 256; i++) {		\
-		if (is_in(s, i) != t(i))	\
-			report_error(#t, i);	\
-	}					\
-	if (t(EOF))				\
-		report_error(#t, EOF);		\
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
-	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
-	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
-	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
-	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
-	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST_CLASS(isdigit, DIGIT);
-	TEST_CLASS(isspace, " \n\r\t");
-	TEST_CLASS(isalpha, LOWER UPPER);
-	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
-	TEST_CLASS(is_glob_special, "*?[\\");
-	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
-	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
-	TEST_CLASS(isascii, ASCII);
-	TEST_CLASS(islower, LOWER);
-	TEST_CLASS(isupper, UPPER);
-	TEST_CLASS(iscntrl, CNTRL);
-	TEST_CLASS(ispunct, PUNCT);
-	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
-	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
-	return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
 	{ "config", cmd__config },
 	{ "crontab", cmd__crontab },
 	{ "csprng", cmd__csprng },
-	{ "ctype", cmd__ctype },
 	{ "date", cmd__date },
 	{ "delta", cmd__delta },
 	{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__crontab(int argc, const char **argv);
 int cmd__csprng(int argc, const char **argv);
-int cmd__ctype(int argc, const char **argv);
 int cmd__date(int argc, const char **argv);
 int cmd__delta(int argc, const char **argv);
 int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh

-test_expect_success 'character classes (isspace, isalpha etc.)' '
-	test-tool ctype
-'
-
 test_expect_success 'mktemp to nonexistent directory prints filename' '
 	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
 	grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..3a338df541
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,78 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+	/*
+	 * We can't find NUL using strchr. Accept it as the first
+	 * character in the spec -- there are no empty classes.
+	 */
+	if (ch == '\0')
+		return ch == *s;
+	if (*s == '\0')
+		s++;
+	return !!strchr(s, ch);
+}
+
+/* Macro to test a character type */
+#define TEST_CTYPE_FUNC(func, string) \
+static void test_ctype_##func(void) { \
+	for (int i = 0; i < 256; i++) { \
+		if (!check_int(func(i), ==, is_in(string, i))) \
+			test_msg("       i: 0x%02x", i); \
+	} \
+}
+
+#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+int cmd_main(int argc, const char **argv) {
+	/* Run all character type tests */
+	TEST_CHAR_CLASS(isspace);
+	TEST_CHAR_CLASS(isdigit);
+	TEST_CHAR_CLASS(isalpha);
+	TEST_CHAR_CLASS(isalnum);
+	TEST_CHAR_CLASS(is_glob_special);
+	TEST_CHAR_CLASS(is_regex_special);
+	TEST_CHAR_CLASS(is_pathspec_magic);
+	TEST_CHAR_CLASS(isascii);
+	TEST_CHAR_CLASS(islower);
+	TEST_CHAR_CLASS(isupper);
+	TEST_CHAR_CLASS(iscntrl);
+	TEST_CHAR_CLASS(ispunct);
+	TEST_CHAR_CLASS(isxdigit);
+	TEST_CHAR_CLASS(isprint);
+
+	return test_done();
+}
--
2.42.0.windows.2
René Scharfe· Jan 7, 2024, 12:45 UTC · re: Achu Luma · lore

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

Am 05.01.24 um 17:14 schrieb Achu Luma:
Show 21 quoted lines
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
>
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
>
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
[snip]
Show 76 quoted lines
> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> deleted file mode 100644
> index e5659df40b..0000000000
> --- a/t/helper/test-ctype.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -#include "test-tool.h"
> -
> -static int rc;
> -
> -static void report_error(const char *class, int ch)
> -{
> -	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> -	rc = 1;
> -}
> -
> -static int is_in(const char *s, int ch)
> -{
> -	/*
> -	 * We can't find NUL using strchr. Accept it as the first
> -	 * character in the spec -- there are no empty classes.
> -	 */
> -	if (ch == '\0')
> -		return ch == *s;
> -	if (*s == '\0')
> -		s++;
> -	return !!strchr(s, ch);
> -}
> -
> -#define TEST_CLASS(t,s) {			\
> -	int i;					\
> -	for (i = 0; i < 256; i++) {		\
> -		if (is_in(s, i) != t(i))	\
> -			report_error(#t, i);	\
> -	}					\
> -	if (t(EOF))				\
> -		report_error(#t, EOF);		\
> -}
> -
> -#define DIGIT "0123456789"
> -#define LOWER "abcdefghijklmnopqrstuvwxyz"
> -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> -#define ASCII \
> -	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> -	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> -	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> -	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> -	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> -	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> -	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> -	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> -#define CNTRL \
> -	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> -	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> -	"\x7f"
> -
> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
> -{
> -	TEST_CLASS(isdigit, DIGIT);
> -	TEST_CLASS(isspace, " \n\r\t");
> -	TEST_CLASS(isalpha, LOWER UPPER);
> -	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
> -	TEST_CLASS(is_glob_special, "*?[\\");
> -	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
> -	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
> -	TEST_CLASS(isascii, ASCII);
> -	TEST_CLASS(islower, LOWER);
> -	TEST_CLASS(isupper, UPPER);
> -	TEST_CLASS(iscntrl, CNTRL);
> -	TEST_CLASS(ispunct, PUNCT);
> -	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
> -	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
> -
> -	return rc;
> -}
[snip]
Show 87 quoted lines
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> new file mode 100644
> index 0000000000..3a338df541
> --- /dev/null
> +++ b/t/unit-tests/t-ctype.c
> @@ -0,0 +1,78 @@
> +#include "test-lib.h"
> +
> +static int is_in(const char *s, int ch)
> +{
> +	/*
> +	 * We can't find NUL using strchr. Accept it as the first
> +	 * character in the spec -- there are no empty classes.
> +	 */
> +	if (ch == '\0')
> +		return ch == *s;
> +	if (*s == '\0')
> +		s++;
> +	return !!strchr(s, ch);
> +}
> +
> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string) \
> +static void test_ctype_##func(void) { \
> +	for (int i = 0; i < 256; i++) { \
> +		if (!check_int(func(i), ==, is_in(string, i))) \
> +			test_msg("       i: 0x%02x", i); \
> +	} \
> +}
> +
> +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
> +
> +#define DIGIT "0123456789"
> +#define LOWER "abcdefghijklmnopqrstuvwxyz"
> +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> +#define ASCII \
> +	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> +	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> +	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> +	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> +	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> +	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> +	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> +	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> +#define CNTRL \
> +	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> +	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> +	"\x7f"
> +
> +TEST_CTYPE_FUNC(isdigit, DIGIT)
> +TEST_CTYPE_FUNC(isspace, " \n\r\t")
> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
> +TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
> +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
> +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
> +TEST_CTYPE_FUNC(isascii, ASCII)
> +TEST_CTYPE_FUNC(islower, LOWER)
> +TEST_CTYPE_FUNC(isupper, UPPER)
> +TEST_CTYPE_FUNC(iscntrl, CNTRL)
> +TEST_CTYPE_FUNC(ispunct, PUNCT)
> +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
> +
> +int cmd_main(int argc, const char **argv) {
> +	/* Run all character type tests */
> +	TEST_CHAR_CLASS(isspace);
> +	TEST_CHAR_CLASS(isdigit);
> +	TEST_CHAR_CLASS(isalpha);
> +	TEST_CHAR_CLASS(isalnum);
> +	TEST_CHAR_CLASS(is_glob_special);
> +	TEST_CHAR_CLASS(is_regex_special);
> +	TEST_CHAR_CLASS(is_pathspec_magic);
> +	TEST_CHAR_CLASS(isascii);
> +	TEST_CHAR_CLASS(islower);
> +	TEST_CHAR_CLASS(isupper);
> +	TEST_CHAR_CLASS(iscntrl);
> +	TEST_CHAR_CLASS(ispunct);
> +	TEST_CHAR_CLASS(isxdigit);
> +	TEST_CHAR_CLASS(isprint);
> +
> +	return test_done();
> +}
> --
> 2.42.0.windows.2
>

Quite an improvement over v3! Now you only need to repeat the class names once.

Can we do any better? We could simply have one test per character per class like this:

#define TEST_CHAR_CLASS(class, expect) \
	for (int i = 0; i < 256; i++) \
		TEST(check_int(class(i), ==, is_in(expect, i)),	\
		     "%s(0x%02x) works", #class, i)
Which would be used like this:
	TEST_CHAR_CLASS(isspace, " \n\r\t");

With that there is no need to define any functions anymore. We also don't need any custom output, as the test name includes the character code. Downside: We'd have thousands of tests. But is that actually a downside or is that how the unit test framework is supposed to be used?

If we need to aggregate the results by class for some reason, we could use strings, like we already do for defining the expected class members. We need special handling for NUL, as that character terminates C strings, but we can put all other characters into a string and then use check_str:

#define TEST_CHAR_CLASS(class, expect) \
	do { \
		int expect_nul = expect[0] == '\0'; \
		char expect_rest[256] = {0}; \
		char actual_rest[256] = {0}; \
		for (int i = 1, j = 0; i < 256; i++) \
			if (strchr(&expect[expect_nul], i)) \
				expect_rest[j++] = i; \
		for (int i = 1, j = 0; i < 256; i++) \
			if (class(i) == 1) \
				actual_rest[j++] = i; \
		TEST(check_int(class(0), ==, expect_nul) && \
		     check_str(actual_rest, expect_rest), \
		     #class " works"); \
	} while (0)

check_str escapes non-printable characters when reporting a mismatch, so this shouldn't mess up your terminal.

By the way: Like the original code these checks are stricter than required by the C standard in requiring the result to be 1 instead of just true (any non-zero value). Perhaps they should be relaxed. But that's a tangent and independent of the convergence to a unit test.

René
Junio C Hamano· Jan 8, 2024, 22:32 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
> Quite an improvement over v3!  Now you only need to repeat the class
> names once.
>
> Can we do any better?
;-)
Phillip Wood· Jan 9, 2024, 10:35 UTC · re: Achu Luma · lore

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

Hi Achu
On 05/01/2024 16:14, Achu Luma wrote:
Show 30 quoted lines
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
> 
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
> 
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
>   The changes between version 3 and version 4 are the following:
> 
>   - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro.
>   - A "0x"prefix has been added to avoid confusion between decimal and
>     hexadecimal codes printed by test_msg().
>   - The "Mentored-by:..." trailer has been restored.
>   - "works as expected" has been reduced to just "works" as suggested by Taylor.
>   - Some "Helped-by: ..." trailers have been added.
>   - Some whitespace fixes have been made.

Thanks for the re-roll, the changes look good, there is one issue that I spotted below

Show 9 quoted lines
> -#define TEST_CLASS(t,s) {			\
> -	int i;					\
> -	for (i = 0; i < 256; i++) {		\
> -		if (is_in(s, i) != t(i))	\
> -			report_error(#t, i);	\
> -	}					\
> -	if (t(EOF))				\
> -		report_error(#t, EOF);		\
> -}
In the old version we test EOF in addition to each character between 0-255
Show 8 quoted lines
> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string) \
> +static void test_ctype_##func(void) { \
> +	for (int i = 0; i < 256; i++) { \
> +		if (!check_int(func(i), ==, is_in(string, i))) \
> +			test_msg("       i: 0x%02x", i); \
> +	} \
> +}

In the new version we only test the characters 0-255, not EOF. If there is a good reason for removing the EOF tests then it should be explained in the commit message. If not it would be good to add those tests back.

Best Wishes
Phillip
Junio C Hamano· Jan 9, 2024, 17:12 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
> In the new version we only test the characters 0-255, not EOF. If
> there is a good reason for removing the EOF tests then it should be
> explained in the commit message. If not it would be good to add those
> tests back.
Thanks for sharp eyes.  Didn't notice the handling of EOF myself.
Achu Luma· Jan 12, 2024, 10:27 UTC · re: Achu Luma · lore

[Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c

In the recent codebase update (8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework.

This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h).

The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: René Scharfe <l.s.r@web.de>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 The change between version 4 and version 5 is:
 - Added tests to handle EOF.
 Thanks to Phillip for noticing the missing tests..
 Here is a diff between v4 and v5:
 +       if (!check(!func(EOF))) \
 +                       test_msg("      i: 0x%02x (EOF)", EOF); \
 Thanks also to René, Phillip, Junio and Taylor who helped with
 previous versions.
 Makefile               |  2 +-
 t/helper/test-ctype.c  | 70 ------------------------------------
 t/helper/test-tool.c   |  1 -
 t/helper/test-tool.h   |  1 -
 t/t0070-fundamental.sh |  4 ---
 t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 81 insertions(+), 77 deletions(-)
 delete mode 100644 t/helper/test-ctype.c
 create mode 100644 t/unit-tests/t-ctype.c
Show changes to 6 files +81 −78

Makefile, t/helper/test-ctype.c, t/helper/test-tool.c, t/helper/test-tool.h, t/t0070-fundamental.sh, t/unit-tests/t-ctype.c

diff --git a/Makefile b/Makefile
index 15990ff312..1a62e48759 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-crontab.o
 TEST_BUILTINS_OBJS += test-csprng.o
-TEST_BUILTINS_OBJS += test-ctype.o
 TEST_BUILTINS_OBJS += test-date.o
 TEST_BUILTINS_OBJS += test-delta.o
 TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
-	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
-	rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
-	/*
-	 * We can't find NUL using strchr. Accept it as the first
-	 * character in the spec -- there are no empty classes.
-	 */
-	if (ch == '\0')
-		return ch == *s;
-	if (*s == '\0')
-		s++;
-	return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) {			\
-	int i;					\
-	for (i = 0; i < 256; i++) {		\
-		if (is_in(s, i) != t(i))	\
-			report_error(#t, i);	\
-	}					\
-	if (t(EOF))				\
-		report_error(#t, EOF);		\
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
-	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
-	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
-	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
-	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
-	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST_CLASS(isdigit, DIGIT);
-	TEST_CLASS(isspace, " \n\r\t");
-	TEST_CLASS(isalpha, LOWER UPPER);
-	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
-	TEST_CLASS(is_glob_special, "*?[\\");
-	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
-	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
-	TEST_CLASS(isascii, ASCII);
-	TEST_CLASS(islower, LOWER);
-	TEST_CLASS(isupper, UPPER);
-	TEST_CLASS(iscntrl, CNTRL);
-	TEST_CLASS(ispunct, PUNCT);
-	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
-	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
-	return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
 	{ "config", cmd__config },
 	{ "crontab", cmd__crontab },
 	{ "csprng", cmd__csprng },
-	{ "ctype", cmd__ctype },
 	{ "date", cmd__date },
 	{ "delta", cmd__delta },
 	{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__crontab(int argc, const char **argv);
 int cmd__csprng(int argc, const char **argv);
-int cmd__ctype(int argc, const char **argv);
 int cmd__date(int argc, const char **argv);
 int cmd__delta(int argc, const char **argv);
 int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh

-test_expect_success 'character classes (isspace, isalpha etc.)' '
-	test-tool ctype
-'
-
 test_expect_success 'mktemp to nonexistent directory prints filename' '
 	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
 	grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..f315489984
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,80 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+	/*
+	 * We can't find NUL using strchr. Accept it as the first
+	 * character in the spec -- there are no empty classes.
+	 */
+	if (ch == '\0')
+		return ch == *s;
+	if (*s == '\0')
+		s++;
+	return !!strchr(s, ch);
+}
+
+/* Macro to test a character type */
+#define TEST_CTYPE_FUNC(func, string) \
+static void test_ctype_##func(void) { \
+	for (int i = 0; i < 256; i++) { \
+		if (!check_int(func(i), ==, is_in(string, i))) \
+			test_msg("       i: 0x%02x", i); \
+	} \
+	if (!check(!func(EOF))) \
+			test_msg("      i: 0x%02x (EOF)", EOF); \
+}
+
+#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+int cmd_main(int argc, const char **argv) {
+	/* Run all character type tests */
+	TEST_CHAR_CLASS(isspace);
+	TEST_CHAR_CLASS(isdigit);
+	TEST_CHAR_CLASS(isalpha);
+	TEST_CHAR_CLASS(isalnum);
+	TEST_CHAR_CLASS(is_glob_special);
+	TEST_CHAR_CLASS(is_regex_special);
+	TEST_CHAR_CLASS(is_pathspec_magic);
+	TEST_CHAR_CLASS(isascii);
+	TEST_CHAR_CLASS(islower);
+	TEST_CHAR_CLASS(isupper);
+	TEST_CHAR_CLASS(iscntrl);
+	TEST_CHAR_CLASS(ispunct);
+	TEST_CHAR_CLASS(isxdigit);
+	TEST_CHAR_CLASS(isprint);
+
+	return test_done();
+}
--
2.42.0.windows.2
Phillip Wood· Jan 15, 2024, 10:39 UTC · re: Achu Luma · lore

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

Hi Achu
On 12/01/2024 10:27, Achu Luma wrote:
Show 29 quoted lines
> In the recent codebase update (8bf6fbd00d (Merge branch
> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> merged, providing a standardized approach for testing C code. Prior to
> this update, some unit tests relied on the test helper mechanism,
> lacking a dedicated unit testing framework. It's more natural to perform
> these unit tests using the new unit test framework.
> 
> This commit migrates the unit tests for C character classification
> functions (isdigit(), isspace(), etc) from the legacy approach
> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
> to the new unit testing framework (t/unit-tests/test-lib.h).
> 
> The migration involves refactoring the tests to utilize the testing
> macros provided by the framework (TEST() and check_*()).
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Helped-by: Taylor Blau <me@ttaylorr.com>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
>   The change between version 4 and version 5 is:
>   - Added tests to handle EOF.
> 
>   Thanks to Phillip for noticing the missing tests..
>   Here is a diff between v4 and v5:
> 
>   +       if (!check(!func(EOF))) \
>   +                       test_msg("      i: 0x%02x (EOF)", EOF); \
Thanks for adding back the test for EOF, this version looks good to me.
Best Wishes
Phillip
Show 238 quoted lines
>   Thanks also to René, Phillip, Junio and Taylor who helped with
>   previous versions.
> 
>   Makefile               |  2 +-
>   t/helper/test-ctype.c  | 70 ------------------------------------
>   t/helper/test-tool.c   |  1 -
>   t/helper/test-tool.h   |  1 -
>   t/t0070-fundamental.sh |  4 ---
>   t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 81 insertions(+), 77 deletions(-)
>   delete mode 100644 t/helper/test-ctype.c
>   create mode 100644 t/unit-tests/t-ctype.c
> 
> diff --git a/Makefile b/Makefile
> index 15990ff312..1a62e48759 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
>   TEST_BUILTINS_OBJS += test-config.o
>   TEST_BUILTINS_OBJS += test-crontab.o
>   TEST_BUILTINS_OBJS += test-csprng.o
> -TEST_BUILTINS_OBJS += test-ctype.o
>   TEST_BUILTINS_OBJS += test-date.o
>   TEST_BUILTINS_OBJS += test-delta.o
>   TEST_BUILTINS_OBJS += test-dir-iterator.o
> @@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
>   UNIT_TEST_PROGRAMS += t-basic
>   UNIT_TEST_PROGRAMS += t-mem-pool
>   UNIT_TEST_PROGRAMS += t-strbuf
> +UNIT_TEST_PROGRAMS += t-ctype
>   UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
>   UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
>   UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> deleted file mode 100644
> index e5659df40b..0000000000
> --- a/t/helper/test-ctype.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -#include "test-tool.h"
> -
> -static int rc;
> -
> -static void report_error(const char *class, int ch)
> -{
> -	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> -	rc = 1;
> -}
> -
> -static int is_in(const char *s, int ch)
> -{
> -	/*
> -	 * We can't find NUL using strchr. Accept it as the first
> -	 * character in the spec -- there are no empty classes.
> -	 */
> -	if (ch == '\0')
> -		return ch == *s;
> -	if (*s == '\0')
> -		s++;
> -	return !!strchr(s, ch);
> -}
> -
> -#define TEST_CLASS(t,s) {			\
> -	int i;					\
> -	for (i = 0; i < 256; i++) {		\
> -		if (is_in(s, i) != t(i))	\
> -			report_error(#t, i);	\
> -	}					\
> -	if (t(EOF))				\
> -		report_error(#t, EOF);		\
> -}
> -
> -#define DIGIT "0123456789"
> -#define LOWER "abcdefghijklmnopqrstuvwxyz"
> -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> -#define ASCII \
> -	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> -	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> -	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> -	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> -	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> -	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> -	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> -	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> -#define CNTRL \
> -	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> -	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> -	"\x7f"
> -
> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
> -{
> -	TEST_CLASS(isdigit, DIGIT);
> -	TEST_CLASS(isspace, " \n\r\t");
> -	TEST_CLASS(isalpha, LOWER UPPER);
> -	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
> -	TEST_CLASS(is_glob_special, "*?[\\");
> -	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
> -	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
> -	TEST_CLASS(isascii, ASCII);
> -	TEST_CLASS(islower, LOWER);
> -	TEST_CLASS(isupper, UPPER);
> -	TEST_CLASS(iscntrl, CNTRL);
> -	TEST_CLASS(ispunct, PUNCT);
> -	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
> -	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
> -
> -	return rc;
> -}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 37ba996539..33b9501c21 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
>   	{ "config", cmd__config },
>   	{ "crontab", cmd__crontab },
>   	{ "csprng", cmd__csprng },
> -	{ "ctype", cmd__ctype },
>   	{ "date", cmd__date },
>   	{ "delta", cmd__delta },
>   	{ "dir-iterator", cmd__dir_iterator },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 8a1a7c63da..b72f07ded9 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
>   int cmd__config(int argc, const char **argv);
>   int cmd__crontab(int argc, const char **argv);
>   int cmd__csprng(int argc, const char **argv);
> -int cmd__ctype(int argc, const char **argv);
>   int cmd__date(int argc, const char **argv);
>   int cmd__delta(int argc, const char **argv);
>   int cmd__dir_iterator(int argc, const char **argv);
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index 487bc8d905..a4756fbab9 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
>   TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
> 
> -test_expect_success 'character classes (isspace, isalpha etc.)' '
> -	test-tool ctype
> -'
> -
>   test_expect_success 'mktemp to nonexistent directory prints filename' '
>   	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
>   	grep "doesnotexist/test" err
> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> new file mode 100644
> index 0000000000..f315489984
> --- /dev/null
> +++ b/t/unit-tests/t-ctype.c
> @@ -0,0 +1,80 @@
> +#include "test-lib.h"
> +
> +static int is_in(const char *s, int ch)
> +{
> +	/*
> +	 * We can't find NUL using strchr. Accept it as the first
> +	 * character in the spec -- there are no empty classes.
> +	 */
> +	if (ch == '\0')
> +		return ch == *s;
> +	if (*s == '\0')
> +		s++;
> +	return !!strchr(s, ch);
> +}
> +
> +/* Macro to test a character type */
> +#define TEST_CTYPE_FUNC(func, string) \
> +static void test_ctype_##func(void) { \
> +	for (int i = 0; i < 256; i++) { \
> +		if (!check_int(func(i), ==, is_in(string, i))) \
> +			test_msg("       i: 0x%02x", i); \
> +	} \
> +	if (!check(!func(EOF))) \
> +			test_msg("      i: 0x%02x (EOF)", EOF); \
> +}
> +
> +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
> +
> +#define DIGIT "0123456789"
> +#define LOWER "abcdefghijklmnopqrstuvwxyz"
> +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
> +#define ASCII \
> +	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> +	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> +	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
> +	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
> +	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
> +	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
> +	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> +	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> +#define CNTRL \
> +	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
> +	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
> +	"\x7f"
> +
> +TEST_CTYPE_FUNC(isdigit, DIGIT)
> +TEST_CTYPE_FUNC(isspace, " \n\r\t")
> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
> +TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
> +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
> +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
> +TEST_CTYPE_FUNC(isascii, ASCII)
> +TEST_CTYPE_FUNC(islower, LOWER)
> +TEST_CTYPE_FUNC(isupper, UPPER)
> +TEST_CTYPE_FUNC(iscntrl, CNTRL)
> +TEST_CTYPE_FUNC(ispunct, PUNCT)
> +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
> +
> +int cmd_main(int argc, const char **argv) {
> +	/* Run all character type tests */
> +	TEST_CHAR_CLASS(isspace);
> +	TEST_CHAR_CLASS(isdigit);
> +	TEST_CHAR_CLASS(isalpha);
> +	TEST_CHAR_CLASS(isalnum);
> +	TEST_CHAR_CLASS(is_glob_special);
> +	TEST_CHAR_CLASS(is_regex_special);
> +	TEST_CHAR_CLASS(is_pathspec_magic);
> +	TEST_CHAR_CLASS(isascii);
> +	TEST_CHAR_CLASS(islower);
> +	TEST_CHAR_CLASS(isupper);
> +	TEST_CHAR_CLASS(iscntrl);
> +	TEST_CHAR_CLASS(ispunct);
> +	TEST_CHAR_CLASS(isxdigit);
> +	TEST_CHAR_CLASS(isprint);
> +
> +	return test_done();
> +}
> --
> 2.42.0.windows.2
> 
> 
Junio C Hamano· Jan 16, 2024, 15:38 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
> Thanks for adding back the test for EOF, this version looks good to me.
Thanks.  Let's merge it to 'next'.
René Scharfe· Jan 16, 2024, 19:27 UTC · re: Junio C Hamano · lore

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

Am 16.01.24 um 16:38 schrieb Junio C Hamano:
Show 5 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> Thanks for adding back the test for EOF, this version looks good to me.
>
> Thanks.  Let's merge it to 'next'.

OK. I'm still interested in replies to my question in https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/, i.e. whether we should have one TEST per class or one per class and character -- or in a broader sense: What's the ideal scope of a TEST? But I can ask it again in the form of a follow-up patch.

René
Christian Couder· Jan 16, 2024, 19:45 UTC · re: René Scharfe · lore

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

On Tue, Jan 16, 2024 at 8:27 PM René Scharfe <l.s.r@web.de> wrote:
Show 13 quoted lines
>
> Am 16.01.24 um 16:38 schrieb Junio C Hamano:
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> >> Thanks for adding back the test for EOF, this version looks good to me.
> >
> > Thanks.  Let's merge it to 'next'.
>
> OK.  I'm still interested in replies to my question in
> https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/,
> i.e. whether we should have one TEST per class or one per class and
> character -- or in a broader sense: What's the ideal scope of a TEST?
> But I can ask it again in the form of a follow-up patch.

I think one test per character per class would result in too much detail in the output. Other than that I think it's better to address your questions to the designers of the unit test framework rather than to the authors of this patch. And yeah, sending a follow up patch would perhaps be the best. Thanks.

Junio C Hamano· Jan 16, 2024, 19:52 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 12 quoted lines
> Am 16.01.24 um 16:38 schrieb Junio C Hamano:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>> Thanks for adding back the test for EOF, this version looks good to me.
>>
>> Thanks.  Let's merge it to 'next'.
>
> OK.  I'm still interested in replies to my question in
> https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/,
> i.e. whether we should have one TEST per class or one per class and
> character -- or in a broader sense: What's the ideal scope of a TEST?
> But I can ask it again in the form of a follow-up patch.

I personally do not have a good answer, but those who are interested in unit-tests more than I do should have their opinions to share ;-)

Josh Steadmon· Jan 17, 2024, 05:37 UTC · re: René Scharfe · lore

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

On 2024.01.16 20:27, René Scharfe wrote:
Show 14 quoted lines
> Am 16.01.24 um 16:38 schrieb Junio C Hamano:
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> >> Thanks for adding back the test for EOF, this version looks good to me.
> >
> > Thanks.  Let's merge it to 'next'.
> 
> OK.  I'm still interested in replies to my question in
> https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/,
> i.e. whether we should have one TEST per class or one per class and
> character -- or in a broader sense: What's the ideal scope of a TEST?
> But I can ask it again in the form of a follow-up patch.
> 
> René

I think that the scope of a TEST() should tend small: we want the minimal amount of setup required to test the invariants that we're interested in. For this particular unit test, since we're just testing simple predicates on static sets of characters, I would be OK seeing one TEST() per class/character. That would certainly make this unit test an outlier in the number of checks, but I'm less worried about that since this is testing system-provided functions that we don't expect to change regularly.

Additionally, the elimination of a level of macro indirection makes this more readable IMO.

← back to recent threads