{"thread":{"id":"60649","subject":"[PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","startedAt":"2023-12-21T23:16:02Z","lastAt":"2024-01-17T05:37:15Z","messageCount":24,"participants":["Achu Luma","Junio C Hamano","Christian Couder","René Scharfe","Phillip Wood","Taylor Blau","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485969","messageId":"20231221231527.8130-1-ach.lumap@gmail.com","threadId":"60649","inReplyTo":null,"subject":"[PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2023-12-21T23:15:27Z","receivedAt":"2023-12-21T23:16:02Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"In the recent codebase update (8bf6fbd00d (Merge branch\n'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\nmerged, providing a standardized approach for testing C code. Prior to\nthis update, some unit tests relied on the test helper mechanism,\nlacking a dedicated unit testing framework. It's more natural to perform\nthese unit tests using the new unit test framework.\n\nThis commit migrates the unit tests for C character classification\nfunctions (isdigit(), isspace(), etc) from the legacy approach\nusing the test-tool command `test-tool ctype` in t/helper/test-ctype.c\nto the new unit testing framework (t/unit-tests/test-lib.h).\n\nThe migration involves refactoring the tests to utilize the testing\nmacros provided by the framework (TEST() and check_*()).\n\nMentored-by: Christian Couder \nSigned-off-by: Achu Luma \n---\n Makefile | 2 +-\n t/helper/test-ctype.c | 70 --------------------------------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0070-fundamental.sh | 4 ---\n t/unit-tests/t-ctype.c | 76 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 77 insertions(+), 77 deletions(-)\n delete mode 100644 t/helper/test-ctype.c\n create mode 100644 t/unit-tests/t-ctype.c\n\ndiff --git a/Makefile b/Makefile\nindex 88ba7a3c51..a4df48ba65 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\n TEST_BUILTINS_OBJS += test-csprng.o\n-TEST_BUILTINS_OBJS += test-ctype.o\n TEST_BUILTINS_OBJS += test-date.o\n TEST_BUILTINS_OBJS += test-delta.o\n TEST_BUILTINS_OBJS += test-dir-iterator.o\n@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-basic\n UNIT_TEST_PROGRAMS += t-strbuf\n+UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\ndiff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\ndeleted file mode 100644\nindex e5659df40b..0000000000\n--- a/t/helper/test-ctype.c\n+++ /dev/null\n@@ -1,70 +0,0 @@\n-#include \"test-tool.h\"\n-\n-static int rc;\n-\n-static void report_error(const char *class, int ch)\n-{\n-\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n-\trc = 1;\n-}\n-\n-static int is_in(const char *s, int ch)\n-{\n-\t/*\n-\t * We can't find NUL using strchr. Accept it as the first\n-\t * character in the spec -- there are no empty classes.\n-\t */\n-\tif (ch == '\\0')\n-\t\treturn ch == *s;\n-\tif (*s == '\\0')\n-\t\ts++;\n-\treturn !!strchr(s, ch);\n-}\n-\n-#define TEST_CLASS(t,s) {\t\t\t\\\n-\tint i;\t\t\t\t\t\\\n-\tfor (i = 0; i < 256; i++) {\t\t\\\n-\t\tif (is_in(s, i) != t(i))\t\\\n-\t\t\treport_error(#t, i);\t\\\n-\t}\t\t\t\t\t\\\n-\tif (t(EOF))\t\t\t\t\\\n-\t\treport_error(#t, EOF);\t\t\\\n-}\n-\n-#define DIGIT \"0123456789\"\n-#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n-#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n-#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n-#define ASCII \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n-\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n-\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n-\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n-\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n-\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n-#define CNTRL \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x7f\"\n-\n-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tTEST_CLASS(isdigit, DIGIT);\n-\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n-\tTEST_CLASS(isalpha, LOWER UPPER);\n-\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n-\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n-\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n-\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n-\tTEST_CLASS(isascii, ASCII);\n-\tTEST_CLASS(islower, LOWER);\n-\tTEST_CLASS(isupper, UPPER);\n-\tTEST_CLASS(iscntrl, CNTRL);\n-\tTEST_CLASS(ispunct, PUNCT);\n-\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n-\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n-\n-\treturn rc;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 37ba996539..33b9501c21 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\n \t{ \"csprng\", cmd__csprng },\n-\t{ \"ctype\", cmd__ctype },\n \t{ \"date\", cmd__date },\n \t{ \"delta\", cmd__delta },\n \t{ \"dir-iterator\", cmd__dir_iterator },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 8a1a7c63da..b72f07ded9 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\n int cmd__csprng(int argc, const char **argv);\n-int cmd__ctype(int argc, const char **argv);\n int cmd__date(int argc, const char **argv);\n int cmd__delta(int argc, const char **argv);\n int cmd__dir_iterator(int argc, const char **argv);\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 487bc8d905..a4756fbab9 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-test_expect_success 'character classes (isspace, isalpha etc.)' '\n-\ttest-tool ctype\n-'\n-\n test_expect_success 'mktemp to nonexistent directory prints filename' '\n \ttest_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&\n \tgrep \"doesnotexist/test\" err\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nnew file mode 100644\nindex 0000000000..41189ba9f9\n--- /dev/null\n+++ b/t/unit-tests/t-ctype.c\n@@ -0,0 +1,76 @@\n+#include \"test-lib.h\"\n+\n+static int is_in(const char *s, int ch)\n+{\n+\t/*\n+\t * We can't find NUL using strchr. Accept it as the first\n+\t * character in the spec -- there are no empty classes.\n+\t */\n+\tif (ch == '\\0')\n+\t\treturn ch == *s;\n+\tif (*s == '\\0')\n+\t\ts++;\n+\treturn !!strchr(s, ch);\n+}\n+\n+/* Macro to test a character type */\n+#define TEST_CTYPE_FUNC(func, string)\t\t\t\\\n+static void test_ctype_##func(void)\t\t\t\t\\\n+{\t\t\t\t\t\t\t\t\\\n+\tint i; \t \t\\\n+\tfor (i = 0; i < 256; i++) \t\t\\\n+\t\tcheck_int(func(i), ==, is_in(string, i)); \t\\\n+}\n+\n+#define DIGIT \"0123456789\"\n+#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n+#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n+#define ASCII \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n+\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n+\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n+\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n+\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n+\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n+#define CNTRL \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x7f\"\n+\n+TEST_CTYPE_FUNC(isdigit, DIGIT)\n+TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n+TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n+TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n+TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n+TEST_CTYPE_FUNC(isascii, ASCII)\n+TEST_CTYPE_FUNC(islower, LOWER)\n+TEST_CTYPE_FUNC(isupper, UPPER)\n+TEST_CTYPE_FUNC(iscntrl, CNTRL)\n+TEST_CTYPE_FUNC(ispunct, PUNCT)\n+TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n+\n+int cmd_main(int argc, const char **argv) {\n+\t/* Run all character type tests */\n+\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n+\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n+\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n+\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n+\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n+\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n+\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n+\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n+\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n+\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n+\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n+\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n+\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n+\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n+\n+\treturn test_done();\n+}\n\nbase-commit: 055bb6e9969085777b7fab83e3fee0017654f134\n-- \n2.42.0.windows.2\n\n"},{"id":"486032","messageId":"xmqqsf3ohkew.fsf@gitster.g","threadId":"60649","inReplyTo":"20231221231527.8130-1-ach.lumap@gmail.com","subject":"Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T18:45:59Z","receivedAt":"2023-12-26T18:46:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Achu Luma writes:\n\n> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\n> deleted file mode 100644\n> index e5659df40b..0000000000\n> --- a/t/helper/test-ctype.c\n> +++ /dev/null\n> @@ -1,70 +0,0 @@\n> -#include \"test-tool.h\"\n> -\n> -static int rc;\n> -\n> -static void report_error(const char *class, int ch)\n> -{\n> -\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n> -\trc = 1;\n> -}\n\nSo, if we have a is_foo() that characterises a byte that ought to\nbe \"foo\" but gets miscategorised not to be \"foo\", we used to\npinpoint exactly the byte value that was an issue. We did not do\nany early return ...\n\n> ...\n> -#define TEST_CLASS(t,s) {\t\t\t\\\n> -\tint i;\t\t\t\t\t\\\n> -\tfor (i = 0; i < 256; i++) {\t\t\\\n> -\t\tif (is_in(s, i) != t(i))\t\\\n> -\t\t\treport_error(#t, i);\t\\\n> -\t}\t\t\t\t\t\\\n> -\tif (t(EOF))\t\t\t\t\\\n> -\t\treport_error(#t, EOF);\t\t\\\n> -}\n\n... and reported for all errors in the \"class\".\n\n> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n> new file mode 100644\n> index 0000000000..41189ba9f9\n> --- /dev/null\n> +++ b/t/unit-tests/t-ctype.c\n> @@ -0,0 +1,76 @@\n> +#include \"test-lib.h\"\n> +\n> +static int is_in(const char *s, int ch)\n> +{\n> +\t/*\n> +\t * We can't find NUL using strchr. Accept it as the first\n> +\t * character in the spec -- there are no empty classes.\n> +\t */\n> +\tif (ch == '\\0')\n> +\t\treturn ch == *s;\n> +\tif (*s == '\\0')\n> +\t\ts++;\n> +\treturn !!strchr(s, ch);\n> +}\n> +\n> +/* Macro to test a character type */\n> +#define TEST_CTYPE_FUNC(func, string)\t\t\t\\\n> +static void test_ctype_##func(void)\t\t\t\t\\\n> +{\t\t\t\t\t\t\t\t\\\n> +\tint i; \t \t\\\n> +\tfor (i = 0; i < 256; i++) \t\t\\\n> +\t\tcheck_int(func(i), ==, is_in(string, i)); \t\\\n> +}\n\nNow, we let check_int() to do the checking for each and every byte\nvalue for the class. check_int() uses different reporting and shows\nthe problematic value in a way that is more verbose and at the same\ntime is a less specific and harder to understand:\n\n\t\ttest_msg(\" left: %\"PRIdMAX, a);\n\t\ttest_msg(\" right: %\"PRIdMAX, b);\n\nBut that is probably the price to pay to use a more generic\nframework, I guess.\n\n> +int cmd_main(int argc, const char **argv) {\n> +\t/* Run all character type tests */\n> +\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n> +\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n> +\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n> +\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n> +\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n> +\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n> +\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n> +\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n> +\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n> +\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n> +\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n> +\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n> +\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n> +\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n> +\n> +\treturn test_done();\n> +}\n\nAs a practice to use the unit-tests framework, the patch looks OK.\nhelper/test-ctype.c indeed is an oddball that runs once and checks\neverything it wants to check, for which the unit tests framework is\nmuch more suited.\n\nLet's see how others react and then queue.\n\nThanks.\n"},{"id":"486054","messageId":"CAP8UFD1vC6=7ESx1w7S9Q9P0Bsc+c03wgHToNBaP+ivvm9BKBg@mail.gmail.com","threadId":"60649","inReplyTo":"xmqqsf3ohkew.fsf@gitster.g","subject":"Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-12-27T10:57:23Z","receivedAt":"2023-12-27T10:57:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano wrote:\n>\n> Achu Luma writes:\n\n> > +/* Macro to test a character type */\n> > +#define TEST_CTYPE_FUNC(func, string) \\\n> > +static void test_ctype_##func(void) \\\n> > +{ \\\n> > + int i; \\\n> > + for (i = 0; i < 256; i++) \\\n> > + check_int(func(i), ==, is_in(string, i)); \\\n> > +}\n>\n> Now, we let check_int() to do the checking for each and every byte\n> value for the class. check_int() uses different reporting and shows\n> the problematic value in a way that is more verbose and at the same\n> time is a less specific and harder to understand:\n>\n> test_msg(\" left: %\"PRIdMAX, a);\n> test_msg(\" right: %\"PRIdMAX, b);\n>\n> But that is probably the price to pay to use a more generic\n> framework, I guess.\n\nI have added Phillip and Josh in Cc: as they might have ideas about this.\n\nAlso it might not be a big issue here, but when the new unit test\nframework was proposed, I commented on the fact that \"left\" and\n\"right\" were perhaps a bit less explicit than \"actual\" and \"expected\".\n\n> > +int cmd_main(int argc, const char **argv) {\n> > + /* Run all character type tests */\n> > + TEST(test_ctype_isspace(), \"isspace() works as we expect\");\n> > + TEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n> > + TEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n> > + TEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n> > + TEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n> > + TEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n> > + TEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n> > + TEST(test_ctype_isascii(), \"isascii() works as we expect\");\n> > + TEST(test_ctype_islower(), \"islower() works as we expect\");\n> > + TEST(test_ctype_isupper(), \"isupper() works as we expect\");\n> > + TEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n> > + TEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n> > + TEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n> > + TEST(test_ctype_isprint(), \"isprint() works as we expect\");\n> > +\n> > + return test_done();\n> > +}\n>\n> As a practice to use the unit-tests framework, the patch looks OK.\n> helper/test-ctype.c indeed is an oddball that runs once and checks\n> everything it wants to check, for which the unit tests framework is\n> much more suited.\n\nYeah, I agree.\n\n> Let's see how others react and then queue.\n>\n> Thanks.\n"},{"id":"486055","messageId":"f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de","threadId":"60649","inReplyTo":"CAP8UFD1vC6=7ESx1w7S9Q9P0Bsc+c03wgHToNBaP+ivvm9BKBg@mail.gmail.com","subject":"Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-27T11:57:26Z","receivedAt":"2023-12-27T11:57:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.12.23 um 11:57 schrieb Christian Couder:\n> On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano wrote:\n>>\n>> Achu Luma writes:\n>\n>>> +/* Macro to test a character type */\n>>> +#define TEST_CTYPE_FUNC(func, string) \\\n>>> +static void test_ctype_##func(void) \\\n>>> +{ \\\n>>> + int i; \\\n>>> + for (i = 0; i < 256; i++) \\\n>>> + check_int(func(i), ==, is_in(string, i)); \\\n>>> +}\n>>\n>> Now, we let check_int() to do the checking for each and every byte\n>> value for the class. check_int() uses different reporting and shows\n>> the problematic value in a way that is more verbose and at the same\n>> time is a less specific and harder to understand:\n>>\n>> test_msg(\" left: %\"PRIdMAX, a);\n>> test_msg(\" right: %\"PRIdMAX, b);\n>>\n>> But that is probably the price to pay to use a more generic\n>> framework, I guess.\n>\n> I have added Phillip and Josh in Cc: as they might have ideas about this.\n\nYou can write custom messages for custom tests using test_assert().\n\n> Also it might not be a big issue here, but when the new unit test\n> framework was proposed, I commented on the fact that \"left\" and\n> \"right\" were perhaps a bit less explicit than \"actual\" and \"expected\".\n\nTrue.\n\n>>> +int cmd_main(int argc, const char **argv) {\n>>> + /* Run all character type tests */\n>>> + TEST(test_ctype_isspace(), \"isspace() works as we expect\");\n>>> + TEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n>>> + TEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n>>> + TEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n>>> + TEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n>>> + TEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n>>> + TEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n>>> + TEST(test_ctype_isascii(), \"isascii() works as we expect\");\n>>> + TEST(test_ctype_islower(), \"islower() works as we expect\");\n>>> + TEST(test_ctype_isupper(), \"isupper() works as we expect\");\n>>> + TEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n>>> + TEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n>>> + TEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n>>> + TEST(test_ctype_isprint(), \"isprint() works as we expect\");\n>>> +\n>>> + return test_done();\n>>> +}\n>>\n>> As a practice to use the unit-tests framework, the patch looks OK.\n\nThe added repetition is a bit grating. With a bit of setup, loop\nunrolling and stringification you can retain the property of only having\nto mention the class name once. Demo patch below.\n\n>> helper/test-ctype.c indeed is an oddball that runs once and checks\n>> everything it wants to check, for which the unit tests framework is\n>> much more suited.\n>\n> Yeah, I agree.\n\nIndeed: test-ctype does unit tests, so the unit test framework should\nbe a perfect fit. It still feels a bit raw that this point, though,\nbut that's to be expected.\n\nRené\n\n\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex 41189ba9f9..7903856cec 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -13,13 +13,23 @@ static int is_in(const char *s, int ch)\n \treturn !!strchr(s, ch);\n }\n\n-/* Macro to test a character type */\n-#define TEST_CTYPE_FUNC(func, string)\t\t\t\\\n-static void test_ctype_##func(void)\t\t\t\t\\\n-{\t\t\t\t\t\t\t\t\\\n-\tint i; \t \t\\\n-\tfor (i = 0; i < 256; i++) \t\t\\\n-\t\tcheck_int(func(i), ==, is_in(string, i)); \t\\\n+struct ctype {\n+\tconst char *name;\n+\tconst char *expect;\n+\tint actual[256];\n+};\n+\n+static void test_ctype(const struct ctype *class)\n+{\n+\tfor (int i = 0; i < 256; i++) {\n+\t\tint expect = is_in(class->expect, i);\n+\t\tint actual = class->actual[i];\n+\t\tint res = test_assert(TEST_LOCATION(), class->name,\n+\t\t\t\t actual == expect);\n+\t\tif (!res)\n+\t\t\ttest_msg(\"%s classifies char %d (0x%02x) wrongly\",\n+\t\t\t\t class->name, i, i);\n+\t}\n }\n\n #define DIGIT \"0123456789\"\n@@ -40,37 +50,39 @@ static void test_ctype_##func(void)\t\t\t\t\\\n \t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n \t\"\\x7f\"\n\n-TEST_CTYPE_FUNC(isdigit, DIGIT)\n-TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n-TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n-TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n-TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n-TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n-TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n-TEST_CTYPE_FUNC(isascii, ASCII)\n-TEST_CTYPE_FUNC(islower, LOWER)\n-TEST_CTYPE_FUNC(isupper, UPPER)\n-TEST_CTYPE_FUNC(iscntrl, CNTRL)\n-TEST_CTYPE_FUNC(ispunct, PUNCT)\n-TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n-TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n+#define APPLY16(f, n) \\\n+\tf(n + 0x0), f(n + 0x1), f(n + 0x2), f(n + 0x3), \\\n+\tf(n + 0x4), f(n + 0x5), f(n + 0x6), f(n + 0x7), \\\n+\tf(n + 0x8), f(n + 0x9), f(n + 0xa), f(n + 0xb), \\\n+\tf(n + 0xc), f(n + 0xd), f(n + 0xe), f(n + 0xf)\n+#define APPLY256(f) \\\n+\tAPPLY16(f, 0x00), APPLY16(f, 0x10), APPLY16(f, 0x20), APPLY16(f, 0x30),\\\n+\tAPPLY16(f, 0x40), APPLY16(f, 0x50), APPLY16(f, 0x60), APPLY16(f, 0x70),\\\n+\tAPPLY16(f, 0x80), APPLY16(f, 0x90), APPLY16(f, 0xa0), APPLY16(f, 0xb0),\\\n+\tAPPLY16(f, 0xc0), APPLY16(f, 0xd0), APPLY16(f, 0xe0), APPLY16(f, 0xf0),\\\n+\n+#define CTYPE(name, expect) { #name, expect, { APPLY256(name) } }\n\n int cmd_main(int argc, const char **argv) {\n+\tstruct ctype classes[] = {\n+\t\tCTYPE(isdigit, DIGIT),\n+\t\tCTYPE(isspace, \" \\n\\r\\t\"),\n+\t\tCTYPE(isalpha, LOWER UPPER),\n+\t\tCTYPE(isalnum, LOWER UPPER DIGIT),\n+\t\tCTYPE(is_glob_special, \"*?[\\\\\"),\n+\t\tCTYPE(is_regex_special, \"$()*+.?[\\\\^{|\"),\n+\t\tCTYPE(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\"),\n+\t\tCTYPE(isascii, ASCII),\n+\t\tCTYPE(islower, LOWER),\n+\t\tCTYPE(isupper, UPPER),\n+\t\tCTYPE(iscntrl, CNTRL),\n+\t\tCTYPE(ispunct, PUNCT),\n+\t\tCTYPE(isxdigit, DIGIT \"abcdefABCDEF\"),\n+\t\tCTYPE(isprint, LOWER UPPER DIGIT PUNCT \" \"),\n+\t};\n \t/* Run all character type tests */\n-\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n-\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n-\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n-\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n-\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n-\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n-\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n-\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n-\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n-\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n-\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n-\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n-\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n-\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n+\tfor (int i = 0; i < ARRAY_SIZE(classes); i++)\n+\t\tTEST(test_ctype(&classes[i]), \"%s works\", classes[i].name);\n\n \treturn test_done();\n }\n\n"},{"id":"486056","messageId":"e1e9290f-755a-457c-911b-769a311c47fb@gmail.com","threadId":"60649","inReplyTo":"f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de","subject":"Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-27T14:40:01Z","receivedAt":"2023-12-27T14:40:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/12/2023 11:57, René Scharfe wrote:\n> Am 27.12.23 um 11:57 schrieb Christian Couder:\n>> On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano wrote:\n>>>\n>>> Achu Luma writes:\n>>\n>>>> +/* Macro to test a character type */\n>>>> +#define TEST_CTYPE_FUNC(func, string) \\\n>>>> +static void test_ctype_##func(void) \\\n>>>> +{ \\\n>>>> + int i; \\\n>>>> + for (i = 0; i < 256; i++) \\\n>>>> + check_int(func(i), ==, is_in(string, i)); \\\n>>>> +}\n>>>\n>>> Now, we let check_int() to do the checking for each and every byte\n>>> value for the class. check_int() uses different reporting and shows\n>>> the problematic value in a way that is more verbose and at the same\n>>> time is a less specific and harder to understand:\n>>>\n>>> test_msg(\" left: %\"PRIdMAX, a);\n>>> test_msg(\" right: %\"PRIdMAX, b);\n>>>\n>>> But that is probably the price to pay to use a more generic\n>>> framework, I guess.\n>>\n>> I have added Phillip and Josh in Cc: as they might have ideas about this.\n> \n> You can write custom messages for custom tests using test_assert().\n\nAnother possibility is to do\n\n\tfor (int i = 0; i < 256; i++) {\n\t\tif (!check_int(func(i), ==, is_in(string, i))\n\t\t\ttest_msg(\" i: %02x\", i);\n\t}\n\nTo print the character code as well as the actual and expected return \nvalues of check_int(). The funny spacing is intended to keep the output \naligned. I did wonder if we should be using\n\n\tcheck(func(i) == is_in(string, i))\n\ninstead of check_int() but I think it is useful to have the return value \nprinted on error in case we start returning \"53\" instead of \"1\" for \n\"true\" [1]. With the extra test_msg() above we can now see if the test \nfails because of a mis-categorization or because func() returned a \ndifferent non-zero value when we were expecting \"1\".\n\n>> Also it might not be a big issue here, but when the new unit test\n>> framework was proposed, I commented on the fact that \"left\" and\n>> \"right\" were perhaps a bit less explicit than \"actual\" and \"expected\".\n\nIf people are worried about this then it would be possible to change the \ncheck_xxx() macros pass the stringified relational operator into the \nvarious check_xxx_loc() functions and then print \"expected\" and \"actual\" \nwhen the operator is \"==\" and \"left\" and \"right\" otherwise.\n\nBest Wishes\n\nPhillip\n\n[1] As an aside I wonder if the ctype functions would make good test \nballoons for using _Bool by changing sane_istest() to be\n\n#define sane_istest(x,mask) ((bool)(sane_ctype[(unsigned char)(x)] & \n(mask)))\n\nso that we check casting to _Bool coerces non-zero values to \"1\"\n"},{"id":"486064","messageId":"xmqqcyurky00.fsf@gitster.g","threadId":"60649","inReplyTo":"f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de","subject":"Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-27T23:48:47Z","receivedAt":"2023-12-27T23:48:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> Also it might not be a big issue here, but when the new unit test\n>> framework was proposed, I commented on the fact that \"left\" and\n>> \"right\" were perhaps a bit less explicit than \"actual\" and \"expected\".\n>\n> True.\n> ...\n> The added repetition is a bit grating. With a bit of setup, loop\n> unrolling and stringification you can retain the property of only having\n> to mention the class name once. Demo patch below.\n\nNice.\n\nThis (and your mempool thing) being one of the early efforts to\nadopt the unit-test framework outside the initial set of sample\ntests, it is understandable that we might find what framework offers\nis still lacking. But at the same time, while the macro tricks\ndemonstrated here are all amusing to read and admire, it feels a bit\ntoo much to expect that the test writers are willing to invent\nsomething like these every time they want to test.\n\nBeing a relatively faithful conversion of the original ctype tests,\nwith its thorough enumeration of test samples and expected output,\nis what makes this test program require these macro tricks, and it\ndoes not have much to do with the features (or lack thereof) of the\nframework, I guess.\n\n> +struct ctype {\n> +\tconst char *name;\n> +\tconst char *expect;\n> +\tint actual[256];\n> +};\n> +\n> +static void test_ctype(const struct ctype *class)\n> +{\n> +\tfor (int i = 0; i < 256; i++) {\n> +\t\tint expect = is_in(class->expect, i);\n> +\t\tint actual = class->actual[i];\n> +\t\tint res = test_assert(TEST_LOCATION(), class->name,\n> +\t\t\t\t actual == expect);\n> +\t\tif (!res)\n> +\t\t\ttest_msg(\"%s classifies char %d (0x%02x) wrongly\",\n> +\t\t\t\t class->name, i, i);\n> +\t}\n> }\n\nSomehow, the \"test_assert\" does not seem to be adding much value\nhere (i.e. we can do \"res = (actual == expect)\" there). Is this\nbecause we want to be able to report success, too?\n\n ... goes and looks at test_assert() ...\n\nAh, is it because we want to be able to \"skip\" (which pretends that\nthe assert() was satisified). OK, but then the error reporting from\nit is redundant with our own test_msg(). \n\nEverything below this line was a fun read ;-)\n\nThanks.\n\n> ...\n> +#define APPLY16(f, n) \\\n> +\tf(n + 0x0), f(n + 0x1), f(n + 0x2), f(n + 0x3), \\\n> +\tf(n + 0x4), f(n + 0x5), f(n + 0x6), f(n + 0x7), \\\n> +\tf(n + 0x8), f(n + 0x9), f(n + 0xa), f(n + 0xb), \\\n> +\tf(n + 0xc), f(n + 0xd), f(n + 0xe), f(n + 0xf)\n> +#define APPLY256(f) \\\n> +\tAPPLY16(f, 0x00), APPLY16(f, 0x10), APPLY16(f, 0x20), APPLY16(f, 0x30),\\\n> +\tAPPLY16(f, 0x40), APPLY16(f, 0x50), APPLY16(f, 0x60), APPLY16(f, 0x70),\\\n> +\tAPPLY16(f, 0x80), APPLY16(f, 0x90), APPLY16(f, 0xa0), APPLY16(f, 0xb0),\\\n> +\tAPPLY16(f, 0xc0), APPLY16(f, 0xd0), APPLY16(f, 0xe0), APPLY16(f, 0xf0),\\\n> +\n> +#define CTYPE(name, expect) { #name, expect, { APPLY256(name) } }\n>\n> int cmd_main(int argc, const char **argv) {\n> +\tstruct ctype classes[] = {\n> +\t\tCTYPE(isdigit, DIGIT),\n> +\t\tCTYPE(isspace, \" \\n\\r\\t\"),\n> +\t\tCTYPE(isalpha, LOWER UPPER),\n> +\t\tCTYPE(isalnum, LOWER UPPER DIGIT),\n> +\t\tCTYPE(is_glob_special, \"*?[\\\\\"),\n> +\t\tCTYPE(is_regex_special, \"$()*+.?[\\\\^{|\"),\n> +\t\tCTYPE(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\"),\n> +\t\tCTYPE(isascii, ASCII),\n> +\t\tCTYPE(islower, LOWER),\n> +\t\tCTYPE(isupper, UPPER),\n> +\t\tCTYPE(iscntrl, CNTRL),\n> +\t\tCTYPE(ispunct, PUNCT),\n> +\t\tCTYPE(isxdigit, DIGIT \"abcdefABCDEF\"),\n> +\t\tCTYPE(isprint, LOWER UPPER DIGIT PUNCT \" \"),\n> +\t};\n> \t/* Run all character type tests */\n> -\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n> -\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n> -\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n> -\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n> -\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n> -\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n> -\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n> -\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n> -\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n> -\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n> -\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n> -\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n> -\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n> -\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n> +\tfor (int i = 0; i < ARRAY_SIZE(classes); i++)\n> +\t\tTEST(test_ctype(&classes[i]), \"%s works\", classes[i].name);\n>\n> \treturn test_done();\n> }\n"},{"id":"486123","messageId":"e01be0ec-a054-42a1-8abe-6891c04b59a2@web.de","threadId":"60649","inReplyTo":"xmqqcyurky00.fsf@gitster.g","subject":"Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-28T16:05:33Z","receivedAt":"2023-12-28T16:05:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.12.23 um 00:48 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>> Also it might not be a big issue here, but when the new unit test\n>>> framework was proposed, I commented on the fact that \"left\" and\n>>> \"right\" were perhaps a bit less explicit than \"actual\" and \"expected\".\n>>\n>> True.\n>> ...\n>> The added repetition is a bit grating. With a bit of setup, loop\n>> unrolling and stringification you can retain the property of only having\n>> to mention the class name once. Demo patch below.\n>\n> Nice.\n>\n> This (and your mempool thing) being one of the early efforts to\n> adopt the unit-test framework outside the initial set of sample\n> tests, it is understandable that we might find what framework offers\n> is still lacking. But at the same time, while the macro tricks\n> demonstrated here are all amusing to read and admire, it feels a bit\n> too much to expect that the test writers are willing to invent\n> something like these every time they want to test.\n>\n> Being a relatively faithful conversion of the original ctype tests,\n> with its thorough enumeration of test samples and expected output,\n> is what makes this test program require these macro tricks, and it\n> does not have much to do with the features (or lack thereof) of the\n> framework, I guess.\n\n*nod*\n\n>\n>> +struct ctype {\n>> +\tconst char *name;\n>> +\tconst char *expect;\n>> +\tint actual[256];\n>> +};\n>> +\n>> +static void test_ctype(const struct ctype *class)\n>> +{\n>> +\tfor (int i = 0; i < 256; i++) {\n>> +\t\tint expect = is_in(class->expect, i);\n>> +\t\tint actual = class->actual[i];\n>> +\t\tint res = test_assert(TEST_LOCATION(), class->name,\n>> +\t\t\t\t actual == expect);\n>> +\t\tif (!res)\n>> +\t\t\ttest_msg(\"%s classifies char %d (0x%02x) wrongly\",\n>> +\t\t\t\t class->name, i, i);\n>> +\t}\n>> }\n>\n> Somehow, the \"test_assert\" does not seem to be adding much value\n> here (i.e. we can do \"res = (actual == expect)\" there). Is this\n> because we want to be able to report success, too?\n>\n> ... goes and looks at test_assert() ...\n>\n> Ah, is it because we want to be able to \"skip\" (which pretends that\n> the assert() was satisified). OK, but then the error reporting from\n> it is redundant with our own test_msg().\n\nTrue, the test_msg() emits the old message here, but it doesn't have to\nreport that the check failed anymore, because test_assert() already\ncovers that part. It would only have to report the misclassified\ncharacter and perhaps the expected result.\n\nRené\n"},{"id":"486165","messageId":"20231230001102.9220-1-ach.lumap@gmail.com","threadId":"60649","inReplyTo":"20231221231527.8130-1-ach.lumap@gmail.com","subject":"[Outreachy][PATCH v2] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2023-12-30T00:09:59Z","receivedAt":"2023-12-30T00:11:58Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"In the recent codebase update(8bf6fbd00d (Merge branch\n'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\nmerged, providing a standardized approach for testing C code. Prior to\nthis update, some unit tests relied on the test helper mechanism,\nlacking a dedicated unit testing framework. It's more natural to perform\nthese unit tests using the new unit test framework.\n\nThis commit migrates the unit tests for C character classification\nfunctions (isdigit(), isspace(), etc) from the legacy approach\nusing the test-tool command `test-tool ctype` in t/helper/test-ctype.c\nto the new unit testing framework (t/unit-tests/test-lib.h).\n\nThe migration involves refactoring the tests to utilize the testing\nmacros provided by the framework (TEST() and check_*()).\n\nMentored-by: Christian Couder \nSigned-off-by: Achu Luma \n---\n In the revised patch, several improvements were made to the \n TEST_CTYPE_FUNC macro. The updated version enhances error handling \n by utilizing a direct comparison approach between the func and \n is_in(string, i) functions across ASCII characters. Additionally, \n the loop control variable i is locally scoped within the loop, ensuring \n proper encapsulation. These changes streamline the comparison process \n and clarify failure reporting. Special thanks to the invaluable reviews \n by Junio, Phillip and René for their insightful feedback and thorough \n review of the patch, contributing significantly to these changes.\n\n Makefile | 2 +-\n t/helper/test-ctype.c | 70 --------------------------------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0070-fundamental.sh | 4 ---\n t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 78 insertions(+), 77 deletions(-)\n delete mode 100644 t/helper/test-ctype.c\n create mode 100644 t/unit-tests/t-ctype.c\n\ndiff --git a/Makefile b/Makefile\nindex 88ba7a3c51..a4df48ba65 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\n TEST_BUILTINS_OBJS += test-csprng.o\n-TEST_BUILTINS_OBJS += test-ctype.o\n TEST_BUILTINS_OBJS += test-date.o\n TEST_BUILTINS_OBJS += test-delta.o\n TEST_BUILTINS_OBJS += test-dir-iterator.o\n@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-basic\n UNIT_TEST_PROGRAMS += t-strbuf\n+UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\ndiff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\ndeleted file mode 100644\nindex e5659df40b..0000000000\n--- a/t/helper/test-ctype.c\n+++ /dev/null\n@@ -1,70 +0,0 @@\n-#include \"test-tool.h\"\n-\n-static int rc;\n-\n-static void report_error(const char *class, int ch)\n-{\n-\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n-\trc = 1;\n-}\n-\n-static int is_in(const char *s, int ch)\n-{\n-\t/*\n-\t * We can't find NUL using strchr. Accept it as the first\n-\t * character in the spec -- there are no empty classes.\n-\t */\n-\tif (ch == '\\0')\n-\t\treturn ch == *s;\n-\tif (*s == '\\0')\n-\t\ts++;\n-\treturn !!strchr(s, ch);\n-}\n-\n-#define TEST_CLASS(t,s) {\t\t\t\\\n-\tint i;\t\t\t\t\t\\\n-\tfor (i = 0; i < 256; i++) {\t\t\\\n-\t\tif (is_in(s, i) != t(i))\t\\\n-\t\t\treport_error(#t, i);\t\\\n-\t}\t\t\t\t\t\\\n-\tif (t(EOF))\t\t\t\t\\\n-\t\treport_error(#t, EOF);\t\t\\\n-}\n-\n-#define DIGIT \"0123456789\"\n-#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n-#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n-#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n-#define ASCII \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n-\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n-\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n-\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n-\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n-\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n-#define CNTRL \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x7f\"\n-\n-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tTEST_CLASS(isdigit, DIGIT);\n-\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n-\tTEST_CLASS(isalpha, LOWER UPPER);\n-\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n-\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n-\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n-\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n-\tTEST_CLASS(isascii, ASCII);\n-\tTEST_CLASS(islower, LOWER);\n-\tTEST_CLASS(isupper, UPPER);\n-\tTEST_CLASS(iscntrl, CNTRL);\n-\tTEST_CLASS(ispunct, PUNCT);\n-\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n-\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n-\n-\treturn rc;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 37ba996539..33b9501c21 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\n \t{ \"csprng\", cmd__csprng },\n-\t{ \"ctype\", cmd__ctype },\n \t{ \"date\", cmd__date },\n \t{ \"delta\", cmd__delta },\n \t{ \"dir-iterator\", cmd__dir_iterator },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 8a1a7c63da..b72f07ded9 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\n int cmd__csprng(int argc, const char **argv);\n-int cmd__ctype(int argc, const char **argv);\n int cmd__date(int argc, const char **argv);\n int cmd__delta(int argc, const char **argv);\n int cmd__dir_iterator(int argc, const char **argv);\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 487bc8d905..a4756fbab9 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-test_expect_success 'character classes (isspace, isalpha etc.)' '\n-\ttest-tool ctype\n-'\n-\n test_expect_success 'mktemp to nonexistent directory prints filename' '\n \ttest_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&\n \tgrep \"doesnotexist/test\" err\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nnew file mode 100644\nindex 0000000000..8a215d387a\n--- /dev/null\n+++ b/t/unit-tests/t-ctype.c\n@@ -0,0 +1,77 @@\n+#include \"test-lib.h\"\n+\n+static int is_in(const char *s, int ch)\n+{\n+\t/*\n+\t * We can't find NUL using strchr. Accept it as the first\n+\t * character in the spec -- there are no empty classes.\n+\t */\n+\tif (ch == '\\0')\n+\t\treturn ch == *s;\n+\tif (*s == '\\0')\n+\t\ts++;\n+\treturn !!strchr(s, ch);\n+}\n+\n+/* Macro to test a character type */\n+#define TEST_CTYPE_FUNC(func, string)\t\t\t\t\\\n+static void test_ctype_##func(void)\t\t\t\t\\\n+{\t\t\t\t\t\t\t\t\\\n+\tfor (int i = 0; i < 256; i++) {\t\t\t\t\\\n+ \tif (!check_int(func(i), ==, is_in(string, i))) \t\\\n+ \t\ttest_msg(\" i: %02x\", i);\t\t\\\n+ } \t\t\t\t\t\t\t\\\n+}\n+\n+#define DIGIT \"0123456789\"\n+#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n+#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n+#define ASCII \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n+\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n+\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n+\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n+\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n+\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n+#define CNTRL \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x7f\"\n+\n+TEST_CTYPE_FUNC(isdigit, DIGIT)\n+TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n+TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n+TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n+TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n+TEST_CTYPE_FUNC(isascii, ASCII)\n+TEST_CTYPE_FUNC(islower, LOWER)\n+TEST_CTYPE_FUNC(isupper, UPPER)\n+TEST_CTYPE_FUNC(iscntrl, CNTRL)\n+TEST_CTYPE_FUNC(ispunct, PUNCT)\n+TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n+\n+int cmd_main(int argc, const char **argv) {\n+\t/* Run all character type tests */\n+\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n+\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n+\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n+\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n+\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n+\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n+\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n+\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n+\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n+\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n+\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n+\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n+\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n+\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n+\n+\treturn test_done();\n+}\n-- \n2.42.0.windows.2\n\n"},{"id":"486184","messageId":"20240101104017.9452-2-ach.lumap@gmail.com","threadId":"60649","inReplyTo":"20231230001102.9220-1-ach.lumap@gmail.com","subject":"[Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2024-01-01T10:40:18Z","receivedAt":"2024-01-01T10:41:07Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"In the recent codebase update(8bf6fbd00d (Merge branch\n'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\nmerged, providing a standardized approach for testing C code. Prior to\nthis update, some unit tests relied on the test helper mechanism,\nlacking a dedicated unit testing framework. It's more natural to perform\nthese unit tests using the new unit test framework.\n\nThis commit migrates the unit tests for C character classification\nfunctions (isdigit(), isspace(), etc) from the legacy approach\nusing the test-tool command `test-tool ctype` in t/helper/test-ctype.c\nto the new unit testing framework (t/unit-tests/test-lib.h).\n\nThe migration involves refactoring the tests to utilize the testing\nmacros provided by the framework (TEST() and check_*()).\n\nSigned-off-by: Achu Luma \n---\n Sorry for the poor description of the changes, maybe the following is better:\n In the revised patch we added a call to test_msg() suggested by Phillip to \n print the character code. This helps us pinpoint exactly the byte value that \n is an issue as suggested by Junio. We keep using check_int() as it allows us \n to still see the actual and expected return which might help us in case func() \n returned a different non-zero value when we were expecting \"1\". It is useful \n to have the return value printed on error in case we start returning \"53\" \n instead of \"1\" for \"true\".\n\n Makefile | 2 +-\n t/helper/test-ctype.c | 70 --------------------------------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0070-fundamental.sh | 4 ---\n t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 78 insertions(+), 77 deletions(-)\n delete mode 100644 t/helper/test-ctype.c\n create mode 100644 t/unit-tests/t-ctype.c\n\ndiff --git a/Makefile b/Makefile\nindex 88ba7a3c51..a4df48ba65 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\n TEST_BUILTINS_OBJS += test-csprng.o\n-TEST_BUILTINS_OBJS += test-ctype.o\n TEST_BUILTINS_OBJS += test-date.o\n TEST_BUILTINS_OBJS += test-delta.o\n TEST_BUILTINS_OBJS += test-dir-iterator.o\n@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n \n UNIT_TEST_PROGRAMS += t-basic\n UNIT_TEST_PROGRAMS += t-strbuf\n+UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\ndiff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\ndeleted file mode 100644\nindex e5659df40b..0000000000\n--- a/t/helper/test-ctype.c\n+++ /dev/null\n@@ -1,70 +0,0 @@\n-#include \"test-tool.h\"\n-\n-static int rc;\n-\n-static void report_error(const char *class, int ch)\n-{\n-\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n-\trc = 1;\n-}\n-\n-static int is_in(const char *s, int ch)\n-{\n-\t/*\n-\t * We can't find NUL using strchr. Accept it as the first\n-\t * character in the spec -- there are no empty classes.\n-\t */\n-\tif (ch == '\\0')\n-\t\treturn ch == *s;\n-\tif (*s == '\\0')\n-\t\ts++;\n-\treturn !!strchr(s, ch);\n-}\n-\n-#define TEST_CLASS(t,s) {\t\t\t\\\n-\tint i;\t\t\t\t\t\\\n-\tfor (i = 0; i < 256; i++) {\t\t\\\n-\t\tif (is_in(s, i) != t(i))\t\\\n-\t\t\treport_error(#t, i);\t\\\n-\t}\t\t\t\t\t\\\n-\tif (t(EOF))\t\t\t\t\\\n-\t\treport_error(#t, EOF);\t\t\\\n-}\n-\n-#define DIGIT \"0123456789\"\n-#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n-#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n-#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n-#define ASCII \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n-\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n-\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n-\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n-\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n-\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n-#define CNTRL \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x7f\"\n-\n-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tTEST_CLASS(isdigit, DIGIT);\n-\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n-\tTEST_CLASS(isalpha, LOWER UPPER);\n-\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n-\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n-\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n-\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n-\tTEST_CLASS(isascii, ASCII);\n-\tTEST_CLASS(islower, LOWER);\n-\tTEST_CLASS(isupper, UPPER);\n-\tTEST_CLASS(iscntrl, CNTRL);\n-\tTEST_CLASS(ispunct, PUNCT);\n-\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n-\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n-\n-\treturn rc;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 37ba996539..33b9501c21 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\n \t{ \"csprng\", cmd__csprng },\n-\t{ \"ctype\", cmd__ctype },\n \t{ \"date\", cmd__date },\n \t{ \"delta\", cmd__delta },\n \t{ \"dir-iterator\", cmd__dir_iterator },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 8a1a7c63da..b72f07ded9 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\n int cmd__csprng(int argc, const char **argv);\n-int cmd__ctype(int argc, const char **argv);\n int cmd__date(int argc, const char **argv);\n int cmd__delta(int argc, const char **argv);\n int cmd__dir_iterator(int argc, const char **argv);\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 487bc8d905..a4756fbab9 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-test_expect_success 'character classes (isspace, isalpha etc.)' '\n-\ttest-tool ctype\n-'\n-\n test_expect_success 'mktemp to nonexistent directory prints filename' '\n \ttest_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&\n \tgrep \"doesnotexist/test\" err\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nnew file mode 100644\nindex 0000000000..8a215d387a\n--- /dev/null\n+++ b/t/unit-tests/t-ctype.c\n@@ -0,0 +1,77 @@\n+#include \"test-lib.h\"\n+\n+static int is_in(const char *s, int ch)\n+{\n+\t/*\n+\t * We can't find NUL using strchr. Accept it as the first\n+\t * character in the spec -- there are no empty classes.\n+\t */\n+\tif (ch == '\\0')\n+\t\treturn ch == *s;\n+\tif (*s == '\\0')\n+\t\ts++;\n+\treturn !!strchr(s, ch);\n+}\n+\n+/* Macro to test a character type */\n+#define TEST_CTYPE_FUNC(func, string)\t\t\t\t\\\n+static void test_ctype_##func(void)\t\t\t\t\\\n+{\t\t\t\t\t\t\t\t\\\n+\tfor (int i = 0; i < 256; i++) {\t\t\t\t\\\n+ \tif (!check_int(func(i), ==, is_in(string, i))) \t\\\n+ \t\ttest_msg(\" i: %02x\", i);\t\t\\\n+ } \t\t\t\t\t\t\t\\\n+}\n+\n+#define DIGIT \"0123456789\"\n+#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n+#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n+#define ASCII \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n+\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n+\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n+\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n+\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n+\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n+#define CNTRL \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x7f\"\n+\n+TEST_CTYPE_FUNC(isdigit, DIGIT)\n+TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n+TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n+TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n+TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n+TEST_CTYPE_FUNC(isascii, ASCII)\n+TEST_CTYPE_FUNC(islower, LOWER)\n+TEST_CTYPE_FUNC(isupper, UPPER)\n+TEST_CTYPE_FUNC(iscntrl, CNTRL)\n+TEST_CTYPE_FUNC(ispunct, PUNCT)\n+TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n+\n+int cmd_main(int argc, const char **argv) {\n+\t/* Run all character type tests */\n+\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n+\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n+\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n+\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n+\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n+\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n+\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n+\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n+\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n+\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n+\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n+\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n+\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n+\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n+\n+\treturn test_done();\n+}\n-- \n2.42.0.windows.2\n\n"},{"id":"486190","messageId":"435a03c7-dbf3-4ddb-b183-cac86ed0467d@web.de","threadId":"60649","inReplyTo":"20240101104017.9452-2-ach.lumap@gmail.com","subject":"Re: [Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-01T16:41:06Z","receivedAt":"2024-01-01T16:41:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.01.24 um 11:40 schrieb Achu Luma:\n> In the recent codebase update(8bf6fbd00d (Merge branch\n> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\n> merged, providing a standardized approach for testing C code. Prior to\n> this update, some unit tests relied on the test helper mechanism,\n> lacking a dedicated unit testing framework. It's more natural to perform\n> these unit tests using the new unit test framework.\n>\n> This commit migrates the unit tests for C character classification\n> functions (isdigit(), isspace(), etc) from the legacy approach\n> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c\n> to the new unit testing framework (t/unit-tests/test-lib.h).\n>\n> The migration involves refactoring the tests to utilize the testing\n> macros provided by the framework (TEST() and check_*()).\n>\n> Signed-off-by: Achu Luma \n\nv1 and v2 had \"Mentored-by: Christian Couder \".\nIt's gone now; intentionally?\n\n> ---\n> Sorry for the poor description of the changes, maybe the following is better:\n> In the revised patch we added a call to test_msg() suggested by Phillip to\n> print the character code. This helps us pinpoint exactly the byte value that\n> is an issue as suggested by Junio. We keep using check_int() as it allows us\n> to still see the actual and expected return which might help us in case func()\n> returned a different non-zero value when we were expecting \"1\". It is useful\n> to have the return value printed on error in case we start returning \"53\"\n> instead of \"1\" for \"true\".\n\nTo illustrate, here are the changes between v1 and v2 in diff form:\n\n--- >8 ---\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nindex 41189ba9f9..8a215d387a 100644\n--- a/t/unit-tests/t-ctype.c\n+++ b/t/unit-tests/t-ctype.c\n@@ -14,12 +14,13 @@ static int is_in(const char *s, int ch)\n }\n\n /* Macro to test a character type */\n-#define TEST_CTYPE_FUNC(func, string)\t\t\t\\\n+#define TEST_CTYPE_FUNC(func, string)\t\t\t\t\\\n static void test_ctype_##func(void)\t\t\t\t\\\n {\t\t\t\t\t\t\t\t\\\n-\tint i; \t \t\\\n-\tfor (i = 0; i < 256; i++) \t\t\\\n-\t\tcheck_int(func(i), ==, is_in(string, i)); \t\\\n+\tfor (int i = 0; i < 256; i++) {\t\t\t\t\\\n+ \tif (!check_int(func(i), ==, is_in(string, i))) \t\\\n+ \t\ttest_msg(\" i: %02x\", i);\t\t\\\n+ } \t\t\t\t\t\t\t\\\n }\n\n #define DIGIT \"0123456789\"\n--- >8 ---\n\nv3 is the same as v2.\n\n>\n> Makefile | 2 +-\n> t/helper/test-ctype.c | 70 --------------------------------------\n> t/helper/test-tool.c | 1 -\n> t/helper/test-tool.h | 1 -\n> t/t0070-fundamental.sh | 4 ---\n> t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++\n> 6 files changed, 78 insertions(+), 77 deletions(-)\n> delete mode 100644 t/helper/test-ctype.c\n> create mode 100644 t/unit-tests/t-ctype.c\n>\n> diff --git a/Makefile b/Makefile\n> index 88ba7a3c51..a4df48ba65 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o\n> TEST_BUILTINS_OBJS += test-config.o\n> TEST_BUILTINS_OBJS += test-crontab.o\n> TEST_BUILTINS_OBJS += test-csprng.o\n> -TEST_BUILTINS_OBJS += test-ctype.o\n> TEST_BUILTINS_OBJS += test-date.o\n> TEST_BUILTINS_OBJS += test-delta.o\n> TEST_BUILTINS_OBJS += test-dir-iterator.o\n> @@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n>\n> UNIT_TEST_PROGRAMS += t-basic\n> UNIT_TEST_PROGRAMS += t-strbuf\n> +UNIT_TEST_PROGRAMS += t-ctype\n> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\n> deleted file mode 100644\n> index e5659df40b..0000000000\n> --- a/t/helper/test-ctype.c\n> +++ /dev/null\n> @@ -1,70 +0,0 @@\n> -#include \"test-tool.h\"\n> -\n> -static int rc;\n> -\n> -static void report_error(const char *class, int ch)\n> -{\n> -\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n> -\trc = 1;\n> -}\n> -\n> -static int is_in(const char *s, int ch)\n> -{\n> -\t/*\n> -\t * We can't find NUL using strchr. Accept it as the first\n> -\t * character in the spec -- there are no empty classes.\n> -\t */\n> -\tif (ch == '\\0')\n> -\t\treturn ch == *s;\n> -\tif (*s == '\\0')\n> -\t\ts++;\n> -\treturn !!strchr(s, ch);\n> -}\n> -\n> -#define TEST_CLASS(t,s) {\t\t\t\\\n> -\tint i;\t\t\t\t\t\\\n> -\tfor (i = 0; i < 256; i++) {\t\t\\\n> -\t\tif (is_in(s, i) != t(i))\t\\\n> -\t\t\treport_error(#t, i);\t\\\n> -\t}\t\t\t\t\t\\\n> -\tif (t(EOF))\t\t\t\t\\\n> -\t\treport_error(#t, EOF);\t\t\\\n> -}\n> -\n> -#define DIGIT \"0123456789\"\n> -#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n> -#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n> -#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n> -#define ASCII \\\n> -\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> -\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> -\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n> -\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n> -\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n> -\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n> -\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n> -\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n> -#define CNTRL \\\n> -\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> -\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> -\t\"\\x7f\"\n> -\n> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n> -{\n> -\tTEST_CLASS(isdigit, DIGIT);\n> -\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n> -\tTEST_CLASS(isalpha, LOWER UPPER);\n> -\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n> -\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n> -\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n> -\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n> -\tTEST_CLASS(isascii, ASCII);\n> -\tTEST_CLASS(islower, LOWER);\n> -\tTEST_CLASS(isupper, UPPER);\n> -\tTEST_CLASS(iscntrl, CNTRL);\n> -\tTEST_CLASS(ispunct, PUNCT);\n> -\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n> -\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n> -\n> -\treturn rc;\n> -}\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 37ba996539..33b9501c21 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {\n> \t{ \"config\", cmd__config },\n> \t{ \"crontab\", cmd__crontab },\n> \t{ \"csprng\", cmd__csprng },\n> -\t{ \"ctype\", cmd__ctype },\n> \t{ \"date\", cmd__date },\n> \t{ \"delta\", cmd__delta },\n> \t{ \"dir-iterator\", cmd__dir_iterator },\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index 8a1a7c63da..b72f07ded9 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);\n> int cmd__config(int argc, const char **argv);\n> int cmd__crontab(int argc, const char **argv);\n> int cmd__csprng(int argc, const char **argv);\n> -int cmd__ctype(int argc, const char **argv);\n> int cmd__date(int argc, const char **argv);\n> int cmd__delta(int argc, const char **argv);\n> int cmd__dir_iterator(int argc, const char **argv);\n> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\n> index 487bc8d905..a4756fbab9 100755\n> --- a/t/t0070-fundamental.sh\n> +++ b/t/t0070-fundamental.sh\n> @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.\n> TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> -test_expect_success 'character classes (isspace, isalpha etc.)' '\n> -\ttest-tool ctype\n> -'\n> -\n> test_expect_success 'mktemp to nonexistent directory prints filename' '\n> \ttest_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&\n> \tgrep \"doesnotexist/test\" err\n> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n> new file mode 100644\n> index 0000000000..8a215d387a\n> --- /dev/null\n> +++ b/t/unit-tests/t-ctype.c\n> @@ -0,0 +1,77 @@\n> +#include \"test-lib.h\"\n> +\n> +static int is_in(const char *s, int ch)\n> +{\n> +\t/*\n> +\t * We can't find NUL using strchr. Accept it as the first\n> +\t * character in the spec -- there are no empty classes.\n> +\t */\n> +\tif (ch == '\\0')\n> +\t\treturn ch == *s;\n> +\tif (*s == '\\0')\n> +\t\ts++;\n> +\treturn !!strchr(s, ch);\n> +}\n> +\n> +/* Macro to test a character type */\n> +#define TEST_CTYPE_FUNC(func, string)\t\t\t\t\\\n> +static void test_ctype_##func(void)\t\t\t\t\\\n> +{\t\t\t\t\t\t\t\t\\\n> +\tfor (int i = 0; i < 256; i++) {\t\t\t\t\\\n> + \tif (!check_int(func(i), ==, is_in(string, i))) \t\\\n> + \t\ttest_msg(\" i: %02x\", i);\t\t\\\n> + } \t\t\t\t\t\t\t\\\n\nThis loop is indented with spaces followed by tabs. The Git project\nprefers indenting by tabs and in some cases tabs followed by spaces, but\nnot the other way array. git am warns about such whitespace errors and\ncan actually fix them automatically, so I imagine this wouldn't be a\ndeal breaker. But even if it seems picky, respecting the project's\npreferences from the start reduces unnecessary friction.\n\nThe original test reported the number of a misclassified character\n(basically its ASCII code) in both decimal and hexadecimal form. This\ncode prints it only in hexadecimal, but without the prefix \"0x\". A\ncasual reader could mistake hexadecimal numbers for decimal ones as a\nresult. Printing only one suffices, but I think it's better to either\nuse decimal notation without any prefix or hexadecimal with the \"0x\"\nprefix to avoid confusion. There's no reason to be stingy with the\nscreen space here.\n\n> +}\n> +\n> +#define DIGIT \"0123456789\"\n> +#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n> +#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n> +#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n> +#define ASCII \\\n> +\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> +\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> +\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n> +\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n> +\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n> +\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n> +\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n> +\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n> +#define CNTRL \\\n> +\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> +\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> +\t\"\\x7f\"\n> +\n> +TEST_CTYPE_FUNC(isdigit, DIGIT)\n> +TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n> +TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n> +TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n> +TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n> +TEST_CTYPE_FUNC(isascii, ASCII)\n> +TEST_CTYPE_FUNC(islower, LOWER)\n> +TEST_CTYPE_FUNC(isupper, UPPER)\n> +TEST_CTYPE_FUNC(iscntrl, CNTRL)\n> +TEST_CTYPE_FUNC(ispunct, PUNCT)\n> +TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n> +\n> +int cmd_main(int argc, const char **argv) {\n> +\t/* Run all character type tests */\n> +\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n> +\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n> +\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n> +\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n> +\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n> +\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n> +\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n> +\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n> +\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n> +\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n> +\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n> +\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n> +\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n> +\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n\nEach class (e.g. space or digit) is mentioned thrice here: When\ndeclaring its function with TEST_CTYPE_FUNC, when calling said function\nand again in the test description. Adding a new class requires adding\ntwo lines of code. That's not too bad, but the original implementation\ndidn't require that repetition and adding a new class only required\nadding a single line.\n\nI mentioned this briefly in my review of v1 in\nhttps://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/\nand showed a way to define character classes without repeating their\nnames. You don't have to follow that suggestion, but it would be nice\nif you could give some feedback about it.\n\n> +\n> +\treturn test_done();\n> +}\n\nRené\n\n"},{"id":"486199","messageId":"xmqqbka3g0by.fsf@gitster.g","threadId":"60649","inReplyTo":"435a03c7-dbf3-4ddb-b183-cac86ed0467d@web.de","subject":"Re: [Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-02T16:35:29Z","receivedAt":"2024-01-02T16:35:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> ...\n>> +\tfor (int i = 0; i < 256; i++) {\t\t\t\t\\\n>> + \tif (!check_int(func(i), ==, is_in(string, i))) \t\\\n>> + \t\ttest_msg(\" i: %02x\", i);\t\t\\\n>> + } \t\t\t\t\t\t\t\\\n>\n> This loop is indented with spaces followed by tabs. The Git project\n> prefers indenting by tabs and in some cases tabs followed by spaces, but\n> not the other way array. git am warns about such whitespace errors and\n> can actually fix them automatically, so I imagine this wouldn't be a\n> deal breaker. But even if it seems picky, respecting the project's\n> preferences from the start reduces unnecessary friction.\n>\n> The original test reported the number of a misclassified character\n> (basically its ASCII code) in both decimal and hexadecimal form. This\n> code prints it only in hexadecimal, but without the prefix \"0x\". A\n> casual reader could mistake hexadecimal numbers for decimal ones as a\n> result. Printing only one suffices, but I think it's better to either\n> use decimal notation without any prefix or hexadecimal with the \"0x\"\n> prefix to avoid confusion. There's no reason to be stingy with the\n> screen space here.\n> ...\n> Each class (e.g. space or digit) is mentioned thrice here: When\n> declaring its function with TEST_CTYPE_FUNC, when calling said function\n> and again in the test description. Adding a new class requires adding\n> two lines of code. That's not too bad, but the original implementation\n> didn't require that repetition and adding a new class only required\n> adding a single line.\n\n\nThanks for an excellent review.\n\n\n>\n> I mentioned this briefly in my review of v1 in\n> https://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/\n> and showed a way to define character classes without repeating their\n> names. You don't have to follow that suggestion, but it would be nice\n> if you could give some feedback about it.\n>\n>> +\n>> +\treturn test_done();\n>> +}\n>\n> René\n"},{"id":"486206","messageId":"ZZRcPapIHFnyZTYB@nand.local","threadId":"60649","inReplyTo":"xmqqsf3ohkew.fsf@gitster.g","subject":"Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-02T18:55:57Z","receivedAt":"2024-01-02T18:55:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Dec 26, 2023 at 10:45:59AM -0800, Junio C Hamano wrote:\n> Achu Luma writes:\n>\n> > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\n> > deleted file mode 100644\n> > index e5659df40b..0000000000\n> > --- a/t/helper/test-ctype.c\n> > +++ /dev/null\n> > @@ -1,70 +0,0 @@\n> > -#include \"test-tool.h\"\n> > -\n> > -static int rc;\n> > -\n> > -static void report_error(const char *class, int ch)\n> > -{\n> > -\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n> > -\trc = 1;\n> > -}\n>\n> So, if we have a is_foo() that characterises a byte that ought to\n> be \"foo\" but gets miscategorised not to be \"foo\", we used to\n> pinpoint exactly the byte value that was an issue. We did not do\n> any early return ...\n>\n> > ...\n> > -#define TEST_CLASS(t,s) {\t\t\t\\\n> > -\tint i;\t\t\t\t\t\\\n> > -\tfor (i = 0; i < 256; i++) {\t\t\\\n> > -\t\tif (is_in(s, i) != t(i))\t\\\n> > -\t\t\treport_error(#t, i);\t\\\n> > -\t}\t\t\t\t\t\\\n> > -\tif (t(EOF))\t\t\t\t\\\n> > -\t\treport_error(#t, EOF);\t\t\\\n> > -}\n>\n> ... and reported for all errors in the \"class\".\n>\n> > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n> > new file mode 100644\n> > index 0000000000..41189ba9f9\n> > --- /dev/null\n> > +++ b/t/unit-tests/t-ctype.c\n> > @@ -0,0 +1,76 @@\n> > +#include \"test-lib.h\"\n> > +\n> > +static int is_in(const char *s, int ch)\n> > +{\n> > +\t/*\n> > +\t * We can't find NUL using strchr. Accept it as the first\n> > +\t * character in the spec -- there are no empty classes.\n> > +\t */\n> > +\tif (ch == '\\0')\n> > +\t\treturn ch == *s;\n> > +\tif (*s == '\\0')\n> > +\t\ts++;\n> > +\treturn !!strchr(s, ch);\n> > +}\n> > +\n> > +/* Macro to test a character type */\n> > +#define TEST_CTYPE_FUNC(func, string)\t\t\t\\\n> > +static void test_ctype_##func(void)\t\t\t\t\\\n> > +{\t\t\t\t\t\t\t\t\\\n> > +\tint i; \t \t\\\n> > +\tfor (i = 0; i < 256; i++) \t\t\\\n> > +\t\tcheck_int(func(i), ==, is_in(string, i)); \t\\\n> > +}\n>\n> Now, we let check_int() to do the checking for each and every byte\n> value for the class. check_int() uses different reporting and shows\n> the problematic value in a way that is more verbose and at the same\n> time is a less specific and harder to understand:\n>\n> \t\ttest_msg(\" left: %\"PRIdMAX, a);\n> \t\ttest_msg(\" right: %\"PRIdMAX, b);\n>\n> But that is probably the price to pay to use a more generic\n> framework, I guess.\n\nPerhaps I'm missing something here, since I haven't followed the\nunit-test effort very closely, but this check_int() macro feels like it\nmight be overkill for what we're trying to do.\n\nWe know that the expected value is the result of is_in(string, i), so I\nwonder if we might benefit from having an \"assert_equals()\" that looks\nlike:\n\n assert_equals(is_in(string, i), func(i));\n\nWhere we follow the usual convention of treating the first argument as\nthe expected value, and the second as the actual value. Then we could\nformat our error message to be more specific, like:\n\n test_msg(\"expected %d, got %d\", expected, actual);\n\nI think that this would be a little more readable, and still seems\nflexible enough to support the kind of thing that check_int(..., ==,\n...) is after.\n\n> > +int cmd_main(int argc, const char **argv) {\n> > +\t/* Run all character type tests */\n> > +\tTEST(test_ctype_isspace(), \"isspace() works as we expect\");\n> > +\tTEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n> > +\tTEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n> > +\tTEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n> > +\tTEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n> > +\tTEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n> > +\tTEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n> > +\tTEST(test_ctype_isascii(), \"isascii() works as we expect\");\n> > +\tTEST(test_ctype_islower(), \"islower() works as we expect\");\n> > +\tTEST(test_ctype_isupper(), \"isupper() works as we expect\");\n> > +\tTEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n> > +\tTEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n> > +\tTEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n> > +\tTEST(test_ctype_isprint(), \"isprint() works as we expect\");\n> > +\n> > +\treturn test_done();\n> > +}\n>\n> As a practice to use the unit-tests framework, the patch looks OK.\n> helper/test-ctype.c indeed is an oddball that runs once and checks\n> everything it wants to check, for which the unit tests framework is\n> much more suited.\n\nAs an aside, I don't think we need the \"works as we expect\" suffix in\neach test description. I personally would be fine with something like:\n\n TEST(test_ctype_isspace(), \"isspace()\");\n TEST(test_ctype_isdigit(), \"isdigit()\");\n ...\n\nBut don't feel strongly about it.\n\nThanks,\nTaylor\n"},{"id":"486343","messageId":"20240105161413.10422-1-ach.lumap@gmail.com","threadId":"60649","inReplyTo":"20240101104017.9452-2-ach.lumap@gmail.com","subject":"[Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2024-01-05T16:14:12Z","receivedAt":"2024-01-05T16:14:58Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"In the recent codebase update (8bf6fbd00d (Merge branch\n'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\nmerged, providing a standardized approach for testing C code. Prior to\nthis update, some unit tests relied on the test helper mechanism,\nlacking a dedicated unit testing framework. It's more natural to perform\nthese unit tests using the new unit test framework.\n\nThis commit migrates the unit tests for C character classification\nfunctions (isdigit(), isspace(), etc) from the legacy approach\nusing the test-tool command `test-tool ctype` in t/helper/test-ctype.c\nto the new unit testing framework (t/unit-tests/test-lib.h).\n\nThe migration involves refactoring the tests to utilize the testing\nmacros provided by the framework (TEST() and check_*()).\n\nMentored-by: Christian Couder \nHelped-by: René Scharfe \nHelped-by: Phillip Wood \nHelped-by: Taylor Blau \nSigned-off-by: Achu Luma \n---\n The changes between version 3 and version 4 are the following:\n\n - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro.\n - A \"0x\"prefix has been added to avoid confusion between decimal and\n hexadecimal codes printed by test_msg().\n - The \"Mentored-by:...\" trailer has been restored.\n - \"works as expected\" has been reduced to just \"works\" as suggested by Taylor.\n - Some \"Helped-by: ...\" trailers have been added.\n - Some whitespace fixes have been made.\n\n Thanks to Junio, René, Phillip and Taylor for commenting on previous versions\n of this patch.\n Here is a diff between v3 and v4:\n\n @@ -14,15 +14,16 @@ static int is_in(const char *s, int ch)\n }\n\n /* Macro to test a character type */\n -#define TEST_CTYPE_FUNC(func, string) \\\n -static void test_ctype_##func(void) \\\n -{ \\\n - for (int i = 0; i < 256; i++) { \\\n - if (!check_int(func(i), ==, is_in(string, i))) \\\n - test_msg(\" i: %02x\", i); \\\n - } \\\n +#define TEST_CTYPE_FUNC(func, string) \\\n +static void test_ctype_##func(void) { \\\n + for (int i = 0; i < 256; i++) { \\\n + if (!check_int(func(i), ==, is_in(string, i))) \\\n + test_msg(\" i: 0x%02x\", i); \\\n + } \\\n }\n\n +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n +\n #define DIGIT \"0123456789\"\n #define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n #define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n @@ -58,20 +59,20 @@ TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n\n int cmd_main(int argc, const char **argv) {\n /* Run all character type tests */\n - TEST(test_ctype_isspace(), \"isspace() works as we expect\");\n - TEST(test_ctype_isdigit(), \"isdigit() works as we expect\");\n - TEST(test_ctype_isalpha(), \"isalpha() works as we expect\");\n - TEST(test_ctype_isalnum(), \"isalnum() works as we expect\");\n - TEST(test_ctype_is_glob_special(), \"is_glob_special() works as we expect\");\n - TEST(test_ctype_is_regex_special(), \"is_regex_special() works as we expect\");\n - TEST(test_ctype_is_pathspec_magic(), \"is_pathspec_magic() works as we expect\");\n - TEST(test_ctype_isascii(), \"isascii() works as we expect\");\n - TEST(test_ctype_islower(), \"islower() works as we expect\");\n - TEST(test_ctype_isupper(), \"isupper() works as we expect\");\n - TEST(test_ctype_iscntrl(), \"iscntrl() works as we expect\");\n - TEST(test_ctype_ispunct(), \"ispunct() works as we expect\");\n - TEST(test_ctype_isxdigit(), \"isxdigit() works as we expect\");\n - TEST(test_ctype_isprint(), \"isprint() works as we expect\");\n + TEST_CHAR_CLASS(isspace);\n + TEST_CHAR_CLASS(isdigit);\n + TEST_CHAR_CLASS(isalpha);\n + TEST_CHAR_CLASS(isalnum);\n + TEST_CHAR_CLASS(is_glob_special);\n + TEST_CHAR_CLASS(is_regex_special);\n + TEST_CHAR_CLASS(is_pathspec_magic);\n + TEST_CHAR_CLASS(isascii);\n + TEST_CHAR_CLASS(islower);\n + TEST_CHAR_CLASS(isupper);\n + TEST_CHAR_CLASS(iscntrl);\n + TEST_CHAR_CLASS(ispunct);\n + TEST_CHAR_CLASS(isxdigit);\n + TEST_CHAR_CLASS(isprint);\n\n Makefile | 2 +-\n t/helper/test-ctype.c | 70 -------------------------------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0070-fundamental.sh | 4 ---\n t/unit-tests/t-ctype.c | 78 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 79 insertions(+), 77 deletions(-)\n delete mode 100644 t/helper/test-ctype.c\n create mode 100644 t/unit-tests/t-ctype.c\n\ndiff --git a/Makefile b/Makefile\nindex 88ba7a3c51..a4df48ba65 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\n TEST_BUILTINS_OBJS += test-csprng.o\n-TEST_BUILTINS_OBJS += test-ctype.o\n TEST_BUILTINS_OBJS += test-date.o\n TEST_BUILTINS_OBJS += test-delta.o\n TEST_BUILTINS_OBJS += test-dir-iterator.o\n@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n\n UNIT_TEST_PROGRAMS += t-basic\n UNIT_TEST_PROGRAMS += t-strbuf\n+UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\ndiff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\ndeleted file mode 100644\nindex e5659df40b..0000000000\n--- a/t/helper/test-ctype.c\n+++ /dev/null\n@@ -1,70 +0,0 @@\n-#include \"test-tool.h\"\n-\n-static int rc;\n-\n-static void report_error(const char *class, int ch)\n-{\n-\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n-\trc = 1;\n-}\n-\n-static int is_in(const char *s, int ch)\n-{\n-\t/*\n-\t * We can't find NUL using strchr. Accept it as the first\n-\t * character in the spec -- there are no empty classes.\n-\t */\n-\tif (ch == '\\0')\n-\t\treturn ch == *s;\n-\tif (*s == '\\0')\n-\t\ts++;\n-\treturn !!strchr(s, ch);\n-}\n-\n-#define TEST_CLASS(t,s) {\t\t\t\\\n-\tint i;\t\t\t\t\t\\\n-\tfor (i = 0; i < 256; i++) {\t\t\\\n-\t\tif (is_in(s, i) != t(i))\t\\\n-\t\t\treport_error(#t, i);\t\\\n-\t}\t\t\t\t\t\\\n-\tif (t(EOF))\t\t\t\t\\\n-\t\treport_error(#t, EOF);\t\t\\\n-}\n-\n-#define DIGIT \"0123456789\"\n-#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n-#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n-#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n-#define ASCII \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n-\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n-\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n-\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n-\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n-\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n-#define CNTRL \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x7f\"\n-\n-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tTEST_CLASS(isdigit, DIGIT);\n-\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n-\tTEST_CLASS(isalpha, LOWER UPPER);\n-\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n-\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n-\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n-\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n-\tTEST_CLASS(isascii, ASCII);\n-\tTEST_CLASS(islower, LOWER);\n-\tTEST_CLASS(isupper, UPPER);\n-\tTEST_CLASS(iscntrl, CNTRL);\n-\tTEST_CLASS(ispunct, PUNCT);\n-\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n-\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n-\n-\treturn rc;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 37ba996539..33b9501c21 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\n \t{ \"csprng\", cmd__csprng },\n-\t{ \"ctype\", cmd__ctype },\n \t{ \"date\", cmd__date },\n \t{ \"delta\", cmd__delta },\n \t{ \"dir-iterator\", cmd__dir_iterator },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 8a1a7c63da..b72f07ded9 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\n int cmd__csprng(int argc, const char **argv);\n-int cmd__ctype(int argc, const char **argv);\n int cmd__date(int argc, const char **argv);\n int cmd__delta(int argc, const char **argv);\n int cmd__dir_iterator(int argc, const char **argv);\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 487bc8d905..a4756fbab9 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n\n-test_expect_success 'character classes (isspace, isalpha etc.)' '\n-\ttest-tool ctype\n-'\n-\n test_expect_success 'mktemp to nonexistent directory prints filename' '\n \ttest_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&\n \tgrep \"doesnotexist/test\" err\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nnew file mode 100644\nindex 0000000000..3a338df541\n--- /dev/null\n+++ b/t/unit-tests/t-ctype.c\n@@ -0,0 +1,78 @@\n+#include \"test-lib.h\"\n+\n+static int is_in(const char *s, int ch)\n+{\n+\t/*\n+\t * We can't find NUL using strchr. Accept it as the first\n+\t * character in the spec -- there are no empty classes.\n+\t */\n+\tif (ch == '\\0')\n+\t\treturn ch == *s;\n+\tif (*s == '\\0')\n+\t\ts++;\n+\treturn !!strchr(s, ch);\n+}\n+\n+/* Macro to test a character type */\n+#define TEST_CTYPE_FUNC(func, string) \\\n+static void test_ctype_##func(void) { \\\n+\tfor (int i = 0; i < 256; i++) { \\\n+\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n+\t\t\ttest_msg(\" i: 0x%02x\", i); \\\n+\t} \\\n+}\n+\n+#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n+\n+#define DIGIT \"0123456789\"\n+#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n+#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n+#define ASCII \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n+\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n+\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n+\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n+\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n+\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n+#define CNTRL \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x7f\"\n+\n+TEST_CTYPE_FUNC(isdigit, DIGIT)\n+TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n+TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n+TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n+TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n+TEST_CTYPE_FUNC(isascii, ASCII)\n+TEST_CTYPE_FUNC(islower, LOWER)\n+TEST_CTYPE_FUNC(isupper, UPPER)\n+TEST_CTYPE_FUNC(iscntrl, CNTRL)\n+TEST_CTYPE_FUNC(ispunct, PUNCT)\n+TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n+\n+int cmd_main(int argc, const char **argv) {\n+\t/* Run all character type tests */\n+\tTEST_CHAR_CLASS(isspace);\n+\tTEST_CHAR_CLASS(isdigit);\n+\tTEST_CHAR_CLASS(isalpha);\n+\tTEST_CHAR_CLASS(isalnum);\n+\tTEST_CHAR_CLASS(is_glob_special);\n+\tTEST_CHAR_CLASS(is_regex_special);\n+\tTEST_CHAR_CLASS(is_pathspec_magic);\n+\tTEST_CHAR_CLASS(isascii);\n+\tTEST_CHAR_CLASS(islower);\n+\tTEST_CHAR_CLASS(isupper);\n+\tTEST_CHAR_CLASS(iscntrl);\n+\tTEST_CHAR_CLASS(ispunct);\n+\tTEST_CHAR_CLASS(isxdigit);\n+\tTEST_CHAR_CLASS(isprint);\n+\n+\treturn test_done();\n+}\n--\n2.42.0.windows.2\n\n"},{"id":"486374","messageId":"a087f57c-ce72-45c7-8182-f38d0aca9030@web.de","threadId":"60649","inReplyTo":"20240105161413.10422-1-ach.lumap@gmail.com","subject":"Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-07T12:45:59Z","receivedAt":"2024-01-07T12:46:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.01.24 um 17:14 schrieb Achu Luma:\n> In the recent codebase update (8bf6fbd00d (Merge branch\n> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\n> merged, providing a standardized approach for testing C code. Prior to\n> this update, some unit tests relied on the test helper mechanism,\n> lacking a dedicated unit testing framework. It's more natural to perform\n> these unit tests using the new unit test framework.\n>\n> This commit migrates the unit tests for C character classification\n> functions (isdigit(), isspace(), etc) from the legacy approach\n> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c\n> to the new unit testing framework (t/unit-tests/test-lib.h).\n>\n> The migration involves refactoring the tests to utilize the testing\n> macros provided by the framework (TEST() and check_*()).\n>\n> Mentored-by: Christian Couder \n> Helped-by: René Scharfe \n> Helped-by: Phillip Wood \n> Helped-by: Taylor Blau \n> Signed-off-by: Achu Luma \n> ---\n\n[snip]\n\n> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\n> deleted file mode 100644\n> index e5659df40b..0000000000\n> --- a/t/helper/test-ctype.c\n> +++ /dev/null\n> @@ -1,70 +0,0 @@\n> -#include \"test-tool.h\"\n> -\n> -static int rc;\n> -\n> -static void report_error(const char *class, int ch)\n> -{\n> -\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n> -\trc = 1;\n> -}\n> -\n> -static int is_in(const char *s, int ch)\n> -{\n> -\t/*\n> -\t * We can't find NUL using strchr. Accept it as the first\n> -\t * character in the spec -- there are no empty classes.\n> -\t */\n> -\tif (ch == '\\0')\n> -\t\treturn ch == *s;\n> -\tif (*s == '\\0')\n> -\t\ts++;\n> -\treturn !!strchr(s, ch);\n> -}\n> -\n> -#define TEST_CLASS(t,s) {\t\t\t\\\n> -\tint i;\t\t\t\t\t\\\n> -\tfor (i = 0; i < 256; i++) {\t\t\\\n> -\t\tif (is_in(s, i) != t(i))\t\\\n> -\t\t\treport_error(#t, i);\t\\\n> -\t}\t\t\t\t\t\\\n> -\tif (t(EOF))\t\t\t\t\\\n> -\t\treport_error(#t, EOF);\t\t\\\n> -}\n> -\n> -#define DIGIT \"0123456789\"\n> -#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n> -#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n> -#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n> -#define ASCII \\\n> -\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> -\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> -\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n> -\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n> -\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n> -\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n> -\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n> -\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n> -#define CNTRL \\\n> -\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> -\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> -\t\"\\x7f\"\n> -\n> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n> -{\n> -\tTEST_CLASS(isdigit, DIGIT);\n> -\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n> -\tTEST_CLASS(isalpha, LOWER UPPER);\n> -\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n> -\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n> -\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n> -\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n> -\tTEST_CLASS(isascii, ASCII);\n> -\tTEST_CLASS(islower, LOWER);\n> -\tTEST_CLASS(isupper, UPPER);\n> -\tTEST_CLASS(iscntrl, CNTRL);\n> -\tTEST_CLASS(ispunct, PUNCT);\n> -\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n> -\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n> -\n> -\treturn rc;\n> -}\n\n[snip]\n\n> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n> new file mode 100644\n> index 0000000000..3a338df541\n> --- /dev/null\n> +++ b/t/unit-tests/t-ctype.c\n> @@ -0,0 +1,78 @@\n> +#include \"test-lib.h\"\n> +\n> +static int is_in(const char *s, int ch)\n> +{\n> +\t/*\n> +\t * We can't find NUL using strchr. Accept it as the first\n> +\t * character in the spec -- there are no empty classes.\n> +\t */\n> +\tif (ch == '\\0')\n> +\t\treturn ch == *s;\n> +\tif (*s == '\\0')\n> +\t\ts++;\n> +\treturn !!strchr(s, ch);\n> +}\n> +\n> +/* Macro to test a character type */\n> +#define TEST_CTYPE_FUNC(func, string) \\\n> +static void test_ctype_##func(void) { \\\n> +\tfor (int i = 0; i < 256; i++) { \\\n> +\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n> +\t\t\ttest_msg(\" i: 0x%02x\", i); \\\n> +\t} \\\n> +}\n> +\n> +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n> +\n> +#define DIGIT \"0123456789\"\n> +#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n> +#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n> +#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n> +#define ASCII \\\n> +\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> +\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> +\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n> +\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n> +\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n> +\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n> +\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n> +\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n> +#define CNTRL \\\n> +\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> +\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> +\t\"\\x7f\"\n> +\n> +TEST_CTYPE_FUNC(isdigit, DIGIT)\n> +TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n> +TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n> +TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n> +TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n> +TEST_CTYPE_FUNC(isascii, ASCII)\n> +TEST_CTYPE_FUNC(islower, LOWER)\n> +TEST_CTYPE_FUNC(isupper, UPPER)\n> +TEST_CTYPE_FUNC(iscntrl, CNTRL)\n> +TEST_CTYPE_FUNC(ispunct, PUNCT)\n> +TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n> +\n> +int cmd_main(int argc, const char **argv) {\n> +\t/* Run all character type tests */\n> +\tTEST_CHAR_CLASS(isspace);\n> +\tTEST_CHAR_CLASS(isdigit);\n> +\tTEST_CHAR_CLASS(isalpha);\n> +\tTEST_CHAR_CLASS(isalnum);\n> +\tTEST_CHAR_CLASS(is_glob_special);\n> +\tTEST_CHAR_CLASS(is_regex_special);\n> +\tTEST_CHAR_CLASS(is_pathspec_magic);\n> +\tTEST_CHAR_CLASS(isascii);\n> +\tTEST_CHAR_CLASS(islower);\n> +\tTEST_CHAR_CLASS(isupper);\n> +\tTEST_CHAR_CLASS(iscntrl);\n> +\tTEST_CHAR_CLASS(ispunct);\n> +\tTEST_CHAR_CLASS(isxdigit);\n> +\tTEST_CHAR_CLASS(isprint);\n> +\n> +\treturn test_done();\n> +}\n> --\n> 2.42.0.windows.2\n>\n\nQuite an improvement over v3! Now you only need to repeat the class\nnames once.\n\nCan we do any better? We could simply have one test per character per\nclass like this:\n\n#define TEST_CHAR_CLASS(class, expect) \\\n\tfor (int i = 0; i < 256; i++) \\\n\t\tTEST(check_int(class(i), ==, is_in(expect, i)),\t\\\n\t\t \"%s(0x%02x) works\", #class, i)\n\nWhich would be used like this:\n\n\tTEST_CHAR_CLASS(isspace, \" \\n\\r\\t\");\n\nWith that there is no need to define any functions anymore. We also\ndon't need any custom output, as the test name includes the character\ncode. Downside: We'd have thousands of tests. But is that actually\na downside or is that how the unit test framework is supposed to be\nused?\n\nIf we need to aggregate the results by class for some reason, we\ncould use strings, like we already do for defining the expected class\nmembers. We need special handling for NUL, as that character\nterminates C strings, but we can put all other characters into a\nstring and then use check_str:\n\n#define TEST_CHAR_CLASS(class, expect) \\\n\tdo { \\\n\t\tint expect_nul = expect[0] == '\\0'; \\\n\t\tchar expect_rest[256] = {0}; \\\n\t\tchar actual_rest[256] = {0}; \\\n\t\tfor (int i = 1, j = 0; i < 256; i++) \\\n\t\t\tif (strchr(&expect[expect_nul], i)) \\\n\t\t\t\texpect_rest[j++] = i; \\\n\t\tfor (int i = 1, j = 0; i < 256; i++) \\\n\t\t\tif (class(i) == 1) \\\n\t\t\t\tactual_rest[j++] = i; \\\n\t\tTEST(check_int(class(0), ==, expect_nul) && \\\n\t\t check_str(actual_rest, expect_rest), \\\n\t\t #class \" works\"); \\\n\t} while (0)\n\ncheck_str escapes non-printable characters when reporting a mismatch,\nso this shouldn't mess up your terminal.\n\nBy the way: Like the original code these checks are stricter than\nrequired by the C standard in requiring the result to be 1 instead of\njust true (any non-zero value). Perhaps they should be relaxed. But\nthat's a tangent and independent of the convergence to a unit test.\n\nRené\n\n"},{"id":"486413","messageId":"xmqqcyubjw23.fsf@gitster.g","threadId":"60649","inReplyTo":"a087f57c-ce72-45c7-8182-f38d0aca9030@web.de","subject":"Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-08T22:32:20Z","receivedAt":"2024-01-08T22:32:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Quite an improvement over v3! Now you only need to repeat the class\n> names once.\n>\n> Can we do any better?\n\n;-)\n"},{"id":"486424","messageId":"33c81610-0958-49da-b702-ba8d96ecf1d3@gmail.com","threadId":"60649","inReplyTo":"20240105161413.10422-1-ach.lumap@gmail.com","subject":"Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-09T10:35:11Z","receivedAt":"2024-01-09T10:35:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Achu\n\nOn 05/01/2024 16:14, Achu Luma wrote:\n> In the recent codebase update (8bf6fbd00d (Merge branch\n> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\n> merged, providing a standardized approach for testing C code. Prior to\n> this update, some unit tests relied on the test helper mechanism,\n> lacking a dedicated unit testing framework. It's more natural to perform\n> these unit tests using the new unit test framework.\n> \n> This commit migrates the unit tests for C character classification\n> functions (isdigit(), isspace(), etc) from the legacy approach\n> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c\n> to the new unit testing framework (t/unit-tests/test-lib.h).\n> \n> The migration involves refactoring the tests to utilize the testing\n> macros provided by the framework (TEST() and check_*()).\n> \n> Mentored-by: Christian Couder \n> Helped-by: René Scharfe \n> Helped-by: Phillip Wood \n> Helped-by: Taylor Blau \n> Signed-off-by: Achu Luma \n> ---\n> The changes between version 3 and version 4 are the following:\n> \n> - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro.\n> - A \"0x\"prefix has been added to avoid confusion between decimal and\n> hexadecimal codes printed by test_msg().\n> - The \"Mentored-by:...\" trailer has been restored.\n> - \"works as expected\" has been reduced to just \"works\" as suggested by Taylor.\n> - Some \"Helped-by: ...\" trailers have been added.\n> - Some whitespace fixes have been made.\n\nThanks for the re-roll, the changes look good, there is one issue that I \nspotted below\n\n> -#define TEST_CLASS(t,s) {\t\t\t\\\n> -\tint i;\t\t\t\t\t\\\n> -\tfor (i = 0; i < 256; i++) {\t\t\\\n> -\t\tif (is_in(s, i) != t(i))\t\\\n> -\t\t\treport_error(#t, i);\t\\\n> -\t}\t\t\t\t\t\\\n> -\tif (t(EOF))\t\t\t\t\\\n> -\t\treport_error(#t, EOF);\t\t\\\n> -}\n\nIn the old version we test EOF in addition to each character between 0-255\n\n> +/* Macro to test a character type */\n> +#define TEST_CTYPE_FUNC(func, string) \\\n> +static void test_ctype_##func(void) { \\\n> +\tfor (int i = 0; i < 256; i++) { \\\n> +\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n> +\t\t\ttest_msg(\" i: 0x%02x\", i); \\\n> +\t} \\\n> +}\n\nIn the new version we only test the characters 0-255, not EOF. If there \nis a good reason for removing the EOF tests then it should be explained \nin the commit message. If not it would be good to add those tests back.\n\nBest Wishes\n\nPhillip\n"},{"id":"486445","messageId":"xmqq5y02juse.fsf@gitster.g","threadId":"60649","inReplyTo":"33c81610-0958-49da-b702-ba8d96ecf1d3@gmail.com","subject":"Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-09T17:12:01Z","receivedAt":"2024-01-09T17:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> In the new version we only test the characters 0-255, not EOF. If\n> there is a good reason for removing the EOF tests then it should be\n> explained in the commit message. If not it would be good to add those\n> tests back.\n\nThanks for sharp eyes. Didn't notice the handling of EOF myself.\n"},{"id":"486705","messageId":"20240112102743.1440-1-ach.lumap@gmail.com","threadId":"60649","inReplyTo":"20240105161413.10422-1-ach.lumap@gmail.com","subject":"[Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Achu Luma","fromEmail":"ach.lumap@gmail.com","sentAt":"2024-01-12T10:27:43Z","receivedAt":"2024-01-12T10:28:40Z","isPatch":true,"sender":{"key":"ach.lumap@gmail.com","avatar":"https://avatars.githubusercontent.com/u/142904668?v=4"},"body":"In the recent codebase update (8bf6fbd00d (Merge branch\n'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\nmerged, providing a standardized approach for testing C code. Prior to\nthis update, some unit tests relied on the test helper mechanism,\nlacking a dedicated unit testing framework. It's more natural to perform\nthese unit tests using the new unit test framework.\n\nThis commit migrates the unit tests for C character classification\nfunctions (isdigit(), isspace(), etc) from the legacy approach\nusing the test-tool command `test-tool ctype` in t/helper/test-ctype.c\nto the new unit testing framework (t/unit-tests/test-lib.h).\n\nThe migration involves refactoring the tests to utilize the testing\nmacros provided by the framework (TEST() and check_*()).\n\nMentored-by: Christian Couder \nHelped-by: René Scharfe \nHelped-by: Phillip Wood \nHelped-by: Taylor Blau \nSigned-off-by: Achu Luma \n---\n The change between version 4 and version 5 is:\n - Added tests to handle EOF.\n\n Thanks to Phillip for noticing the missing tests..\n Here is a diff between v4 and v5:\n\n + if (!check(!func(EOF))) \\\n + test_msg(\" i: 0x%02x (EOF)\", EOF); \\\n\n Thanks also to René, Phillip, Junio and Taylor who helped with\n previous versions.\n\n Makefile | 2 +-\n t/helper/test-ctype.c | 70 ------------------------------------\n t/helper/test-tool.c | 1 -\n t/helper/test-tool.h | 1 -\n t/t0070-fundamental.sh | 4 ---\n t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 81 insertions(+), 77 deletions(-)\n delete mode 100644 t/helper/test-ctype.c\n create mode 100644 t/unit-tests/t-ctype.c\n\ndiff --git a/Makefile b/Makefile\nindex 15990ff312..1a62e48759 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\n TEST_BUILTINS_OBJS += test-crontab.o\n TEST_BUILTINS_OBJS += test-csprng.o\n-TEST_BUILTINS_OBJS += test-ctype.o\n TEST_BUILTINS_OBJS += test-date.o\n TEST_BUILTINS_OBJS += test-delta.o\n TEST_BUILTINS_OBJS += test-dir-iterator.o\n@@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n UNIT_TEST_PROGRAMS += t-basic\n UNIT_TEST_PROGRAMS += t-mem-pool\n UNIT_TEST_PROGRAMS += t-strbuf\n+UNIT_TEST_PROGRAMS += t-ctype\n UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\ndiff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\ndeleted file mode 100644\nindex e5659df40b..0000000000\n--- a/t/helper/test-ctype.c\n+++ /dev/null\n@@ -1,70 +0,0 @@\n-#include \"test-tool.h\"\n-\n-static int rc;\n-\n-static void report_error(const char *class, int ch)\n-{\n-\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n-\trc = 1;\n-}\n-\n-static int is_in(const char *s, int ch)\n-{\n-\t/*\n-\t * We can't find NUL using strchr. Accept it as the first\n-\t * character in the spec -- there are no empty classes.\n-\t */\n-\tif (ch == '\\0')\n-\t\treturn ch == *s;\n-\tif (*s == '\\0')\n-\t\ts++;\n-\treturn !!strchr(s, ch);\n-}\n-\n-#define TEST_CLASS(t,s) {\t\t\t\\\n-\tint i;\t\t\t\t\t\\\n-\tfor (i = 0; i < 256; i++) {\t\t\\\n-\t\tif (is_in(s, i) != t(i))\t\\\n-\t\t\treport_error(#t, i);\t\\\n-\t}\t\t\t\t\t\\\n-\tif (t(EOF))\t\t\t\t\\\n-\t\treport_error(#t, EOF);\t\t\\\n-}\n-\n-#define DIGIT \"0123456789\"\n-#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n-#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n-#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n-#define ASCII \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n-\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n-\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n-\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n-\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n-\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n-#define CNTRL \\\n-\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n-\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n-\t\"\\x7f\"\n-\n-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n-{\n-\tTEST_CLASS(isdigit, DIGIT);\n-\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n-\tTEST_CLASS(isalpha, LOWER UPPER);\n-\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n-\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n-\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n-\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n-\tTEST_CLASS(isascii, ASCII);\n-\tTEST_CLASS(islower, LOWER);\n-\tTEST_CLASS(isupper, UPPER);\n-\tTEST_CLASS(iscntrl, CNTRL);\n-\tTEST_CLASS(ispunct, PUNCT);\n-\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n-\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n-\n-\treturn rc;\n-}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 37ba996539..33b9501c21 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {\n \t{ \"config\", cmd__config },\n \t{ \"crontab\", cmd__crontab },\n \t{ \"csprng\", cmd__csprng },\n-\t{ \"ctype\", cmd__ctype },\n \t{ \"date\", cmd__date },\n \t{ \"delta\", cmd__delta },\n \t{ \"dir-iterator\", cmd__dir_iterator },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 8a1a7c63da..b72f07ded9 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__crontab(int argc, const char **argv);\n int cmd__csprng(int argc, const char **argv);\n-int cmd__ctype(int argc, const char **argv);\n int cmd__date(int argc, const char **argv);\n int cmd__delta(int argc, const char **argv);\n int cmd__dir_iterator(int argc, const char **argv);\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 487bc8d905..a4756fbab9 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n\n-test_expect_success 'character classes (isspace, isalpha etc.)' '\n-\ttest-tool ctype\n-'\n-\n test_expect_success 'mktemp to nonexistent directory prints filename' '\n \ttest_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&\n \tgrep \"doesnotexist/test\" err\ndiff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\nnew file mode 100644\nindex 0000000000..f315489984\n--- /dev/null\n+++ b/t/unit-tests/t-ctype.c\n@@ -0,0 +1,80 @@\n+#include \"test-lib.h\"\n+\n+static int is_in(const char *s, int ch)\n+{\n+\t/*\n+\t * We can't find NUL using strchr. Accept it as the first\n+\t * character in the spec -- there are no empty classes.\n+\t */\n+\tif (ch == '\\0')\n+\t\treturn ch == *s;\n+\tif (*s == '\\0')\n+\t\ts++;\n+\treturn !!strchr(s, ch);\n+}\n+\n+/* Macro to test a character type */\n+#define TEST_CTYPE_FUNC(func, string) \\\n+static void test_ctype_##func(void) { \\\n+\tfor (int i = 0; i < 256; i++) { \\\n+\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n+\t\t\ttest_msg(\" i: 0x%02x\", i); \\\n+\t} \\\n+\tif (!check(!func(EOF))) \\\n+\t\t\ttest_msg(\" i: 0x%02x (EOF)\", EOF); \\\n+}\n+\n+#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n+\n+#define DIGIT \"0123456789\"\n+#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n+#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n+#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n+#define ASCII \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n+\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n+\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n+\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n+\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n+\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n+#define CNTRL \\\n+\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n+\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n+\t\"\\x7f\"\n+\n+TEST_CTYPE_FUNC(isdigit, DIGIT)\n+TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n+TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n+TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n+TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n+TEST_CTYPE_FUNC(isascii, ASCII)\n+TEST_CTYPE_FUNC(islower, LOWER)\n+TEST_CTYPE_FUNC(isupper, UPPER)\n+TEST_CTYPE_FUNC(iscntrl, CNTRL)\n+TEST_CTYPE_FUNC(ispunct, PUNCT)\n+TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n+\n+int cmd_main(int argc, const char **argv) {\n+\t/* Run all character type tests */\n+\tTEST_CHAR_CLASS(isspace);\n+\tTEST_CHAR_CLASS(isdigit);\n+\tTEST_CHAR_CLASS(isalpha);\n+\tTEST_CHAR_CLASS(isalnum);\n+\tTEST_CHAR_CLASS(is_glob_special);\n+\tTEST_CHAR_CLASS(is_regex_special);\n+\tTEST_CHAR_CLASS(is_pathspec_magic);\n+\tTEST_CHAR_CLASS(isascii);\n+\tTEST_CHAR_CLASS(islower);\n+\tTEST_CHAR_CLASS(isupper);\n+\tTEST_CHAR_CLASS(iscntrl);\n+\tTEST_CHAR_CLASS(ispunct);\n+\tTEST_CHAR_CLASS(isxdigit);\n+\tTEST_CHAR_CLASS(isprint);\n+\n+\treturn test_done();\n+}\n--\n2.42.0.windows.2\n\n"},{"id":"486802","messageId":"0d18a95a-543a-41de-8441-c8894d46d380@gmail.com","threadId":"60649","inReplyTo":"20240112102743.1440-1-ach.lumap@gmail.com","subject":"Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-15T10:39:16Z","receivedAt":"2024-01-15T10:39:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Achu\n\nOn 12/01/2024 10:27, Achu Luma wrote:\n> In the recent codebase update (8bf6fbd00d (Merge branch\n> 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was\n> merged, providing a standardized approach for testing C code. Prior to\n> this update, some unit tests relied on the test helper mechanism,\n> lacking a dedicated unit testing framework. It's more natural to perform\n> these unit tests using the new unit test framework.\n> \n> This commit migrates the unit tests for C character classification\n> functions (isdigit(), isspace(), etc) from the legacy approach\n> using the test-tool command `test-tool ctype` in t/helper/test-ctype.c\n> to the new unit testing framework (t/unit-tests/test-lib.h).\n> \n> The migration involves refactoring the tests to utilize the testing\n> macros provided by the framework (TEST() and check_*()).\n> \n> Mentored-by: Christian Couder \n> Helped-by: René Scharfe \n> Helped-by: Phillip Wood \n> Helped-by: Taylor Blau \n> Signed-off-by: Achu Luma \n> ---\n> The change between version 4 and version 5 is:\n> - Added tests to handle EOF.\n> \n> Thanks to Phillip for noticing the missing tests..\n> Here is a diff between v4 and v5:\n> \n> + if (!check(!func(EOF))) \\\n> + test_msg(\" i: 0x%02x (EOF)\", EOF); \\\n\nThanks for adding back the test for EOF, this version looks good to me.\n\nBest Wishes\n\nPhillip\n\n> Thanks also to René, Phillip, Junio and Taylor who helped with\n> previous versions.\n> \n> Makefile | 2 +-\n> t/helper/test-ctype.c | 70 ------------------------------------\n> t/helper/test-tool.c | 1 -\n> t/helper/test-tool.h | 1 -\n> t/t0070-fundamental.sh | 4 ---\n> t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++\n> 6 files changed, 81 insertions(+), 77 deletions(-)\n> delete mode 100644 t/helper/test-ctype.c\n> create mode 100644 t/unit-tests/t-ctype.c\n> \n> diff --git a/Makefile b/Makefile\n> index 15990ff312..1a62e48759 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o\n> TEST_BUILTINS_OBJS += test-config.o\n> TEST_BUILTINS_OBJS += test-crontab.o\n> TEST_BUILTINS_OBJS += test-csprng.o\n> -TEST_BUILTINS_OBJS += test-ctype.o\n> TEST_BUILTINS_OBJS += test-date.o\n> TEST_BUILTINS_OBJS += test-delta.o\n> TEST_BUILTINS_OBJS += test-dir-iterator.o\n> @@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%\n> UNIT_TEST_PROGRAMS += t-basic\n> UNIT_TEST_PROGRAMS += t-mem-pool\n> UNIT_TEST_PROGRAMS += t-strbuf\n> +UNIT_TEST_PROGRAMS += t-ctype\n> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))\n> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))\n> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o\n> diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c\n> deleted file mode 100644\n> index e5659df40b..0000000000\n> --- a/t/helper/test-ctype.c\n> +++ /dev/null\n> @@ -1,70 +0,0 @@\n> -#include \"test-tool.h\"\n> -\n> -static int rc;\n> -\n> -static void report_error(const char *class, int ch)\n> -{\n> -\tprintf(\"%s classifies char %d (0x%02x) wrongly\\n\", class, ch, ch);\n> -\trc = 1;\n> -}\n> -\n> -static int is_in(const char *s, int ch)\n> -{\n> -\t/*\n> -\t * We can't find NUL using strchr. Accept it as the first\n> -\t * character in the spec -- there are no empty classes.\n> -\t */\n> -\tif (ch == '\\0')\n> -\t\treturn ch == *s;\n> -\tif (*s == '\\0')\n> -\t\ts++;\n> -\treturn !!strchr(s, ch);\n> -}\n> -\n> -#define TEST_CLASS(t,s) {\t\t\t\\\n> -\tint i;\t\t\t\t\t\\\n> -\tfor (i = 0; i < 256; i++) {\t\t\\\n> -\t\tif (is_in(s, i) != t(i))\t\\\n> -\t\t\treport_error(#t, i);\t\\\n> -\t}\t\t\t\t\t\\\n> -\tif (t(EOF))\t\t\t\t\\\n> -\t\treport_error(#t, EOF);\t\t\\\n> -}\n> -\n> -#define DIGIT \"0123456789\"\n> -#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n> -#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n> -#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n> -#define ASCII \\\n> -\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> -\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> -\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n> -\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n> -\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n> -\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n> -\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n> -\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n> -#define CNTRL \\\n> -\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> -\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> -\t\"\\x7f\"\n> -\n> -int cmd__ctype(int argc UNUSED, const char **argv UNUSED)\n> -{\n> -\tTEST_CLASS(isdigit, DIGIT);\n> -\tTEST_CLASS(isspace, \" \\n\\r\\t\");\n> -\tTEST_CLASS(isalpha, LOWER UPPER);\n> -\tTEST_CLASS(isalnum, LOWER UPPER DIGIT);\n> -\tTEST_CLASS(is_glob_special, \"*?[\\\\\");\n> -\tTEST_CLASS(is_regex_special, \"$()*+.?[\\\\^{|\");\n> -\tTEST_CLASS(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\");\n> -\tTEST_CLASS(isascii, ASCII);\n> -\tTEST_CLASS(islower, LOWER);\n> -\tTEST_CLASS(isupper, UPPER);\n> -\tTEST_CLASS(iscntrl, CNTRL);\n> -\tTEST_CLASS(ispunct, PUNCT);\n> -\tTEST_CLASS(isxdigit, DIGIT \"abcdefABCDEF\");\n> -\tTEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT \" \");\n> -\n> -\treturn rc;\n> -}\n> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n> index 37ba996539..33b9501c21 100644\n> --- a/t/helper/test-tool.c\n> +++ b/t/helper/test-tool.c\n> @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {\n> \t{ \"config\", cmd__config },\n> \t{ \"crontab\", cmd__crontab },\n> \t{ \"csprng\", cmd__csprng },\n> -\t{ \"ctype\", cmd__ctype },\n> \t{ \"date\", cmd__date },\n> \t{ \"delta\", cmd__delta },\n> \t{ \"dir-iterator\", cmd__dir_iterator },\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index 8a1a7c63da..b72f07ded9 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);\n> int cmd__config(int argc, const char **argv);\n> int cmd__crontab(int argc, const char **argv);\n> int cmd__csprng(int argc, const char **argv);\n> -int cmd__ctype(int argc, const char **argv);\n> int cmd__date(int argc, const char **argv);\n> int cmd__delta(int argc, const char **argv);\n> int cmd__dir_iterator(int argc, const char **argv);\n> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\n> index 487bc8d905..a4756fbab9 100755\n> --- a/t/t0070-fundamental.sh\n> +++ b/t/t0070-fundamental.sh\n> @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.\n> TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n> \n> -test_expect_success 'character classes (isspace, isalpha etc.)' '\n> -\ttest-tool ctype\n> -'\n> -\n> test_expect_success 'mktemp to nonexistent directory prints filename' '\n> \ttest_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&\n> \tgrep \"doesnotexist/test\" err\n> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c\n> new file mode 100644\n> index 0000000000..f315489984\n> --- /dev/null\n> +++ b/t/unit-tests/t-ctype.c\n> @@ -0,0 +1,80 @@\n> +#include \"test-lib.h\"\n> +\n> +static int is_in(const char *s, int ch)\n> +{\n> +\t/*\n> +\t * We can't find NUL using strchr. Accept it as the first\n> +\t * character in the spec -- there are no empty classes.\n> +\t */\n> +\tif (ch == '\\0')\n> +\t\treturn ch == *s;\n> +\tif (*s == '\\0')\n> +\t\ts++;\n> +\treturn !!strchr(s, ch);\n> +}\n> +\n> +/* Macro to test a character type */\n> +#define TEST_CTYPE_FUNC(func, string) \\\n> +static void test_ctype_##func(void) { \\\n> +\tfor (int i = 0; i < 256; i++) { \\\n> +\t\tif (!check_int(func(i), ==, is_in(string, i))) \\\n> +\t\t\ttest_msg(\" i: 0x%02x\", i); \\\n> +\t} \\\n> +\tif (!check(!func(EOF))) \\\n> +\t\t\ttest_msg(\" i: 0x%02x (EOF)\", EOF); \\\n> +}\n> +\n> +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class \" works\")\n> +\n> +#define DIGIT \"0123456789\"\n> +#define LOWER \"abcdefghijklmnopqrstuvwxyz\"\n> +#define UPPER \"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n> +#define PUNCT \"!\\\"#$%&'()*+,-./:;<=>?@[\\\\]^_`{|}~\"\n> +#define ASCII \\\n> +\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> +\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> +\t\"\\x20\\x21\\x22\\x23\\x24\\x25\\x26\\x27\\x28\\x29\\x2a\\x2b\\x2c\\x2d\\x2e\\x2f\" \\\n> +\t\"\\x30\\x31\\x32\\x33\\x34\\x35\\x36\\x37\\x38\\x39\\x3a\\x3b\\x3c\\x3d\\x3e\\x3f\" \\\n> +\t\"\\x40\\x41\\x42\\x43\\x44\\x45\\x46\\x47\\x48\\x49\\x4a\\x4b\\x4c\\x4d\\x4e\\x4f\" \\\n> +\t\"\\x50\\x51\\x52\\x53\\x54\\x55\\x56\\x57\\x58\\x59\\x5a\\x5b\\x5c\\x5d\\x5e\\x5f\" \\\n> +\t\"\\x60\\x61\\x62\\x63\\x64\\x65\\x66\\x67\\x68\\x69\\x6a\\x6b\\x6c\\x6d\\x6e\\x6f\" \\\n> +\t\"\\x70\\x71\\x72\\x73\\x74\\x75\\x76\\x77\\x78\\x79\\x7a\\x7b\\x7c\\x7d\\x7e\\x7f\"\n> +#define CNTRL \\\n> +\t\"\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x0e\\x0f\" \\\n> +\t\"\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f\" \\\n> +\t\"\\x7f\"\n> +\n> +TEST_CTYPE_FUNC(isdigit, DIGIT)\n> +TEST_CTYPE_FUNC(isspace, \" \\n\\r\\t\")\n> +TEST_CTYPE_FUNC(isalpha, LOWER UPPER)\n> +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)\n> +TEST_CTYPE_FUNC(is_glob_special, \"*?[\\\\\")\n> +TEST_CTYPE_FUNC(is_regex_special, \"$()*+.?[\\\\^{|\")\n> +TEST_CTYPE_FUNC(is_pathspec_magic, \"!\\\"#%&',-/:;<=>@_`~\")\n> +TEST_CTYPE_FUNC(isascii, ASCII)\n> +TEST_CTYPE_FUNC(islower, LOWER)\n> +TEST_CTYPE_FUNC(isupper, UPPER)\n> +TEST_CTYPE_FUNC(iscntrl, CNTRL)\n> +TEST_CTYPE_FUNC(ispunct, PUNCT)\n> +TEST_CTYPE_FUNC(isxdigit, DIGIT \"abcdefABCDEF\")\n> +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT \" \")\n> +\n> +int cmd_main(int argc, const char **argv) {\n> +\t/* Run all character type tests */\n> +\tTEST_CHAR_CLASS(isspace);\n> +\tTEST_CHAR_CLASS(isdigit);\n> +\tTEST_CHAR_CLASS(isalpha);\n> +\tTEST_CHAR_CLASS(isalnum);\n> +\tTEST_CHAR_CLASS(is_glob_special);\n> +\tTEST_CHAR_CLASS(is_regex_special);\n> +\tTEST_CHAR_CLASS(is_pathspec_magic);\n> +\tTEST_CHAR_CLASS(isascii);\n> +\tTEST_CHAR_CLASS(islower);\n> +\tTEST_CHAR_CLASS(isupper);\n> +\tTEST_CHAR_CLASS(iscntrl);\n> +\tTEST_CHAR_CLASS(ispunct);\n> +\tTEST_CHAR_CLASS(isxdigit);\n> +\tTEST_CHAR_CLASS(isprint);\n> +\n> +\treturn test_done();\n> +}\n> --\n> 2.42.0.windows.2\n> \n> \n"},{"id":"486845","messageId":"xmqqply147bj.fsf@gitster.g","threadId":"60649","inReplyTo":"0d18a95a-543a-41de-8441-c8894d46d380@gmail.com","subject":"Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T15:38:24Z","receivedAt":"2024-01-16T15:38:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Thanks for adding back the test for EOF, this version looks good to me.\n\nThanks. Let's merge it to 'next'.\n"},{"id":"486860","messageId":"41cf1944-2456-4115-a934-aff2306a26e5@web.de","threadId":"60649","inReplyTo":"xmqqply147bj.fsf@gitster.g","subject":"Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-16T19:27:13Z","receivedAt":"2024-01-16T19:27:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.01.24 um 16:38 schrieb Junio C Hamano:\n> Phillip Wood writes:\n>\n>> Thanks for adding back the test for EOF, this version looks good to me.\n>\n> Thanks. Let's merge it to 'next'.\n\nOK. I'm still interested in replies to my question in\nhttps://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/,\ni.e. whether we should have one TEST per class or one per class and\ncharacter -- or in a broader sense: What's the ideal scope of a TEST?\nBut I can ask it again in the form of a follow-up patch.\n\nRené\n"},{"id":"486861","messageId":"CAP8UFD3=Jf3XAK1kZVNi0iR_9iE-rha-SZb2H2z4UeL51YPekA@mail.gmail.com","threadId":"60649","inReplyTo":"41cf1944-2456-4115-a934-aff2306a26e5@web.de","subject":"Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-16T19:45:09Z","receivedAt":"2024-01-16T19:45:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jan 16, 2024 at 8:27 PM René Scharfe wrote:\n>\n> Am 16.01.24 um 16:38 schrieb Junio C Hamano:\n> > Phillip Wood writes:\n> >\n> >> Thanks for adding back the test for EOF, this version looks good to me.\n> >\n> > Thanks. Let's merge it to 'next'.\n>\n> OK. I'm still interested in replies to my question in\n> https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/,\n> i.e. whether we should have one TEST per class or one per class and\n> character -- or in a broader sense: What's the ideal scope of a TEST?\n> But I can ask it again in the form of a follow-up patch.\n\nI think one test per character per class would result in too much\ndetail in the output. Other than that I think it's better to address\nyour questions to the designers of the unit test framework rather than\nto the authors of this patch. And yeah, sending a follow up patch\nwould perhaps be the best. Thanks.\n"},{"id":"486862","messageId":"xmqq1qah12ej.fsf@gitster.g","threadId":"60649","inReplyTo":"41cf1944-2456-4115-a934-aff2306a26e5@web.de","subject":"Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T19:52:52Z","receivedAt":"2024-01-16T19:52:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 16.01.24 um 16:38 schrieb Junio C Hamano:\n>> Phillip Wood writes:\n>>\n>>> Thanks for adding back the test for EOF, this version looks good to me.\n>>\n>> Thanks. Let's merge it to 'next'.\n>\n> OK. I'm still interested in replies to my question in\n> https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/,\n> i.e. whether we should have one TEST per class or one per class and\n> character -- or in a broader sense: What's the ideal scope of a TEST?\n> But I can ask it again in the form of a follow-up patch.\n\nI personally do not have a good answer, but those who are interested\nin unit-tests more than I do should have their opinions to share ;-)\n"},{"id":"486904","messageId":"ZadnhRtYfrFeMCbX@google.com","threadId":"60649","inReplyTo":"41cf1944-2456-4115-a934-aff2306a26e5@web.de","subject":"Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-01-17T05:37:09Z","receivedAt":"2024-01-17T05:37:15Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.01.16 20:27, René Scharfe wrote:\n> Am 16.01.24 um 16:38 schrieb Junio C Hamano:\n> > Phillip Wood writes:\n> >\n> >> Thanks for adding back the test for EOF, this version looks good to me.\n> >\n> > Thanks. Let's merge it to 'next'.\n> \n> OK. I'm still interested in replies to my question in\n> https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/,\n> i.e. whether we should have one TEST per class or one per class and\n> character -- or in a broader sense: What's the ideal scope of a TEST?\n> But I can ask it again in the form of a follow-up patch.\n> \n> René\n\nI think that the scope of a TEST() should tend small: we want the\nminimal amount of setup required to test the invariants that we're\ninterested in. For this particular unit test, since we're just testing\nsimple predicates on static sets of characters, I would be OK seeing one\nTEST() per class/character. That would certainly make this unit test an\noutlier in the number of checks, but I'm less worried about that since\nthis is testing system-provided functions that we don't expect to change\nregularly.\n\nAdditionally, the elimination of a level of macro indirection makes this\nmore readable IMO.\n"}]}