* [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c @ 2023-12-21 23:15 Achu Luma 2023-12-26 18:45 ` Junio C Hamano 2023-12-30 0:09 ` [Outreachy][PATCH v2] " Achu Luma 0 siblings, 2 replies; 24+ messages in thread From: Achu Luma @ 2023-12-21 23:15 UTC (permalink / raw) To: christian.couder; +Cc: git, Achu Luma, Christian Couder In the recent codebase update (8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework. This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h). The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()). Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Achu Luma <ach.lumap@gmail.com> --- Makefile | 2 +- t/helper/test-ctype.c | 70 -------------------------------------- t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0070-fundamental.sh | 4 --- t/unit-tests/t-ctype.c | 76 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 77 insertions(+), 77 deletions(-) delete mode 100644 t/helper/test-ctype.c create mode 100644 t/unit-tests/t-ctype.c diff --git a/Makefile b/Makefile index 88ba7a3c51..a4df48ba65 100644 --- a/Makefile +++ b/Makefile @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o TEST_BUILTINS_OBJS += test-csprng.o -TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o TEST_BUILTINS_OBJS += test-dir-iterator.o @@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-basic UNIT_TEST_PROGRAMS += t-strbuf +UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c deleted file mode 100644 index e5659df40b..0000000000 --- a/t/helper/test-ctype.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "test-tool.h" - -static int rc; - -static void report_error(const char *class, int ch) -{ - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); - rc = 1; -} - -static int is_in(const char *s, int ch) -{ - /* - * We can't find NUL using strchr. Accept it as the first - * character in the spec -- there are no empty classes. - */ - if (ch == '\0') - return ch == *s; - if (*s == '\0') - s++; - return !!strchr(s, ch); -} - -#define TEST_CLASS(t,s) { \ - int i; \ - for (i = 0; i < 256; i++) { \ - if (is_in(s, i) != t(i)) \ - report_error(#t, i); \ - } \ - if (t(EOF)) \ - report_error(#t, EOF); \ -} - -#define DIGIT "0123456789" -#define LOWER "abcdefghijklmnopqrstuvwxyz" -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" -#define ASCII \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" -#define CNTRL \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x7f" - -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) -{ - TEST_CLASS(isdigit, DIGIT); - TEST_CLASS(isspace, " \n\r\t"); - TEST_CLASS(isalpha, LOWER UPPER); - TEST_CLASS(isalnum, LOWER UPPER DIGIT); - TEST_CLASS(is_glob_special, "*?[\\"); - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); - TEST_CLASS(isascii, ASCII); - TEST_CLASS(islower, LOWER); - TEST_CLASS(isupper, UPPER); - TEST_CLASS(iscntrl, CNTRL); - TEST_CLASS(ispunct, PUNCT); - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); - - return rc; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 37ba996539..33b9501c21 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = { { "config", cmd__config }, { "crontab", cmd__crontab }, { "csprng", cmd__csprng }, - { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, { "dir-iterator", cmd__dir_iterator }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 8a1a7c63da..b72f07ded9 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); int cmd__csprng(int argc, const char **argv); -int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); int cmd__dir_iterator(int argc, const char **argv); diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 487bc8d905..a4756fbab9 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions. TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh -test_expect_success 'character classes (isspace, isalpha etc.)' ' - test-tool ctype -' - test_expect_success 'mktemp to nonexistent directory prints filename' ' test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err && grep "doesnotexist/test" err diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c new file mode 100644 index 0000000000..41189ba9f9 --- /dev/null +++ b/t/unit-tests/t-ctype.c @@ -0,0 +1,76 @@ +#include "test-lib.h" + +static int is_in(const char *s, int ch) +{ + /* + * We can't find NUL using strchr. Accept it as the first + * character in the spec -- there are no empty classes. + */ + if (ch == '\0') + return ch == *s; + if (*s == '\0') + s++; + return !!strchr(s, ch); +} + +/* Macro to test a character type */ +#define TEST_CTYPE_FUNC(func, string) \ +static void test_ctype_##func(void) \ +{ \ + int i; \ + for (i = 0; i < 256; i++) \ + check_int(func(i), ==, is_in(string, i)); \ +} + +#define DIGIT "0123456789" +#define LOWER "abcdefghijklmnopqrstuvwxyz" +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" +#define ASCII \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" +#define CNTRL \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x7f" + +TEST_CTYPE_FUNC(isdigit, DIGIT) +TEST_CTYPE_FUNC(isspace, " \n\r\t") +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") +TEST_CTYPE_FUNC(isascii, ASCII) +TEST_CTYPE_FUNC(islower, LOWER) +TEST_CTYPE_FUNC(isupper, UPPER) +TEST_CTYPE_FUNC(iscntrl, CNTRL) +TEST_CTYPE_FUNC(ispunct, PUNCT) +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") + +int cmd_main(int argc, const char **argv) { + /* Run all character type tests */ + TEST(test_ctype_isspace(), "isspace() works as we expect"); + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); + TEST(test_ctype_isascii(), "isascii() works as we expect"); + TEST(test_ctype_islower(), "islower() works as we expect"); + TEST(test_ctype_isupper(), "isupper() works as we expect"); + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); + TEST(test_ctype_isprint(), "isprint() works as we expect"); + + return test_done(); +} base-commit: 055bb6e9969085777b7fab83e3fee0017654f134 -- 2.42.0.windows.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-21 23:15 [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c Achu Luma @ 2023-12-26 18:45 ` Junio C Hamano 2023-12-27 10:57 ` Christian Couder 2024-01-02 18:55 ` Taylor Blau 2023-12-30 0:09 ` [Outreachy][PATCH v2] " Achu Luma 1 sibling, 2 replies; 24+ messages in thread From: Junio C Hamano @ 2023-12-26 18:45 UTC (permalink / raw) To: Achu Luma; +Cc: christian.couder, git, Christian Couder Achu Luma <ach.lumap@gmail.com> writes: > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c > deleted file mode 100644 > index e5659df40b..0000000000 > --- a/t/helper/test-ctype.c > +++ /dev/null > @@ -1,70 +0,0 @@ > -#include "test-tool.h" > - > -static int rc; > - > -static void report_error(const char *class, int ch) > -{ > - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); > - rc = 1; > -} So, if we have a is_foo() that characterises a byte that ought to be "foo" but gets miscategorised not to be "foo", we used to pinpoint exactly the byte value that was an issue. We did not do any early return ... > ... > -#define TEST_CLASS(t,s) { \ > - int i; \ > - for (i = 0; i < 256; i++) { \ > - if (is_in(s, i) != t(i)) \ > - report_error(#t, i); \ > - } \ > - if (t(EOF)) \ > - report_error(#t, EOF); \ > -} ... and reported for all errors in the "class". > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c > new file mode 100644 > index 0000000000..41189ba9f9 > --- /dev/null > +++ b/t/unit-tests/t-ctype.c > @@ -0,0 +1,76 @@ > +#include "test-lib.h" > + > +static int is_in(const char *s, int ch) > +{ > + /* > + * We can't find NUL using strchr. Accept it as the first > + * character in the spec -- there are no empty classes. > + */ > + if (ch == '\0') > + return ch == *s; > + if (*s == '\0') > + s++; > + return !!strchr(s, ch); > +} > + > +/* Macro to test a character type */ > +#define TEST_CTYPE_FUNC(func, string) \ > +static void test_ctype_##func(void) \ > +{ \ > + int i; \ > + for (i = 0; i < 256; i++) \ > + check_int(func(i), ==, is_in(string, i)); \ > +} Now, we let check_int() to do the checking for each and every byte value for the class. check_int() uses different reporting and shows the problematic value in a way that is more verbose and at the same time is a less specific and harder to understand: test_msg(" left: %"PRIdMAX, a); test_msg(" right: %"PRIdMAX, b); But that is probably the price to pay to use a more generic framework, I guess. > +int cmd_main(int argc, const char **argv) { > + /* Run all character type tests */ > + TEST(test_ctype_isspace(), "isspace() works as we expect"); > + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); > + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); > + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); > + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); > + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); > + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); > + TEST(test_ctype_isascii(), "isascii() works as we expect"); > + TEST(test_ctype_islower(), "islower() works as we expect"); > + TEST(test_ctype_isupper(), "isupper() works as we expect"); > + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); > + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); > + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); > + TEST(test_ctype_isprint(), "isprint() works as we expect"); > + > + return test_done(); > +} As a practice to use the unit-tests framework, the patch looks OK. helper/test-ctype.c indeed is an oddball that runs once and checks everything it wants to check, for which the unit tests framework is much more suited. Let's see how others react and then queue. Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-26 18:45 ` Junio C Hamano @ 2023-12-27 10:57 ` Christian Couder 2023-12-27 11:57 ` René Scharfe 2024-01-02 18:55 ` Taylor Blau 1 sibling, 1 reply; 24+ messages in thread From: Christian Couder @ 2023-12-27 10:57 UTC (permalink / raw) To: Junio C Hamano Cc: Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote: > > Achu Luma <ach.lumap@gmail.com> writes: > > +/* Macro to test a character type */ > > +#define TEST_CTYPE_FUNC(func, string) \ > > +static void test_ctype_##func(void) \ > > +{ \ > > + int i; \ > > + for (i = 0; i < 256; i++) \ > > + check_int(func(i), ==, is_in(string, i)); \ > > +} > > Now, we let check_int() to do the checking for each and every byte > value for the class. check_int() uses different reporting and shows > the problematic value in a way that is more verbose and at the same > time is a less specific and harder to understand: > > test_msg(" left: %"PRIdMAX, a); > test_msg(" right: %"PRIdMAX, b); > > But that is probably the price to pay to use a more generic > framework, I guess. I have added Phillip and Josh in Cc: as they might have ideas about this. Also it might not be a big issue here, but when the new unit test framework was proposed, I commented on the fact that "left" and "right" were perhaps a bit less explicit than "actual" and "expected". > > +int cmd_main(int argc, const char **argv) { > > + /* Run all character type tests */ > > + TEST(test_ctype_isspace(), "isspace() works as we expect"); > > + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); > > + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); > > + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); > > + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); > > + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); > > + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); > > + TEST(test_ctype_isascii(), "isascii() works as we expect"); > > + TEST(test_ctype_islower(), "islower() works as we expect"); > > + TEST(test_ctype_isupper(), "isupper() works as we expect"); > > + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); > > + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); > > + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); > > + TEST(test_ctype_isprint(), "isprint() works as we expect"); > > + > > + return test_done(); > > +} > > As a practice to use the unit-tests framework, the patch looks OK. > helper/test-ctype.c indeed is an oddball that runs once and checks > everything it wants to check, for which the unit tests framework is > much more suited. Yeah, I agree. > Let's see how others react and then queue. > > Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-27 10:57 ` Christian Couder @ 2023-12-27 11:57 ` René Scharfe 2023-12-27 14:40 ` Phillip Wood 2023-12-27 23:48 ` Junio C Hamano 0 siblings, 2 replies; 24+ messages in thread From: René Scharfe @ 2023-12-27 11:57 UTC (permalink / raw) To: Christian Couder, Junio C Hamano Cc: Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon Am 27.12.23 um 11:57 schrieb Christian Couder: > On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Achu Luma <ach.lumap@gmail.com> writes: > >>> +/* Macro to test a character type */ >>> +#define TEST_CTYPE_FUNC(func, string) \ >>> +static void test_ctype_##func(void) \ >>> +{ \ >>> + int i; \ >>> + for (i = 0; i < 256; i++) \ >>> + check_int(func(i), ==, is_in(string, i)); \ >>> +} >> >> Now, we let check_int() to do the checking for each and every byte >> value for the class. check_int() uses different reporting and shows >> the problematic value in a way that is more verbose and at the same >> time is a less specific and harder to understand: >> >> test_msg(" left: %"PRIdMAX, a); >> test_msg(" right: %"PRIdMAX, b); >> >> But that is probably the price to pay to use a more generic >> framework, I guess. > > I have added Phillip and Josh in Cc: as they might have ideas about this. You can write custom messages for custom tests using test_assert(). > Also it might not be a big issue here, but when the new unit test > framework was proposed, I commented on the fact that "left" and > "right" were perhaps a bit less explicit than "actual" and "expected". True. >>> +int cmd_main(int argc, const char **argv) { >>> + /* Run all character type tests */ >>> + TEST(test_ctype_isspace(), "isspace() works as we expect"); >>> + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); >>> + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); >>> + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); >>> + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); >>> + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); >>> + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); >>> + TEST(test_ctype_isascii(), "isascii() works as we expect"); >>> + TEST(test_ctype_islower(), "islower() works as we expect"); >>> + TEST(test_ctype_isupper(), "isupper() works as we expect"); >>> + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); >>> + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); >>> + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); >>> + TEST(test_ctype_isprint(), "isprint() works as we expect"); >>> + >>> + return test_done(); >>> +} >> >> As a practice to use the unit-tests framework, the patch looks OK. The added repetition is a bit grating. With a bit of setup, loop unrolling and stringification you can retain the property of only having to mention the class name once. Demo patch below. >> helper/test-ctype.c indeed is an oddball that runs once and checks >> everything it wants to check, for which the unit tests framework is >> much more suited. > > Yeah, I agree. Indeed: test-ctype does unit tests, so the unit test framework should be a perfect fit. It still feels a bit raw that this point, though, but that's to be expected. René diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c index 41189ba9f9..7903856cec 100644 --- a/t/unit-tests/t-ctype.c +++ b/t/unit-tests/t-ctype.c @@ -13,13 +13,23 @@ static int is_in(const char *s, int ch) return !!strchr(s, ch); } -/* Macro to test a character type */ -#define TEST_CTYPE_FUNC(func, string) \ -static void test_ctype_##func(void) \ -{ \ - int i; \ - for (i = 0; i < 256; i++) \ - check_int(func(i), ==, is_in(string, i)); \ +struct ctype { + const char *name; + const char *expect; + int actual[256]; +}; + +static void test_ctype(const struct ctype *class) +{ + for (int i = 0; i < 256; i++) { + int expect = is_in(class->expect, i); + int actual = class->actual[i]; + int res = test_assert(TEST_LOCATION(), class->name, + actual == expect); + if (!res) + test_msg("%s classifies char %d (0x%02x) wrongly", + class->name, i, i); + } } #define DIGIT "0123456789" @@ -40,37 +50,39 @@ static void test_ctype_##func(void) \ "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ "\x7f" -TEST_CTYPE_FUNC(isdigit, DIGIT) -TEST_CTYPE_FUNC(isspace, " \n\r\t") -TEST_CTYPE_FUNC(isalpha, LOWER UPPER) -TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) -TEST_CTYPE_FUNC(is_glob_special, "*?[\\") -TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") -TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") -TEST_CTYPE_FUNC(isascii, ASCII) -TEST_CTYPE_FUNC(islower, LOWER) -TEST_CTYPE_FUNC(isupper, UPPER) -TEST_CTYPE_FUNC(iscntrl, CNTRL) -TEST_CTYPE_FUNC(ispunct, PUNCT) -TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") -TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") +#define APPLY16(f, n) \ + f(n + 0x0), f(n + 0x1), f(n + 0x2), f(n + 0x3), \ + f(n + 0x4), f(n + 0x5), f(n + 0x6), f(n + 0x7), \ + f(n + 0x8), f(n + 0x9), f(n + 0xa), f(n + 0xb), \ + f(n + 0xc), f(n + 0xd), f(n + 0xe), f(n + 0xf) +#define APPLY256(f) \ + APPLY16(f, 0x00), APPLY16(f, 0x10), APPLY16(f, 0x20), APPLY16(f, 0x30),\ + APPLY16(f, 0x40), APPLY16(f, 0x50), APPLY16(f, 0x60), APPLY16(f, 0x70),\ + APPLY16(f, 0x80), APPLY16(f, 0x90), APPLY16(f, 0xa0), APPLY16(f, 0xb0),\ + APPLY16(f, 0xc0), APPLY16(f, 0xd0), APPLY16(f, 0xe0), APPLY16(f, 0xf0),\ + +#define CTYPE(name, expect) { #name, expect, { APPLY256(name) } } int cmd_main(int argc, const char **argv) { + struct ctype classes[] = { + CTYPE(isdigit, DIGIT), + CTYPE(isspace, " \n\r\t"), + CTYPE(isalpha, LOWER UPPER), + CTYPE(isalnum, LOWER UPPER DIGIT), + CTYPE(is_glob_special, "*?[\\"), + CTYPE(is_regex_special, "$()*+.?[\\^{|"), + CTYPE(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"), + CTYPE(isascii, ASCII), + CTYPE(islower, LOWER), + CTYPE(isupper, UPPER), + CTYPE(iscntrl, CNTRL), + CTYPE(ispunct, PUNCT), + CTYPE(isxdigit, DIGIT "abcdefABCDEF"), + CTYPE(isprint, LOWER UPPER DIGIT PUNCT " "), + }; /* Run all character type tests */ - TEST(test_ctype_isspace(), "isspace() works as we expect"); - TEST(test_ctype_isdigit(), "isdigit() works as we expect"); - TEST(test_ctype_isalpha(), "isalpha() works as we expect"); - TEST(test_ctype_isalnum(), "isalnum() works as we expect"); - TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); - TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); - TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); - TEST(test_ctype_isascii(), "isascii() works as we expect"); - TEST(test_ctype_islower(), "islower() works as we expect"); - TEST(test_ctype_isupper(), "isupper() works as we expect"); - TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); - TEST(test_ctype_ispunct(), "ispunct() works as we expect"); - TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); - TEST(test_ctype_isprint(), "isprint() works as we expect"); + for (int i = 0; i < ARRAY_SIZE(classes); i++) + TEST(test_ctype(&classes[i]), "%s works", classes[i].name); return test_done(); } ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-27 11:57 ` René Scharfe @ 2023-12-27 14:40 ` Phillip Wood 2023-12-27 23:48 ` Junio C Hamano 1 sibling, 0 replies; 24+ messages in thread From: Phillip Wood @ 2023-12-27 14:40 UTC (permalink / raw) To: René Scharfe, Christian Couder, Junio C Hamano Cc: Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon On 27/12/2023 11:57, René Scharfe wrote: > Am 27.12.23 um 11:57 schrieb Christian Couder: >> On Tue, Dec 26, 2023 at 7:46 PM Junio C Hamano <gitster@pobox.com> wrote: >>> >>> Achu Luma <ach.lumap@gmail.com> writes: >> >>>> +/* Macro to test a character type */ >>>> +#define TEST_CTYPE_FUNC(func, string) \ >>>> +static void test_ctype_##func(void) \ >>>> +{ \ >>>> + int i; \ >>>> + for (i = 0; i < 256; i++) \ >>>> + check_int(func(i), ==, is_in(string, i)); \ >>>> +} >>> >>> Now, we let check_int() to do the checking for each and every byte >>> value for the class. check_int() uses different reporting and shows >>> the problematic value in a way that is more verbose and at the same >>> time is a less specific and harder to understand: >>> >>> test_msg(" left: %"PRIdMAX, a); >>> test_msg(" right: %"PRIdMAX, b); >>> >>> But that is probably the price to pay to use a more generic >>> framework, I guess. >> >> I have added Phillip and Josh in Cc: as they might have ideas about this. > > You can write custom messages for custom tests using test_assert(). Another possibility is to do for (int i = 0; i < 256; i++) { if (!check_int(func(i), ==, is_in(string, i)) test_msg(" i: %02x", i); } To print the character code as well as the actual and expected return values of check_int(). The funny spacing is intended to keep the output aligned. I did wonder if we should be using check(func(i) == is_in(string, i)) instead of check_int() but I think it is useful to have the return value printed on error in case we start returning "53" instead of "1" for "true" [1]. With the extra test_msg() above we can now see if the test fails because of a mis-categorization or because func() returned a different non-zero value when we were expecting "1". >> Also it might not be a big issue here, but when the new unit test >> framework was proposed, I commented on the fact that "left" and >> "right" were perhaps a bit less explicit than "actual" and "expected". If people are worried about this then it would be possible to change the check_xxx() macros pass the stringified relational operator into the various check_xxx_loc() functions and then print "expected" and "actual" when the operator is "==" and "left" and "right" otherwise. Best Wishes Phillip [1] As an aside I wonder if the ctype functions would make good test balloons for using _Bool by changing sane_istest() to be #define sane_istest(x,mask) ((bool)(sane_ctype[(unsigned char)(x)] & (mask))) so that we check casting to _Bool coerces non-zero values to "1" ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-27 11:57 ` René Scharfe 2023-12-27 14:40 ` Phillip Wood @ 2023-12-27 23:48 ` Junio C Hamano 2023-12-28 16:05 ` René Scharfe 1 sibling, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2023-12-27 23:48 UTC (permalink / raw) To: René Scharfe Cc: Christian Couder, Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon René Scharfe <l.s.r@web.de> writes: >> Also it might not be a big issue here, but when the new unit test >> framework was proposed, I commented on the fact that "left" and >> "right" were perhaps a bit less explicit than "actual" and "expected". > > True. > ... > The added repetition is a bit grating. With a bit of setup, loop > unrolling and stringification you can retain the property of only having > to mention the class name once. Demo patch below. Nice. This (and your mempool thing) being one of the early efforts to adopt the unit-test framework outside the initial set of sample tests, it is understandable that we might find what framework offers is still lacking. But at the same time, while the macro tricks demonstrated here are all amusing to read and admire, it feels a bit too much to expect that the test writers are willing to invent something like these every time they want to test. Being a relatively faithful conversion of the original ctype tests, with its thorough enumeration of test samples and expected output, is what makes this test program require these macro tricks, and it does not have much to do with the features (or lack thereof) of the framework, I guess. > +struct ctype { > + const char *name; > + const char *expect; > + int actual[256]; > +}; > + > +static void test_ctype(const struct ctype *class) > +{ > + for (int i = 0; i < 256; i++) { > + int expect = is_in(class->expect, i); > + int actual = class->actual[i]; > + int res = test_assert(TEST_LOCATION(), class->name, > + actual == expect); > + if (!res) > + test_msg("%s classifies char %d (0x%02x) wrongly", > + class->name, i, i); > + } > } Somehow, the "test_assert" does not seem to be adding much value here (i.e. we can do "res = (actual == expect)" there). Is this because we want to be able to report success, too? ... goes and looks at test_assert() ... Ah, is it because we want to be able to "skip" (which pretends that the assert() was satisified). OK, but then the error reporting from it is redundant with our own test_msg(). Everything below this line was a fun read ;-) Thanks. > ... > +#define APPLY16(f, n) \ > + f(n + 0x0), f(n + 0x1), f(n + 0x2), f(n + 0x3), \ > + f(n + 0x4), f(n + 0x5), f(n + 0x6), f(n + 0x7), \ > + f(n + 0x8), f(n + 0x9), f(n + 0xa), f(n + 0xb), \ > + f(n + 0xc), f(n + 0xd), f(n + 0xe), f(n + 0xf) > +#define APPLY256(f) \ > + APPLY16(f, 0x00), APPLY16(f, 0x10), APPLY16(f, 0x20), APPLY16(f, 0x30),\ > + APPLY16(f, 0x40), APPLY16(f, 0x50), APPLY16(f, 0x60), APPLY16(f, 0x70),\ > + APPLY16(f, 0x80), APPLY16(f, 0x90), APPLY16(f, 0xa0), APPLY16(f, 0xb0),\ > + APPLY16(f, 0xc0), APPLY16(f, 0xd0), APPLY16(f, 0xe0), APPLY16(f, 0xf0),\ > + > +#define CTYPE(name, expect) { #name, expect, { APPLY256(name) } } > > int cmd_main(int argc, const char **argv) { > + struct ctype classes[] = { > + CTYPE(isdigit, DIGIT), > + CTYPE(isspace, " \n\r\t"), > + CTYPE(isalpha, LOWER UPPER), > + CTYPE(isalnum, LOWER UPPER DIGIT), > + CTYPE(is_glob_special, "*?[\\"), > + CTYPE(is_regex_special, "$()*+.?[\\^{|"), > + CTYPE(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"), > + CTYPE(isascii, ASCII), > + CTYPE(islower, LOWER), > + CTYPE(isupper, UPPER), > + CTYPE(iscntrl, CNTRL), > + CTYPE(ispunct, PUNCT), > + CTYPE(isxdigit, DIGIT "abcdefABCDEF"), > + CTYPE(isprint, LOWER UPPER DIGIT PUNCT " "), > + }; > /* Run all character type tests */ > - TEST(test_ctype_isspace(), "isspace() works as we expect"); > - TEST(test_ctype_isdigit(), "isdigit() works as we expect"); > - TEST(test_ctype_isalpha(), "isalpha() works as we expect"); > - TEST(test_ctype_isalnum(), "isalnum() works as we expect"); > - TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); > - TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); > - TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); > - TEST(test_ctype_isascii(), "isascii() works as we expect"); > - TEST(test_ctype_islower(), "islower() works as we expect"); > - TEST(test_ctype_isupper(), "isupper() works as we expect"); > - TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); > - TEST(test_ctype_ispunct(), "ispunct() works as we expect"); > - TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); > - TEST(test_ctype_isprint(), "isprint() works as we expect"); > + for (int i = 0; i < ARRAY_SIZE(classes); i++) > + TEST(test_ctype(&classes[i]), "%s works", classes[i].name); > > return test_done(); > } ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-27 23:48 ` Junio C Hamano @ 2023-12-28 16:05 ` René Scharfe 0 siblings, 0 replies; 24+ messages in thread From: René Scharfe @ 2023-12-28 16:05 UTC (permalink / raw) To: Junio C Hamano Cc: Christian Couder, Achu Luma, git, Christian Couder, Phillip Wood, Josh Steadmon Am 28.12.23 um 00:48 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >>> Also it might not be a big issue here, but when the new unit test >>> framework was proposed, I commented on the fact that "left" and >>> "right" were perhaps a bit less explicit than "actual" and "expected". >> >> True. >> ... >> The added repetition is a bit grating. With a bit of setup, loop >> unrolling and stringification you can retain the property of only having >> to mention the class name once. Demo patch below. > > Nice. > > This (and your mempool thing) being one of the early efforts to > adopt the unit-test framework outside the initial set of sample > tests, it is understandable that we might find what framework offers > is still lacking. But at the same time, while the macro tricks > demonstrated here are all amusing to read and admire, it feels a bit > too much to expect that the test writers are willing to invent > something like these every time they want to test. > > Being a relatively faithful conversion of the original ctype tests, > with its thorough enumeration of test samples and expected output, > is what makes this test program require these macro tricks, and it > does not have much to do with the features (or lack thereof) of the > framework, I guess. *nod* > >> +struct ctype { >> + const char *name; >> + const char *expect; >> + int actual[256]; >> +}; >> + >> +static void test_ctype(const struct ctype *class) >> +{ >> + for (int i = 0; i < 256; i++) { >> + int expect = is_in(class->expect, i); >> + int actual = class->actual[i]; >> + int res = test_assert(TEST_LOCATION(), class->name, >> + actual == expect); >> + if (!res) >> + test_msg("%s classifies char %d (0x%02x) wrongly", >> + class->name, i, i); >> + } >> } > > Somehow, the "test_assert" does not seem to be adding much value > here (i.e. we can do "res = (actual == expect)" there). Is this > because we want to be able to report success, too? > > ... goes and looks at test_assert() ... > > Ah, is it because we want to be able to "skip" (which pretends that > the assert() was satisified). OK, but then the error reporting from > it is redundant with our own test_msg(). True, the test_msg() emits the old message here, but it doesn't have to report that the check failed anymore, because test_assert() already covers that part. It would only have to report the misclassified character and perhaps the expected result. René ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-26 18:45 ` Junio C Hamano 2023-12-27 10:57 ` Christian Couder @ 2024-01-02 18:55 ` Taylor Blau 1 sibling, 0 replies; 24+ messages in thread From: Taylor Blau @ 2024-01-02 18:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Achu Luma, christian.couder, git, Christian Couder On Tue, Dec 26, 2023 at 10:45:59AM -0800, Junio C Hamano wrote: > Achu Luma <ach.lumap@gmail.com> writes: > > > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c > > deleted file mode 100644 > > index e5659df40b..0000000000 > > --- a/t/helper/test-ctype.c > > +++ /dev/null > > @@ -1,70 +0,0 @@ > > -#include "test-tool.h" > > - > > -static int rc; > > - > > -static void report_error(const char *class, int ch) > > -{ > > - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); > > - rc = 1; > > -} > > So, if we have a is_foo() that characterises a byte that ought to > be "foo" but gets miscategorised not to be "foo", we used to > pinpoint exactly the byte value that was an issue. We did not do > any early return ... > > > ... > > -#define TEST_CLASS(t,s) { \ > > - int i; \ > > - for (i = 0; i < 256; i++) { \ > > - if (is_in(s, i) != t(i)) \ > > - report_error(#t, i); \ > > - } \ > > - if (t(EOF)) \ > > - report_error(#t, EOF); \ > > -} > > ... and reported for all errors in the "class". > > > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c > > new file mode 100644 > > index 0000000000..41189ba9f9 > > --- /dev/null > > +++ b/t/unit-tests/t-ctype.c > > @@ -0,0 +1,76 @@ > > +#include "test-lib.h" > > + > > +static int is_in(const char *s, int ch) > > +{ > > + /* > > + * We can't find NUL using strchr. Accept it as the first > > + * character in the spec -- there are no empty classes. > > + */ > > + if (ch == '\0') > > + return ch == *s; > > + if (*s == '\0') > > + s++; > > + return !!strchr(s, ch); > > +} > > + > > +/* Macro to test a character type */ > > +#define TEST_CTYPE_FUNC(func, string) \ > > +static void test_ctype_##func(void) \ > > +{ \ > > + int i; \ > > + for (i = 0; i < 256; i++) \ > > + check_int(func(i), ==, is_in(string, i)); \ > > +} > > Now, we let check_int() to do the checking for each and every byte > value for the class. check_int() uses different reporting and shows > the problematic value in a way that is more verbose and at the same > time is a less specific and harder to understand: > > test_msg(" left: %"PRIdMAX, a); > test_msg(" right: %"PRIdMAX, b); > > But that is probably the price to pay to use a more generic > framework, I guess. Perhaps I'm missing something here, since I haven't followed the unit-test effort very closely, but this check_int() macro feels like it might be overkill for what we're trying to do. We know that the expected value is the result of is_in(string, i), so I wonder if we might benefit from having an "assert_equals()" that looks like: assert_equals(is_in(string, i), func(i)); Where we follow the usual convention of treating the first argument as the expected value, and the second as the actual value. Then we could format our error message to be more specific, like: test_msg("expected %d, got %d", expected, actual); I think that this would be a little more readable, and still seems flexible enough to support the kind of thing that check_int(..., ==, ...) is after. > > +int cmd_main(int argc, const char **argv) { > > + /* Run all character type tests */ > > + TEST(test_ctype_isspace(), "isspace() works as we expect"); > > + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); > > + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); > > + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); > > + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); > > + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); > > + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); > > + TEST(test_ctype_isascii(), "isascii() works as we expect"); > > + TEST(test_ctype_islower(), "islower() works as we expect"); > > + TEST(test_ctype_isupper(), "isupper() works as we expect"); > > + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); > > + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); > > + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); > > + TEST(test_ctype_isprint(), "isprint() works as we expect"); > > + > > + return test_done(); > > +} > > As a practice to use the unit-tests framework, the patch looks OK. > helper/test-ctype.c indeed is an oddball that runs once and checks > everything it wants to check, for which the unit tests framework is > much more suited. As an aside, I don't think we need the "works as we expect" suffix in each test description. I personally would be fine with something like: TEST(test_ctype_isspace(), "isspace()"); TEST(test_ctype_isdigit(), "isdigit()"); ... But don't feel strongly about it. Thanks, Taylor ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Outreachy][PATCH v2] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-21 23:15 [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c Achu Luma 2023-12-26 18:45 ` Junio C Hamano @ 2023-12-30 0:09 ` Achu Luma 2024-01-01 10:40 ` [Outreachy][PATCH v3] " Achu Luma 1 sibling, 1 reply; 24+ messages in thread From: Achu Luma @ 2023-12-30 0:09 UTC (permalink / raw) To: git Cc: chriscool, christian.couder, l.s.r, gitster, phillip.wood, steadmon, Achu Luma In the recent codebase update(8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework. This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h). The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()). Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Achu Luma <ach.lumap@gmail.com> --- In the revised patch, several improvements were made to the TEST_CTYPE_FUNC macro. The updated version enhances error handling by utilizing a direct comparison approach between the func and is_in(string, i) functions across ASCII characters. Additionally, the loop control variable i is locally scoped within the loop, ensuring proper encapsulation. These changes streamline the comparison process and clarify failure reporting. Special thanks to the invaluable reviews by Junio, Phillip and René for their insightful feedback and thorough review of the patch, contributing significantly to these changes. Makefile | 2 +- t/helper/test-ctype.c | 70 -------------------------------------- t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0070-fundamental.sh | 4 --- t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 78 insertions(+), 77 deletions(-) delete mode 100644 t/helper/test-ctype.c create mode 100644 t/unit-tests/t-ctype.c diff --git a/Makefile b/Makefile index 88ba7a3c51..a4df48ba65 100644 --- a/Makefile +++ b/Makefile @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o TEST_BUILTINS_OBJS += test-csprng.o -TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o TEST_BUILTINS_OBJS += test-dir-iterator.o @@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-basic UNIT_TEST_PROGRAMS += t-strbuf +UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c deleted file mode 100644 index e5659df40b..0000000000 --- a/t/helper/test-ctype.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "test-tool.h" - -static int rc; - -static void report_error(const char *class, int ch) -{ - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); - rc = 1; -} - -static int is_in(const char *s, int ch) -{ - /* - * We can't find NUL using strchr. Accept it as the first - * character in the spec -- there are no empty classes. - */ - if (ch == '\0') - return ch == *s; - if (*s == '\0') - s++; - return !!strchr(s, ch); -} - -#define TEST_CLASS(t,s) { \ - int i; \ - for (i = 0; i < 256; i++) { \ - if (is_in(s, i) != t(i)) \ - report_error(#t, i); \ - } \ - if (t(EOF)) \ - report_error(#t, EOF); \ -} - -#define DIGIT "0123456789" -#define LOWER "abcdefghijklmnopqrstuvwxyz" -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" -#define ASCII \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" -#define CNTRL \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x7f" - -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) -{ - TEST_CLASS(isdigit, DIGIT); - TEST_CLASS(isspace, " \n\r\t"); - TEST_CLASS(isalpha, LOWER UPPER); - TEST_CLASS(isalnum, LOWER UPPER DIGIT); - TEST_CLASS(is_glob_special, "*?[\\"); - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); - TEST_CLASS(isascii, ASCII); - TEST_CLASS(islower, LOWER); - TEST_CLASS(isupper, UPPER); - TEST_CLASS(iscntrl, CNTRL); - TEST_CLASS(ispunct, PUNCT); - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); - - return rc; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 37ba996539..33b9501c21 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = { { "config", cmd__config }, { "crontab", cmd__crontab }, { "csprng", cmd__csprng }, - { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, { "dir-iterator", cmd__dir_iterator }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 8a1a7c63da..b72f07ded9 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); int cmd__csprng(int argc, const char **argv); -int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); int cmd__dir_iterator(int argc, const char **argv); diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 487bc8d905..a4756fbab9 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions. TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh -test_expect_success 'character classes (isspace, isalpha etc.)' ' - test-tool ctype -' - test_expect_success 'mktemp to nonexistent directory prints filename' ' test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err && grep "doesnotexist/test" err diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c new file mode 100644 index 0000000000..8a215d387a --- /dev/null +++ b/t/unit-tests/t-ctype.c @@ -0,0 +1,77 @@ +#include "test-lib.h" + +static int is_in(const char *s, int ch) +{ + /* + * We can't find NUL using strchr. Accept it as the first + * character in the spec -- there are no empty classes. + */ + if (ch == '\0') + return ch == *s; + if (*s == '\0') + s++; + return !!strchr(s, ch); +} + +/* Macro to test a character type */ +#define TEST_CTYPE_FUNC(func, string) \ +static void test_ctype_##func(void) \ +{ \ + for (int i = 0; i < 256; i++) { \ + if (!check_int(func(i), ==, is_in(string, i))) \ + test_msg(" i: %02x", i); \ + } \ +} + +#define DIGIT "0123456789" +#define LOWER "abcdefghijklmnopqrstuvwxyz" +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" +#define ASCII \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" +#define CNTRL \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x7f" + +TEST_CTYPE_FUNC(isdigit, DIGIT) +TEST_CTYPE_FUNC(isspace, " \n\r\t") +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") +TEST_CTYPE_FUNC(isascii, ASCII) +TEST_CTYPE_FUNC(islower, LOWER) +TEST_CTYPE_FUNC(isupper, UPPER) +TEST_CTYPE_FUNC(iscntrl, CNTRL) +TEST_CTYPE_FUNC(ispunct, PUNCT) +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") + +int cmd_main(int argc, const char **argv) { + /* Run all character type tests */ + TEST(test_ctype_isspace(), "isspace() works as we expect"); + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); + TEST(test_ctype_isascii(), "isascii() works as we expect"); + TEST(test_ctype_islower(), "islower() works as we expect"); + TEST(test_ctype_isupper(), "isupper() works as we expect"); + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); + TEST(test_ctype_isprint(), "isprint() works as we expect"); + + return test_done(); +} -- 2.42.0.windows.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c 2023-12-30 0:09 ` [Outreachy][PATCH v2] " Achu Luma @ 2024-01-01 10:40 ` Achu Luma 2024-01-01 16:41 ` René Scharfe 2024-01-05 16:14 ` [Outreachy][PATCH v4] " Achu Luma 0 siblings, 2 replies; 24+ messages in thread From: Achu Luma @ 2024-01-01 10:40 UTC (permalink / raw) To: git Cc: chriscool, christian.couder, gitster, l.s.r, phillip.wood, steadmon, Achu Luma In the recent codebase update(8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework. This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h). The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()). Signed-off-by: Achu Luma <ach.lumap@gmail.com> --- Sorry for the poor description of the changes, maybe the following is better: In the revised patch we added a call to test_msg() suggested by Phillip to print the character code. This helps us pinpoint exactly the byte value that is an issue as suggested by Junio. We keep using check_int() as it allows us to still see the actual and expected return which might help us in case func() returned a different non-zero value when we were expecting "1". It is useful to have the return value printed on error in case we start returning "53" instead of "1" for "true". Makefile | 2 +- t/helper/test-ctype.c | 70 -------------------------------------- t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0070-fundamental.sh | 4 --- t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 78 insertions(+), 77 deletions(-) delete mode 100644 t/helper/test-ctype.c create mode 100644 t/unit-tests/t-ctype.c diff --git a/Makefile b/Makefile index 88ba7a3c51..a4df48ba65 100644 --- a/Makefile +++ b/Makefile @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o TEST_BUILTINS_OBJS += test-csprng.o -TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o TEST_BUILTINS_OBJS += test-dir-iterator.o @@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-basic UNIT_TEST_PROGRAMS += t-strbuf +UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c deleted file mode 100644 index e5659df40b..0000000000 --- a/t/helper/test-ctype.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "test-tool.h" - -static int rc; - -static void report_error(const char *class, int ch) -{ - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); - rc = 1; -} - -static int is_in(const char *s, int ch) -{ - /* - * We can't find NUL using strchr. Accept it as the first - * character in the spec -- there are no empty classes. - */ - if (ch == '\0') - return ch == *s; - if (*s == '\0') - s++; - return !!strchr(s, ch); -} - -#define TEST_CLASS(t,s) { \ - int i; \ - for (i = 0; i < 256; i++) { \ - if (is_in(s, i) != t(i)) \ - report_error(#t, i); \ - } \ - if (t(EOF)) \ - report_error(#t, EOF); \ -} - -#define DIGIT "0123456789" -#define LOWER "abcdefghijklmnopqrstuvwxyz" -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" -#define ASCII \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" -#define CNTRL \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x7f" - -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) -{ - TEST_CLASS(isdigit, DIGIT); - TEST_CLASS(isspace, " \n\r\t"); - TEST_CLASS(isalpha, LOWER UPPER); - TEST_CLASS(isalnum, LOWER UPPER DIGIT); - TEST_CLASS(is_glob_special, "*?[\\"); - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); - TEST_CLASS(isascii, ASCII); - TEST_CLASS(islower, LOWER); - TEST_CLASS(isupper, UPPER); - TEST_CLASS(iscntrl, CNTRL); - TEST_CLASS(ispunct, PUNCT); - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); - - return rc; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 37ba996539..33b9501c21 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = { { "config", cmd__config }, { "crontab", cmd__crontab }, { "csprng", cmd__csprng }, - { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, { "dir-iterator", cmd__dir_iterator }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 8a1a7c63da..b72f07ded9 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); int cmd__csprng(int argc, const char **argv); -int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); int cmd__dir_iterator(int argc, const char **argv); diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 487bc8d905..a4756fbab9 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions. TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh -test_expect_success 'character classes (isspace, isalpha etc.)' ' - test-tool ctype -' - test_expect_success 'mktemp to nonexistent directory prints filename' ' test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err && grep "doesnotexist/test" err diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c new file mode 100644 index 0000000000..8a215d387a --- /dev/null +++ b/t/unit-tests/t-ctype.c @@ -0,0 +1,77 @@ +#include "test-lib.h" + +static int is_in(const char *s, int ch) +{ + /* + * We can't find NUL using strchr. Accept it as the first + * character in the spec -- there are no empty classes. + */ + if (ch == '\0') + return ch == *s; + if (*s == '\0') + s++; + return !!strchr(s, ch); +} + +/* Macro to test a character type */ +#define TEST_CTYPE_FUNC(func, string) \ +static void test_ctype_##func(void) \ +{ \ + for (int i = 0; i < 256; i++) { \ + if (!check_int(func(i), ==, is_in(string, i))) \ + test_msg(" i: %02x", i); \ + } \ +} + +#define DIGIT "0123456789" +#define LOWER "abcdefghijklmnopqrstuvwxyz" +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" +#define ASCII \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" +#define CNTRL \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x7f" + +TEST_CTYPE_FUNC(isdigit, DIGIT) +TEST_CTYPE_FUNC(isspace, " \n\r\t") +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") +TEST_CTYPE_FUNC(isascii, ASCII) +TEST_CTYPE_FUNC(islower, LOWER) +TEST_CTYPE_FUNC(isupper, UPPER) +TEST_CTYPE_FUNC(iscntrl, CNTRL) +TEST_CTYPE_FUNC(ispunct, PUNCT) +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") + +int cmd_main(int argc, const char **argv) { + /* Run all character type tests */ + TEST(test_ctype_isspace(), "isspace() works as we expect"); + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); + TEST(test_ctype_isascii(), "isascii() works as we expect"); + TEST(test_ctype_islower(), "islower() works as we expect"); + TEST(test_ctype_isupper(), "isupper() works as we expect"); + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); + TEST(test_ctype_isprint(), "isprint() works as we expect"); + + return test_done(); +} -- 2.42.0.windows.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-01 10:40 ` [Outreachy][PATCH v3] " Achu Luma @ 2024-01-01 16:41 ` René Scharfe 2024-01-02 16:35 ` Junio C Hamano 2024-01-05 16:14 ` [Outreachy][PATCH v4] " Achu Luma 1 sibling, 1 reply; 24+ messages in thread From: René Scharfe @ 2024-01-01 16:41 UTC (permalink / raw) To: Achu Luma, git Cc: chriscool, christian.couder, gitster, phillip.wood, steadmon Am 01.01.24 um 11:40 schrieb Achu Luma: > In the recent codebase update(8bf6fbd00d (Merge branch > 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was > merged, providing a standardized approach for testing C code. Prior to > this update, some unit tests relied on the test helper mechanism, > lacking a dedicated unit testing framework. It's more natural to perform > these unit tests using the new unit test framework. > > This commit migrates the unit tests for C character classification > functions (isdigit(), isspace(), etc) from the legacy approach > using the test-tool command `test-tool ctype` in t/helper/test-ctype.c > to the new unit testing framework (t/unit-tests/test-lib.h). > > The migration involves refactoring the tests to utilize the testing > macros provided by the framework (TEST() and check_*()). > > Signed-off-by: Achu Luma <ach.lumap@gmail.com> v1 and v2 had "Mentored-by: Christian Couder <chriscool@tuxfamily.org>". It's gone now; intentionally? > --- > Sorry for the poor description of the changes, maybe the following is better: > In the revised patch we added a call to test_msg() suggested by Phillip to > print the character code. This helps us pinpoint exactly the byte value that > is an issue as suggested by Junio. We keep using check_int() as it allows us > to still see the actual and expected return which might help us in case func() > returned a different non-zero value when we were expecting "1". It is useful > to have the return value printed on error in case we start returning "53" > instead of "1" for "true". To illustrate, here are the changes between v1 and v2 in diff form: --- >8 --- diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c index 41189ba9f9..8a215d387a 100644 --- a/t/unit-tests/t-ctype.c +++ b/t/unit-tests/t-ctype.c @@ -14,12 +14,13 @@ static int is_in(const char *s, int ch) } /* Macro to test a character type */ -#define TEST_CTYPE_FUNC(func, string) \ +#define TEST_CTYPE_FUNC(func, string) \ static void test_ctype_##func(void) \ { \ - int i; \ - for (i = 0; i < 256; i++) \ - check_int(func(i), ==, is_in(string, i)); \ + for (int i = 0; i < 256; i++) { \ + if (!check_int(func(i), ==, is_in(string, i))) \ + test_msg(" i: %02x", i); \ + } \ } #define DIGIT "0123456789" --- >8 --- v3 is the same as v2. > > Makefile | 2 +- > t/helper/test-ctype.c | 70 -------------------------------------- > t/helper/test-tool.c | 1 - > t/helper/test-tool.h | 1 - > t/t0070-fundamental.sh | 4 --- > t/unit-tests/t-ctype.c | 77 ++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 78 insertions(+), 77 deletions(-) > delete mode 100644 t/helper/test-ctype.c > create mode 100644 t/unit-tests/t-ctype.c > > diff --git a/Makefile b/Makefile > index 88ba7a3c51..a4df48ba65 100644 > --- a/Makefile > +++ b/Makefile > @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o > TEST_BUILTINS_OBJS += test-config.o > TEST_BUILTINS_OBJS += test-crontab.o > TEST_BUILTINS_OBJS += test-csprng.o > -TEST_BUILTINS_OBJS += test-ctype.o > TEST_BUILTINS_OBJS += test-date.o > TEST_BUILTINS_OBJS += test-delta.o > TEST_BUILTINS_OBJS += test-dir-iterator.o > @@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/% > > UNIT_TEST_PROGRAMS += t-basic > UNIT_TEST_PROGRAMS += t-strbuf > +UNIT_TEST_PROGRAMS += t-ctype > UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) > UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) > UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c > deleted file mode 100644 > index e5659df40b..0000000000 > --- a/t/helper/test-ctype.c > +++ /dev/null > @@ -1,70 +0,0 @@ > -#include "test-tool.h" > - > -static int rc; > - > -static void report_error(const char *class, int ch) > -{ > - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); > - rc = 1; > -} > - > -static int is_in(const char *s, int ch) > -{ > - /* > - * We can't find NUL using strchr. Accept it as the first > - * character in the spec -- there are no empty classes. > - */ > - if (ch == '\0') > - return ch == *s; > - if (*s == '\0') > - s++; > - return !!strchr(s, ch); > -} > - > -#define TEST_CLASS(t,s) { \ > - int i; \ > - for (i = 0; i < 256; i++) { \ > - if (is_in(s, i) != t(i)) \ > - report_error(#t, i); \ > - } \ > - if (t(EOF)) \ > - report_error(#t, EOF); \ > -} > - > -#define DIGIT "0123456789" > -#define LOWER "abcdefghijklmnopqrstuvwxyz" > -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" > -#define ASCII \ > - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ > - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ > - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ > - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ > - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ > - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" > -#define CNTRL \ > - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > - "\x7f" > - > -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) > -{ > - TEST_CLASS(isdigit, DIGIT); > - TEST_CLASS(isspace, " \n\r\t"); > - TEST_CLASS(isalpha, LOWER UPPER); > - TEST_CLASS(isalnum, LOWER UPPER DIGIT); > - TEST_CLASS(is_glob_special, "*?[\\"); > - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); > - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); > - TEST_CLASS(isascii, ASCII); > - TEST_CLASS(islower, LOWER); > - TEST_CLASS(isupper, UPPER); > - TEST_CLASS(iscntrl, CNTRL); > - TEST_CLASS(ispunct, PUNCT); > - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); > - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); > - > - return rc; > -} > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index 37ba996539..33b9501c21 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = { > { "config", cmd__config }, > { "crontab", cmd__crontab }, > { "csprng", cmd__csprng }, > - { "ctype", cmd__ctype }, > { "date", cmd__date }, > { "delta", cmd__delta }, > { "dir-iterator", cmd__dir_iterator }, > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h > index 8a1a7c63da..b72f07ded9 100644 > --- a/t/helper/test-tool.h > +++ b/t/helper/test-tool.h > @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv); > int cmd__config(int argc, const char **argv); > int cmd__crontab(int argc, const char **argv); > int cmd__csprng(int argc, const char **argv); > -int cmd__ctype(int argc, const char **argv); > int cmd__date(int argc, const char **argv); > int cmd__delta(int argc, const char **argv); > int cmd__dir_iterator(int argc, const char **argv); > diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh > index 487bc8d905..a4756fbab9 100755 > --- a/t/t0070-fundamental.sh > +++ b/t/t0070-fundamental.sh > @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions. > TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > -test_expect_success 'character classes (isspace, isalpha etc.)' ' > - test-tool ctype > -' > - > test_expect_success 'mktemp to nonexistent directory prints filename' ' > test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err && > grep "doesnotexist/test" err > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c > new file mode 100644 > index 0000000000..8a215d387a > --- /dev/null > +++ b/t/unit-tests/t-ctype.c > @@ -0,0 +1,77 @@ > +#include "test-lib.h" > + > +static int is_in(const char *s, int ch) > +{ > + /* > + * We can't find NUL using strchr. Accept it as the first > + * character in the spec -- there are no empty classes. > + */ > + if (ch == '\0') > + return ch == *s; > + if (*s == '\0') > + s++; > + return !!strchr(s, ch); > +} > + > +/* Macro to test a character type */ > +#define TEST_CTYPE_FUNC(func, string) \ > +static void test_ctype_##func(void) \ > +{ \ > + for (int i = 0; i < 256; i++) { \ > + if (!check_int(func(i), ==, is_in(string, i))) \ > + test_msg(" i: %02x", i); \ > + } \ This loop is indented with spaces followed by tabs. The Git project prefers indenting by tabs and in some cases tabs followed by spaces, but not the other way array. git am warns about such whitespace errors and can actually fix them automatically, so I imagine this wouldn't be a deal breaker. But even if it seems picky, respecting the project's preferences from the start reduces unnecessary friction. The original test reported the number of a misclassified character (basically its ASCII code) in both decimal and hexadecimal form. This code prints it only in hexadecimal, but without the prefix "0x". A casual reader could mistake hexadecimal numbers for decimal ones as a result. Printing only one suffices, but I think it's better to either use decimal notation without any prefix or hexadecimal with the "0x" prefix to avoid confusion. There's no reason to be stingy with the screen space here. > +} > + > +#define DIGIT "0123456789" > +#define LOWER "abcdefghijklmnopqrstuvwxyz" > +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" > +#define ASCII \ > + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ > + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ > + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ > + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ > + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ > + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" > +#define CNTRL \ > + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > + "\x7f" > + > +TEST_CTYPE_FUNC(isdigit, DIGIT) > +TEST_CTYPE_FUNC(isspace, " \n\r\t") > +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) > +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) > +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") > +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") > +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") > +TEST_CTYPE_FUNC(isascii, ASCII) > +TEST_CTYPE_FUNC(islower, LOWER) > +TEST_CTYPE_FUNC(isupper, UPPER) > +TEST_CTYPE_FUNC(iscntrl, CNTRL) > +TEST_CTYPE_FUNC(ispunct, PUNCT) > +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") > +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") > + > +int cmd_main(int argc, const char **argv) { > + /* Run all character type tests */ > + TEST(test_ctype_isspace(), "isspace() works as we expect"); > + TEST(test_ctype_isdigit(), "isdigit() works as we expect"); > + TEST(test_ctype_isalpha(), "isalpha() works as we expect"); > + TEST(test_ctype_isalnum(), "isalnum() works as we expect"); > + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); > + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); > + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); > + TEST(test_ctype_isascii(), "isascii() works as we expect"); > + TEST(test_ctype_islower(), "islower() works as we expect"); > + TEST(test_ctype_isupper(), "isupper() works as we expect"); > + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); > + TEST(test_ctype_ispunct(), "ispunct() works as we expect"); > + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); > + TEST(test_ctype_isprint(), "isprint() works as we expect"); Each class (e.g. space or digit) is mentioned thrice here: When declaring its function with TEST_CTYPE_FUNC, when calling said function and again in the test description. Adding a new class requires adding two lines of code. That's not too bad, but the original implementation didn't require that repetition and adding a new class only required adding a single line. I mentioned this briefly in my review of v1 in https://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/ and showed a way to define character classes without repeating their names. You don't have to follow that suggestion, but it would be nice if you could give some feedback about it. > + > + return test_done(); > +} René ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-01 16:41 ` René Scharfe @ 2024-01-02 16:35 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2024-01-02 16:35 UTC (permalink / raw) To: René Scharfe Cc: Achu Luma, git, chriscool, christian.couder, phillip.wood, steadmon René Scharfe <l.s.r@web.de> writes: >> ... >> + for (int i = 0; i < 256; i++) { \ >> + if (!check_int(func(i), ==, is_in(string, i))) \ >> + test_msg(" i: %02x", i); \ >> + } \ > > This loop is indented with spaces followed by tabs. The Git project > prefers indenting by tabs and in some cases tabs followed by spaces, but > not the other way array. git am warns about such whitespace errors and > can actually fix them automatically, so I imagine this wouldn't be a > deal breaker. But even if it seems picky, respecting the project's > preferences from the start reduces unnecessary friction. > > The original test reported the number of a misclassified character > (basically its ASCII code) in both decimal and hexadecimal form. This > code prints it only in hexadecimal, but without the prefix "0x". A > casual reader could mistake hexadecimal numbers for decimal ones as a > result. Printing only one suffices, but I think it's better to either > use decimal notation without any prefix or hexadecimal with the "0x" > prefix to avoid confusion. There's no reason to be stingy with the > screen space here. > ... > Each class (e.g. space or digit) is mentioned thrice here: When > declaring its function with TEST_CTYPE_FUNC, when calling said function > and again in the test description. Adding a new class requires adding > two lines of code. That's not too bad, but the original implementation > didn't require that repetition and adding a new class only required > adding a single line. Thanks for an excellent review. > > I mentioned this briefly in my review of v1 in > https://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/ > and showed a way to define character classes without repeating their > names. You don't have to follow that suggestion, but it would be nice > if you could give some feedback about it. > >> + >> + return test_done(); >> +} > > René ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-01 10:40 ` [Outreachy][PATCH v3] " Achu Luma 2024-01-01 16:41 ` René Scharfe @ 2024-01-05 16:14 ` Achu Luma 2024-01-07 12:45 ` René Scharfe ` (2 more replies) 1 sibling, 3 replies; 24+ messages in thread From: Achu Luma @ 2024-01-05 16:14 UTC (permalink / raw) To: git Cc: gitster, chriscool, christian.couder, l.s.r, phillip.wood, steadmon, me, Achu Luma, Phillip Wood In the recent codebase update (8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework. This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h). The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()). Mentored-by: Christian Couder <chriscool@tuxfamily.org> Helped-by: René Scharfe <l.s.r@web.de> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Achu Luma <ach.lumap@gmail.com> --- The changes between version 3 and version 4 are the following: - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro. - A "0x"prefix has been added to avoid confusion between decimal and hexadecimal codes printed by test_msg(). - The "Mentored-by:..." trailer has been restored. - "works as expected" has been reduced to just "works" as suggested by Taylor. - Some "Helped-by: ..." trailers have been added. - Some whitespace fixes have been made. Thanks to Junio, René, Phillip and Taylor for commenting on previous versions of this patch. Here is a diff between v3 and v4: @@ -14,15 +14,16 @@ static int is_in(const char *s, int ch) } /* Macro to test a character type */ -#define TEST_CTYPE_FUNC(func, string) \ -static void test_ctype_##func(void) \ -{ \ - for (int i = 0; i < 256; i++) { \ - if (!check_int(func(i), ==, is_in(string, i))) \ - test_msg(" i: %02x", i); \ - } \ +#define TEST_CTYPE_FUNC(func, string) \ +static void test_ctype_##func(void) { \ + for (int i = 0; i < 256; i++) { \ + if (!check_int(func(i), ==, is_in(string, i))) \ + test_msg(" i: 0x%02x", i); \ + } \ } +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works") + #define DIGIT "0123456789" #define LOWER "abcdefghijklmnopqrstuvwxyz" #define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" @@ -58,20 +59,20 @@ TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") int cmd_main(int argc, const char **argv) { /* Run all character type tests */ - TEST(test_ctype_isspace(), "isspace() works as we expect"); - TEST(test_ctype_isdigit(), "isdigit() works as we expect"); - TEST(test_ctype_isalpha(), "isalpha() works as we expect"); - TEST(test_ctype_isalnum(), "isalnum() works as we expect"); - TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect"); - TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect"); - TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect"); - TEST(test_ctype_isascii(), "isascii() works as we expect"); - TEST(test_ctype_islower(), "islower() works as we expect"); - TEST(test_ctype_isupper(), "isupper() works as we expect"); - TEST(test_ctype_iscntrl(), "iscntrl() works as we expect"); - TEST(test_ctype_ispunct(), "ispunct() works as we expect"); - TEST(test_ctype_isxdigit(), "isxdigit() works as we expect"); - TEST(test_ctype_isprint(), "isprint() works as we expect"); + TEST_CHAR_CLASS(isspace); + TEST_CHAR_CLASS(isdigit); + TEST_CHAR_CLASS(isalpha); + TEST_CHAR_CLASS(isalnum); + TEST_CHAR_CLASS(is_glob_special); + TEST_CHAR_CLASS(is_regex_special); + TEST_CHAR_CLASS(is_pathspec_magic); + TEST_CHAR_CLASS(isascii); + TEST_CHAR_CLASS(islower); + TEST_CHAR_CLASS(isupper); + TEST_CHAR_CLASS(iscntrl); + TEST_CHAR_CLASS(ispunct); + TEST_CHAR_CLASS(isxdigit); + TEST_CHAR_CLASS(isprint); Makefile | 2 +- t/helper/test-ctype.c | 70 ------------------------------------- t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0070-fundamental.sh | 4 --- t/unit-tests/t-ctype.c | 78 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 79 insertions(+), 77 deletions(-) delete mode 100644 t/helper/test-ctype.c create mode 100644 t/unit-tests/t-ctype.c diff --git a/Makefile b/Makefile index 88ba7a3c51..a4df48ba65 100644 --- a/Makefile +++ b/Makefile @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o TEST_BUILTINS_OBJS += test-csprng.o -TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o TEST_BUILTINS_OBJS += test-dir-iterator.o @@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-basic UNIT_TEST_PROGRAMS += t-strbuf +UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c deleted file mode 100644 index e5659df40b..0000000000 --- a/t/helper/test-ctype.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "test-tool.h" - -static int rc; - -static void report_error(const char *class, int ch) -{ - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); - rc = 1; -} - -static int is_in(const char *s, int ch) -{ - /* - * We can't find NUL using strchr. Accept it as the first - * character in the spec -- there are no empty classes. - */ - if (ch == '\0') - return ch == *s; - if (*s == '\0') - s++; - return !!strchr(s, ch); -} - -#define TEST_CLASS(t,s) { \ - int i; \ - for (i = 0; i < 256; i++) { \ - if (is_in(s, i) != t(i)) \ - report_error(#t, i); \ - } \ - if (t(EOF)) \ - report_error(#t, EOF); \ -} - -#define DIGIT "0123456789" -#define LOWER "abcdefghijklmnopqrstuvwxyz" -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" -#define ASCII \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" -#define CNTRL \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x7f" - -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) -{ - TEST_CLASS(isdigit, DIGIT); - TEST_CLASS(isspace, " \n\r\t"); - TEST_CLASS(isalpha, LOWER UPPER); - TEST_CLASS(isalnum, LOWER UPPER DIGIT); - TEST_CLASS(is_glob_special, "*?[\\"); - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); - TEST_CLASS(isascii, ASCII); - TEST_CLASS(islower, LOWER); - TEST_CLASS(isupper, UPPER); - TEST_CLASS(iscntrl, CNTRL); - TEST_CLASS(ispunct, PUNCT); - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); - - return rc; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 37ba996539..33b9501c21 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = { { "config", cmd__config }, { "crontab", cmd__crontab }, { "csprng", cmd__csprng }, - { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, { "dir-iterator", cmd__dir_iterator }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 8a1a7c63da..b72f07ded9 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); int cmd__csprng(int argc, const char **argv); -int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); int cmd__dir_iterator(int argc, const char **argv); diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 487bc8d905..a4756fbab9 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions. TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh -test_expect_success 'character classes (isspace, isalpha etc.)' ' - test-tool ctype -' - test_expect_success 'mktemp to nonexistent directory prints filename' ' test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err && grep "doesnotexist/test" err diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c new file mode 100644 index 0000000000..3a338df541 --- /dev/null +++ b/t/unit-tests/t-ctype.c @@ -0,0 +1,78 @@ +#include "test-lib.h" + +static int is_in(const char *s, int ch) +{ + /* + * We can't find NUL using strchr. Accept it as the first + * character in the spec -- there are no empty classes. + */ + if (ch == '\0') + return ch == *s; + if (*s == '\0') + s++; + return !!strchr(s, ch); +} + +/* Macro to test a character type */ +#define TEST_CTYPE_FUNC(func, string) \ +static void test_ctype_##func(void) { \ + for (int i = 0; i < 256; i++) { \ + if (!check_int(func(i), ==, is_in(string, i))) \ + test_msg(" i: 0x%02x", i); \ + } \ +} + +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works") + +#define DIGIT "0123456789" +#define LOWER "abcdefghijklmnopqrstuvwxyz" +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" +#define ASCII \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" +#define CNTRL \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x7f" + +TEST_CTYPE_FUNC(isdigit, DIGIT) +TEST_CTYPE_FUNC(isspace, " \n\r\t") +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") +TEST_CTYPE_FUNC(isascii, ASCII) +TEST_CTYPE_FUNC(islower, LOWER) +TEST_CTYPE_FUNC(isupper, UPPER) +TEST_CTYPE_FUNC(iscntrl, CNTRL) +TEST_CTYPE_FUNC(ispunct, PUNCT) +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") + +int cmd_main(int argc, const char **argv) { + /* Run all character type tests */ + TEST_CHAR_CLASS(isspace); + TEST_CHAR_CLASS(isdigit); + TEST_CHAR_CLASS(isalpha); + TEST_CHAR_CLASS(isalnum); + TEST_CHAR_CLASS(is_glob_special); + TEST_CHAR_CLASS(is_regex_special); + TEST_CHAR_CLASS(is_pathspec_magic); + TEST_CHAR_CLASS(isascii); + TEST_CHAR_CLASS(islower); + TEST_CHAR_CLASS(isupper); + TEST_CHAR_CLASS(iscntrl); + TEST_CHAR_CLASS(ispunct); + TEST_CHAR_CLASS(isxdigit); + TEST_CHAR_CLASS(isprint); + + return test_done(); +} -- 2.42.0.windows.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-05 16:14 ` [Outreachy][PATCH v4] " Achu Luma @ 2024-01-07 12:45 ` René Scharfe 2024-01-08 22:32 ` Junio C Hamano 2024-01-09 10:35 ` Phillip Wood 2024-01-12 10:27 ` [Outreachy][PATCH v5] " Achu Luma 2 siblings, 1 reply; 24+ messages in thread From: René Scharfe @ 2024-01-07 12:45 UTC (permalink / raw) To: Achu Luma, git Cc: gitster, chriscool, christian.couder, phillip.wood, steadmon, me, Phillip Wood Am 05.01.24 um 17:14 schrieb Achu Luma: > In the recent codebase update (8bf6fbd00d (Merge branch > 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was > merged, providing a standardized approach for testing C code. Prior to > this update, some unit tests relied on the test helper mechanism, > lacking a dedicated unit testing framework. It's more natural to perform > these unit tests using the new unit test framework. > > This commit migrates the unit tests for C character classification > functions (isdigit(), isspace(), etc) from the legacy approach > using the test-tool command `test-tool ctype` in t/helper/test-ctype.c > to the new unit testing framework (t/unit-tests/test-lib.h). > > The migration involves refactoring the tests to utilize the testing > macros provided by the framework (TEST() and check_*()). > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Helped-by: René Scharfe <l.s.r@web.de> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Helped-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Achu Luma <ach.lumap@gmail.com> > --- [snip] > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c > deleted file mode 100644 > index e5659df40b..0000000000 > --- a/t/helper/test-ctype.c > +++ /dev/null > @@ -1,70 +0,0 @@ > -#include "test-tool.h" > - > -static int rc; > - > -static void report_error(const char *class, int ch) > -{ > - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); > - rc = 1; > -} > - > -static int is_in(const char *s, int ch) > -{ > - /* > - * We can't find NUL using strchr. Accept it as the first > - * character in the spec -- there are no empty classes. > - */ > - if (ch == '\0') > - return ch == *s; > - if (*s == '\0') > - s++; > - return !!strchr(s, ch); > -} > - > -#define TEST_CLASS(t,s) { \ > - int i; \ > - for (i = 0; i < 256; i++) { \ > - if (is_in(s, i) != t(i)) \ > - report_error(#t, i); \ > - } \ > - if (t(EOF)) \ > - report_error(#t, EOF); \ > -} > - > -#define DIGIT "0123456789" > -#define LOWER "abcdefghijklmnopqrstuvwxyz" > -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" > -#define ASCII \ > - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ > - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ > - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ > - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ > - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ > - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" > -#define CNTRL \ > - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > - "\x7f" > - > -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) > -{ > - TEST_CLASS(isdigit, DIGIT); > - TEST_CLASS(isspace, " \n\r\t"); > - TEST_CLASS(isalpha, LOWER UPPER); > - TEST_CLASS(isalnum, LOWER UPPER DIGIT); > - TEST_CLASS(is_glob_special, "*?[\\"); > - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); > - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); > - TEST_CLASS(isascii, ASCII); > - TEST_CLASS(islower, LOWER); > - TEST_CLASS(isupper, UPPER); > - TEST_CLASS(iscntrl, CNTRL); > - TEST_CLASS(ispunct, PUNCT); > - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); > - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); > - > - return rc; > -} [snip] > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c > new file mode 100644 > index 0000000000..3a338df541 > --- /dev/null > +++ b/t/unit-tests/t-ctype.c > @@ -0,0 +1,78 @@ > +#include "test-lib.h" > + > +static int is_in(const char *s, int ch) > +{ > + /* > + * We can't find NUL using strchr. Accept it as the first > + * character in the spec -- there are no empty classes. > + */ > + if (ch == '\0') > + return ch == *s; > + if (*s == '\0') > + s++; > + return !!strchr(s, ch); > +} > + > +/* Macro to test a character type */ > +#define TEST_CTYPE_FUNC(func, string) \ > +static void test_ctype_##func(void) { \ > + for (int i = 0; i < 256; i++) { \ > + if (!check_int(func(i), ==, is_in(string, i))) \ > + test_msg(" i: 0x%02x", i); \ > + } \ > +} > + > +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works") > + > +#define DIGIT "0123456789" > +#define LOWER "abcdefghijklmnopqrstuvwxyz" > +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" > +#define ASCII \ > + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ > + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ > + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ > + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ > + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ > + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" > +#define CNTRL \ > + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > + "\x7f" > + > +TEST_CTYPE_FUNC(isdigit, DIGIT) > +TEST_CTYPE_FUNC(isspace, " \n\r\t") > +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) > +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) > +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") > +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") > +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") > +TEST_CTYPE_FUNC(isascii, ASCII) > +TEST_CTYPE_FUNC(islower, LOWER) > +TEST_CTYPE_FUNC(isupper, UPPER) > +TEST_CTYPE_FUNC(iscntrl, CNTRL) > +TEST_CTYPE_FUNC(ispunct, PUNCT) > +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") > +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") > + > +int cmd_main(int argc, const char **argv) { > + /* Run all character type tests */ > + TEST_CHAR_CLASS(isspace); > + TEST_CHAR_CLASS(isdigit); > + TEST_CHAR_CLASS(isalpha); > + TEST_CHAR_CLASS(isalnum); > + TEST_CHAR_CLASS(is_glob_special); > + TEST_CHAR_CLASS(is_regex_special); > + TEST_CHAR_CLASS(is_pathspec_magic); > + TEST_CHAR_CLASS(isascii); > + TEST_CHAR_CLASS(islower); > + TEST_CHAR_CLASS(isupper); > + TEST_CHAR_CLASS(iscntrl); > + TEST_CHAR_CLASS(ispunct); > + TEST_CHAR_CLASS(isxdigit); > + TEST_CHAR_CLASS(isprint); > + > + return test_done(); > +} > -- > 2.42.0.windows.2 > Quite an improvement over v3! Now you only need to repeat the class names once. Can we do any better? We could simply have one test per character per class like this: #define TEST_CHAR_CLASS(class, expect) \ for (int i = 0; i < 256; i++) \ TEST(check_int(class(i), ==, is_in(expect, i)), \ "%s(0x%02x) works", #class, i) Which would be used like this: TEST_CHAR_CLASS(isspace, " \n\r\t"); With that there is no need to define any functions anymore. We also don't need any custom output, as the test name includes the character code. Downside: We'd have thousands of tests. But is that actually a downside or is that how the unit test framework is supposed to be used? If we need to aggregate the results by class for some reason, we could use strings, like we already do for defining the expected class members. We need special handling for NUL, as that character terminates C strings, but we can put all other characters into a string and then use check_str: #define TEST_CHAR_CLASS(class, expect) \ do { \ int expect_nul = expect[0] == '\0'; \ char expect_rest[256] = {0}; \ char actual_rest[256] = {0}; \ for (int i = 1, j = 0; i < 256; i++) \ if (strchr(&expect[expect_nul], i)) \ expect_rest[j++] = i; \ for (int i = 1, j = 0; i < 256; i++) \ if (class(i) == 1) \ actual_rest[j++] = i; \ TEST(check_int(class(0), ==, expect_nul) && \ check_str(actual_rest, expect_rest), \ #class " works"); \ } while (0) check_str escapes non-printable characters when reporting a mismatch, so this shouldn't mess up your terminal. By the way: Like the original code these checks are stricter than required by the C standard in requiring the result to be 1 instead of just true (any non-zero value). Perhaps they should be relaxed. But that's a tangent and independent of the convergence to a unit test. René ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-07 12:45 ` René Scharfe @ 2024-01-08 22:32 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2024-01-08 22:32 UTC (permalink / raw) To: René Scharfe Cc: Achu Luma, git, chriscool, christian.couder, phillip.wood, steadmon, me, Phillip Wood René Scharfe <l.s.r@web.de> writes: > Quite an improvement over v3! Now you only need to repeat the class > names once. > > Can we do any better? ;-) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-05 16:14 ` [Outreachy][PATCH v4] " Achu Luma 2024-01-07 12:45 ` René Scharfe @ 2024-01-09 10:35 ` Phillip Wood 2024-01-09 17:12 ` Junio C Hamano 2024-01-12 10:27 ` [Outreachy][PATCH v5] " Achu Luma 2 siblings, 1 reply; 24+ messages in thread From: Phillip Wood @ 2024-01-09 10:35 UTC (permalink / raw) To: Achu Luma, git Cc: gitster, chriscool, christian.couder, l.s.r, phillip.wood, steadmon, me Hi Achu On 05/01/2024 16:14, Achu Luma wrote: > In the recent codebase update (8bf6fbd00d (Merge branch > 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was > merged, providing a standardized approach for testing C code. Prior to > this update, some unit tests relied on the test helper mechanism, > lacking a dedicated unit testing framework. It's more natural to perform > these unit tests using the new unit test framework. > > This commit migrates the unit tests for C character classification > functions (isdigit(), isspace(), etc) from the legacy approach > using the test-tool command `test-tool ctype` in t/helper/test-ctype.c > to the new unit testing framework (t/unit-tests/test-lib.h). > > The migration involves refactoring the tests to utilize the testing > macros provided by the framework (TEST() and check_*()). > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Helped-by: René Scharfe <l.s.r@web.de> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Helped-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Achu Luma <ach.lumap@gmail.com> > --- > The changes between version 3 and version 4 are the following: > > - Some duplication has been reduced using a new TEST_CHAR_CLASS() macro. > - A "0x"prefix has been added to avoid confusion between decimal and > hexadecimal codes printed by test_msg(). > - The "Mentored-by:..." trailer has been restored. > - "works as expected" has been reduced to just "works" as suggested by Taylor. > - Some "Helped-by: ..." trailers have been added. > - Some whitespace fixes have been made. Thanks for the re-roll, the changes look good, there is one issue that I spotted below > -#define TEST_CLASS(t,s) { \ > - int i; \ > - for (i = 0; i < 256; i++) { \ > - if (is_in(s, i) != t(i)) \ > - report_error(#t, i); \ > - } \ > - if (t(EOF)) \ > - report_error(#t, EOF); \ > -} In the old version we test EOF in addition to each character between 0-255 > +/* Macro to test a character type */ > +#define TEST_CTYPE_FUNC(func, string) \ > +static void test_ctype_##func(void) { \ > + for (int i = 0; i < 256; i++) { \ > + if (!check_int(func(i), ==, is_in(string, i))) \ > + test_msg(" i: 0x%02x", i); \ > + } \ > +} In the new version we only test the characters 0-255, not EOF. If there is a good reason for removing the EOF tests then it should be explained in the commit message. If not it would be good to add those tests back. Best Wishes Phillip ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-09 10:35 ` Phillip Wood @ 2024-01-09 17:12 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2024-01-09 17:12 UTC (permalink / raw) To: Phillip Wood Cc: Achu Luma, git, chriscool, christian.couder, l.s.r, phillip.wood, steadmon, me Phillip Wood <phillip.wood123@gmail.com> writes: > In the new version we only test the characters 0-255, not EOF. If > there is a good reason for removing the EOF tests then it should be > explained in the commit message. If not it would be good to add those > tests back. Thanks for sharp eyes. Didn't notice the handling of EOF myself. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-05 16:14 ` [Outreachy][PATCH v4] " Achu Luma 2024-01-07 12:45 ` René Scharfe 2024-01-09 10:35 ` Phillip Wood @ 2024-01-12 10:27 ` Achu Luma 2024-01-15 10:39 ` Phillip Wood 2 siblings, 1 reply; 24+ messages in thread From: Achu Luma @ 2024-01-12 10:27 UTC (permalink / raw) To: git Cc: chriscool, christian.couder, gitster, l.s.r, me, phillip.wood123, phillip.wood, steadmon, Achu Luma In the recent codebase update (8bf6fbd00d (Merge branch 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was merged, providing a standardized approach for testing C code. Prior to this update, some unit tests relied on the test helper mechanism, lacking a dedicated unit testing framework. It's more natural to perform these unit tests using the new unit test framework. This commit migrates the unit tests for C character classification functions (isdigit(), isspace(), etc) from the legacy approach using the test-tool command `test-tool ctype` in t/helper/test-ctype.c to the new unit testing framework (t/unit-tests/test-lib.h). The migration involves refactoring the tests to utilize the testing macros provided by the framework (TEST() and check_*()). Mentored-by: Christian Couder <chriscool@tuxfamily.org> Helped-by: René Scharfe <l.s.r@web.de> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Achu Luma <ach.lumap@gmail.com> --- The change between version 4 and version 5 is: - Added tests to handle EOF. Thanks to Phillip for noticing the missing tests.. Here is a diff between v4 and v5: + if (!check(!func(EOF))) \ + test_msg(" i: 0x%02x (EOF)", EOF); \ Thanks also to René, Phillip, Junio and Taylor who helped with previous versions. Makefile | 2 +- t/helper/test-ctype.c | 70 ------------------------------------ t/helper/test-tool.c | 1 - t/helper/test-tool.h | 1 - t/t0070-fundamental.sh | 4 --- t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 81 insertions(+), 77 deletions(-) delete mode 100644 t/helper/test-ctype.c create mode 100644 t/unit-tests/t-ctype.c diff --git a/Makefile b/Makefile index 15990ff312..1a62e48759 100644 --- a/Makefile +++ b/Makefile @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o TEST_BUILTINS_OBJS += test-csprng.o -TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o TEST_BUILTINS_OBJS += test-dir-iterator.o @@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-basic UNIT_TEST_PROGRAMS += t-mem-pool UNIT_TEST_PROGRAMS += t-strbuf +UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c deleted file mode 100644 index e5659df40b..0000000000 --- a/t/helper/test-ctype.c +++ /dev/null @@ -1,70 +0,0 @@ -#include "test-tool.h" - -static int rc; - -static void report_error(const char *class, int ch) -{ - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); - rc = 1; -} - -static int is_in(const char *s, int ch) -{ - /* - * We can't find NUL using strchr. Accept it as the first - * character in the spec -- there are no empty classes. - */ - if (ch == '\0') - return ch == *s; - if (*s == '\0') - s++; - return !!strchr(s, ch); -} - -#define TEST_CLASS(t,s) { \ - int i; \ - for (i = 0; i < 256; i++) { \ - if (is_in(s, i) != t(i)) \ - report_error(#t, i); \ - } \ - if (t(EOF)) \ - report_error(#t, EOF); \ -} - -#define DIGIT "0123456789" -#define LOWER "abcdefghijklmnopqrstuvwxyz" -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" -#define ASCII \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" -#define CNTRL \ - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ - "\x7f" - -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) -{ - TEST_CLASS(isdigit, DIGIT); - TEST_CLASS(isspace, " \n\r\t"); - TEST_CLASS(isalpha, LOWER UPPER); - TEST_CLASS(isalnum, LOWER UPPER DIGIT); - TEST_CLASS(is_glob_special, "*?[\\"); - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); - TEST_CLASS(isascii, ASCII); - TEST_CLASS(islower, LOWER); - TEST_CLASS(isupper, UPPER); - TEST_CLASS(iscntrl, CNTRL); - TEST_CLASS(ispunct, PUNCT); - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); - - return rc; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 37ba996539..33b9501c21 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = { { "config", cmd__config }, { "crontab", cmd__crontab }, { "csprng", cmd__csprng }, - { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, { "dir-iterator", cmd__dir_iterator }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 8a1a7c63da..b72f07ded9 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); int cmd__csprng(int argc, const char **argv); -int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); int cmd__dir_iterator(int argc, const char **argv); diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 487bc8d905..a4756fbab9 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions. TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh -test_expect_success 'character classes (isspace, isalpha etc.)' ' - test-tool ctype -' - test_expect_success 'mktemp to nonexistent directory prints filename' ' test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err && grep "doesnotexist/test" err diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c new file mode 100644 index 0000000000..f315489984 --- /dev/null +++ b/t/unit-tests/t-ctype.c @@ -0,0 +1,80 @@ +#include "test-lib.h" + +static int is_in(const char *s, int ch) +{ + /* + * We can't find NUL using strchr. Accept it as the first + * character in the spec -- there are no empty classes. + */ + if (ch == '\0') + return ch == *s; + if (*s == '\0') + s++; + return !!strchr(s, ch); +} + +/* Macro to test a character type */ +#define TEST_CTYPE_FUNC(func, string) \ +static void test_ctype_##func(void) { \ + for (int i = 0; i < 256; i++) { \ + if (!check_int(func(i), ==, is_in(string, i))) \ + test_msg(" i: 0x%02x", i); \ + } \ + if (!check(!func(EOF))) \ + test_msg(" i: 0x%02x (EOF)", EOF); \ +} + +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works") + +#define DIGIT "0123456789" +#define LOWER "abcdefghijklmnopqrstuvwxyz" +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" +#define ASCII \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" +#define CNTRL \ + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ + "\x7f" + +TEST_CTYPE_FUNC(isdigit, DIGIT) +TEST_CTYPE_FUNC(isspace, " \n\r\t") +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") +TEST_CTYPE_FUNC(isascii, ASCII) +TEST_CTYPE_FUNC(islower, LOWER) +TEST_CTYPE_FUNC(isupper, UPPER) +TEST_CTYPE_FUNC(iscntrl, CNTRL) +TEST_CTYPE_FUNC(ispunct, PUNCT) +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") + +int cmd_main(int argc, const char **argv) { + /* Run all character type tests */ + TEST_CHAR_CLASS(isspace); + TEST_CHAR_CLASS(isdigit); + TEST_CHAR_CLASS(isalpha); + TEST_CHAR_CLASS(isalnum); + TEST_CHAR_CLASS(is_glob_special); + TEST_CHAR_CLASS(is_regex_special); + TEST_CHAR_CLASS(is_pathspec_magic); + TEST_CHAR_CLASS(isascii); + TEST_CHAR_CLASS(islower); + TEST_CHAR_CLASS(isupper); + TEST_CHAR_CLASS(iscntrl); + TEST_CHAR_CLASS(ispunct); + TEST_CHAR_CLASS(isxdigit); + TEST_CHAR_CLASS(isprint); + + return test_done(); +} -- 2.42.0.windows.2 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-12 10:27 ` [Outreachy][PATCH v5] " Achu Luma @ 2024-01-15 10:39 ` Phillip Wood 2024-01-16 15:38 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Phillip Wood @ 2024-01-15 10:39 UTC (permalink / raw) To: Achu Luma, git Cc: chriscool, christian.couder, gitster, l.s.r, me, phillip.wood, steadmon Hi Achu On 12/01/2024 10:27, Achu Luma wrote: > In the recent codebase update (8bf6fbd00d (Merge branch > 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was > merged, providing a standardized approach for testing C code. Prior to > this update, some unit tests relied on the test helper mechanism, > lacking a dedicated unit testing framework. It's more natural to perform > these unit tests using the new unit test framework. > > This commit migrates the unit tests for C character classification > functions (isdigit(), isspace(), etc) from the legacy approach > using the test-tool command `test-tool ctype` in t/helper/test-ctype.c > to the new unit testing framework (t/unit-tests/test-lib.h). > > The migration involves refactoring the tests to utilize the testing > macros provided by the framework (TEST() and check_*()). > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Helped-by: René Scharfe <l.s.r@web.de> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Helped-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Achu Luma <ach.lumap@gmail.com> > --- > The change between version 4 and version 5 is: > - Added tests to handle EOF. > > Thanks to Phillip for noticing the missing tests.. > Here is a diff between v4 and v5: > > + if (!check(!func(EOF))) \ > + test_msg(" i: 0x%02x (EOF)", EOF); \ Thanks for adding back the test for EOF, this version looks good to me. Best Wishes Phillip > Thanks also to René, Phillip, Junio and Taylor who helped with > previous versions. > > Makefile | 2 +- > t/helper/test-ctype.c | 70 ------------------------------------ > t/helper/test-tool.c | 1 - > t/helper/test-tool.h | 1 - > t/t0070-fundamental.sh | 4 --- > t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 81 insertions(+), 77 deletions(-) > delete mode 100644 t/helper/test-ctype.c > create mode 100644 t/unit-tests/t-ctype.c > > diff --git a/Makefile b/Makefile > index 15990ff312..1a62e48759 100644 > --- a/Makefile > +++ b/Makefile > @@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o > TEST_BUILTINS_OBJS += test-config.o > TEST_BUILTINS_OBJS += test-crontab.o > TEST_BUILTINS_OBJS += test-csprng.o > -TEST_BUILTINS_OBJS += test-ctype.o > TEST_BUILTINS_OBJS += test-date.o > TEST_BUILTINS_OBJS += test-delta.o > TEST_BUILTINS_OBJS += test-dir-iterator.o > @@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/% > UNIT_TEST_PROGRAMS += t-basic > UNIT_TEST_PROGRAMS += t-mem-pool > UNIT_TEST_PROGRAMS += t-strbuf > +UNIT_TEST_PROGRAMS += t-ctype > UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) > UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) > UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c > deleted file mode 100644 > index e5659df40b..0000000000 > --- a/t/helper/test-ctype.c > +++ /dev/null > @@ -1,70 +0,0 @@ > -#include "test-tool.h" > - > -static int rc; > - > -static void report_error(const char *class, int ch) > -{ > - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch); > - rc = 1; > -} > - > -static int is_in(const char *s, int ch) > -{ > - /* > - * We can't find NUL using strchr. Accept it as the first > - * character in the spec -- there are no empty classes. > - */ > - if (ch == '\0') > - return ch == *s; > - if (*s == '\0') > - s++; > - return !!strchr(s, ch); > -} > - > -#define TEST_CLASS(t,s) { \ > - int i; \ > - for (i = 0; i < 256; i++) { \ > - if (is_in(s, i) != t(i)) \ > - report_error(#t, i); \ > - } \ > - if (t(EOF)) \ > - report_error(#t, EOF); \ > -} > - > -#define DIGIT "0123456789" > -#define LOWER "abcdefghijklmnopqrstuvwxyz" > -#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > -#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" > -#define ASCII \ > - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > - "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ > - "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ > - "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ > - "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ > - "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ > - "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" > -#define CNTRL \ > - "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > - "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > - "\x7f" > - > -int cmd__ctype(int argc UNUSED, const char **argv UNUSED) > -{ > - TEST_CLASS(isdigit, DIGIT); > - TEST_CLASS(isspace, " \n\r\t"); > - TEST_CLASS(isalpha, LOWER UPPER); > - TEST_CLASS(isalnum, LOWER UPPER DIGIT); > - TEST_CLASS(is_glob_special, "*?[\\"); > - TEST_CLASS(is_regex_special, "$()*+.?[\\^{|"); > - TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); > - TEST_CLASS(isascii, ASCII); > - TEST_CLASS(islower, LOWER); > - TEST_CLASS(isupper, UPPER); > - TEST_CLASS(iscntrl, CNTRL); > - TEST_CLASS(ispunct, PUNCT); > - TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF"); > - TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); > - > - return rc; > -} > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index 37ba996539..33b9501c21 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -19,7 +19,6 @@ static struct test_cmd cmds[] = { > { "config", cmd__config }, > { "crontab", cmd__crontab }, > { "csprng", cmd__csprng }, > - { "ctype", cmd__ctype }, > { "date", cmd__date }, > { "delta", cmd__delta }, > { "dir-iterator", cmd__dir_iterator }, > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h > index 8a1a7c63da..b72f07ded9 100644 > --- a/t/helper/test-tool.h > +++ b/t/helper/test-tool.h > @@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv); > int cmd__config(int argc, const char **argv); > int cmd__crontab(int argc, const char **argv); > int cmd__csprng(int argc, const char **argv); > -int cmd__ctype(int argc, const char **argv); > int cmd__date(int argc, const char **argv); > int cmd__delta(int argc, const char **argv); > int cmd__dir_iterator(int argc, const char **argv); > diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh > index 487bc8d905..a4756fbab9 100755 > --- a/t/t0070-fundamental.sh > +++ b/t/t0070-fundamental.sh > @@ -9,10 +9,6 @@ Verify wrappers and compatibility functions. > TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > -test_expect_success 'character classes (isspace, isalpha etc.)' ' > - test-tool ctype > -' > - > test_expect_success 'mktemp to nonexistent directory prints filename' ' > test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err && > grep "doesnotexist/test" err > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c > new file mode 100644 > index 0000000000..f315489984 > --- /dev/null > +++ b/t/unit-tests/t-ctype.c > @@ -0,0 +1,80 @@ > +#include "test-lib.h" > + > +static int is_in(const char *s, int ch) > +{ > + /* > + * We can't find NUL using strchr. Accept it as the first > + * character in the spec -- there are no empty classes. > + */ > + if (ch == '\0') > + return ch == *s; > + if (*s == '\0') > + s++; > + return !!strchr(s, ch); > +} > + > +/* Macro to test a character type */ > +#define TEST_CTYPE_FUNC(func, string) \ > +static void test_ctype_##func(void) { \ > + for (int i = 0; i < 256; i++) { \ > + if (!check_int(func(i), ==, is_in(string, i))) \ > + test_msg(" i: 0x%02x", i); \ > + } \ > + if (!check(!func(EOF))) \ > + test_msg(" i: 0x%02x (EOF)", EOF); \ > +} > + > +#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works") > + > +#define DIGIT "0123456789" > +#define LOWER "abcdefghijklmnopqrstuvwxyz" > +#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ" > +#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" > +#define ASCII \ > + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > + "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \ > + "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \ > + "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \ > + "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \ > + "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \ > + "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f" > +#define CNTRL \ > + "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \ > + "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ > + "\x7f" > + > +TEST_CTYPE_FUNC(isdigit, DIGIT) > +TEST_CTYPE_FUNC(isspace, " \n\r\t") > +TEST_CTYPE_FUNC(isalpha, LOWER UPPER) > +TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT) > +TEST_CTYPE_FUNC(is_glob_special, "*?[\\") > +TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|") > +TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~") > +TEST_CTYPE_FUNC(isascii, ASCII) > +TEST_CTYPE_FUNC(islower, LOWER) > +TEST_CTYPE_FUNC(isupper, UPPER) > +TEST_CTYPE_FUNC(iscntrl, CNTRL) > +TEST_CTYPE_FUNC(ispunct, PUNCT) > +TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF") > +TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ") > + > +int cmd_main(int argc, const char **argv) { > + /* Run all character type tests */ > + TEST_CHAR_CLASS(isspace); > + TEST_CHAR_CLASS(isdigit); > + TEST_CHAR_CLASS(isalpha); > + TEST_CHAR_CLASS(isalnum); > + TEST_CHAR_CLASS(is_glob_special); > + TEST_CHAR_CLASS(is_regex_special); > + TEST_CHAR_CLASS(is_pathspec_magic); > + TEST_CHAR_CLASS(isascii); > + TEST_CHAR_CLASS(islower); > + TEST_CHAR_CLASS(isupper); > + TEST_CHAR_CLASS(iscntrl); > + TEST_CHAR_CLASS(ispunct); > + TEST_CHAR_CLASS(isxdigit); > + TEST_CHAR_CLASS(isprint); > + > + return test_done(); > +} > -- > 2.42.0.windows.2 > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-15 10:39 ` Phillip Wood @ 2024-01-16 15:38 ` Junio C Hamano 2024-01-16 19:27 ` René Scharfe 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2024-01-16 15:38 UTC (permalink / raw) To: Phillip Wood Cc: Achu Luma, git, chriscool, christian.couder, l.s.r, me, phillip.wood, steadmon Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks for adding back the test for EOF, this version looks good to me. Thanks. Let's merge it to 'next'. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-16 15:38 ` Junio C Hamano @ 2024-01-16 19:27 ` René Scharfe 2024-01-16 19:45 ` Christian Couder ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: René Scharfe @ 2024-01-16 19:27 UTC (permalink / raw) To: Junio C Hamano, Phillip Wood Cc: Achu Luma, git, chriscool, christian.couder, me, phillip.wood, steadmon Am 16.01.24 um 16:38 schrieb Junio C Hamano: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> Thanks for adding back the test for EOF, this version looks good to me. > > Thanks. Let's merge it to 'next'. OK. I'm still interested in replies to my question in https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/, i.e. whether we should have one TEST per class or one per class and character -- or in a broader sense: What's the ideal scope of a TEST? But I can ask it again in the form of a follow-up patch. René ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-16 19:27 ` René Scharfe @ 2024-01-16 19:45 ` Christian Couder 2024-01-16 19:52 ` Junio C Hamano 2024-01-17 5:37 ` Josh Steadmon 2 siblings, 0 replies; 24+ messages in thread From: Christian Couder @ 2024-01-16 19:45 UTC (permalink / raw) To: René Scharfe Cc: Junio C Hamano, Phillip Wood, Achu Luma, git, chriscool, me, phillip.wood, steadmon On Tue, Jan 16, 2024 at 8:27 PM René Scharfe <l.s.r@web.de> wrote: > > Am 16.01.24 um 16:38 schrieb Junio C Hamano: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > >> Thanks for adding back the test for EOF, this version looks good to me. > > > > Thanks. Let's merge it to 'next'. > > OK. I'm still interested in replies to my question in > https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/, > i.e. whether we should have one TEST per class or one per class and > character -- or in a broader sense: What's the ideal scope of a TEST? > But I can ask it again in the form of a follow-up patch. I think one test per character per class would result in too much detail in the output. Other than that I think it's better to address your questions to the designers of the unit test framework rather than to the authors of this patch. And yeah, sending a follow up patch would perhaps be the best. Thanks. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-16 19:27 ` René Scharfe 2024-01-16 19:45 ` Christian Couder @ 2024-01-16 19:52 ` Junio C Hamano 2024-01-17 5:37 ` Josh Steadmon 2 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2024-01-16 19:52 UTC (permalink / raw) To: René Scharfe Cc: Phillip Wood, Achu Luma, git, chriscool, christian.couder, me, phillip.wood, steadmon René Scharfe <l.s.r@web.de> writes: > Am 16.01.24 um 16:38 schrieb Junio C Hamano: >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >>> Thanks for adding back the test for EOF, this version looks good to me. >> >> Thanks. Let's merge it to 'next'. > > OK. I'm still interested in replies to my question in > https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/, > i.e. whether we should have one TEST per class or one per class and > character -- or in a broader sense: What's the ideal scope of a TEST? > But I can ask it again in the form of a follow-up patch. I personally do not have a good answer, but those who are interested in unit-tests more than I do should have their opinions to share ;-) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c 2024-01-16 19:27 ` René Scharfe 2024-01-16 19:45 ` Christian Couder 2024-01-16 19:52 ` Junio C Hamano @ 2024-01-17 5:37 ` Josh Steadmon 2 siblings, 0 replies; 24+ messages in thread From: Josh Steadmon @ 2024-01-17 5:37 UTC (permalink / raw) To: René Scharfe Cc: Junio C Hamano, Phillip Wood, Achu Luma, git, chriscool, christian.couder, me, phillip.wood On 2024.01.16 20:27, René Scharfe wrote: > Am 16.01.24 um 16:38 schrieb Junio C Hamano: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > >> Thanks for adding back the test for EOF, this version looks good to me. > > > > Thanks. Let's merge it to 'next'. > > OK. I'm still interested in replies to my question in > https://lore.kernel.org/git/a087f57c-ce72-45c7-8182-f38d0aca9030@web.de/, > i.e. whether we should have one TEST per class or one per class and > character -- or in a broader sense: What's the ideal scope of a TEST? > But I can ask it again in the form of a follow-up patch. > > René I think that the scope of a TEST() should tend small: we want the minimal amount of setup required to test the invariants that we're interested in. For this particular unit test, since we're just testing simple predicates on static sets of characters, I would be OK seeing one TEST() per class/character. That would certainly make this unit test an outlier in the number of checks, but I'm less worried about that since this is testing system-provided functions that we don't expect to change regularly. Additionally, the elimination of a level of macro indirection makes this more readable IMO. ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2024-01-17 5:37 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-21 23:15 [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c Achu Luma 2023-12-26 18:45 ` Junio C Hamano 2023-12-27 10:57 ` Christian Couder 2023-12-27 11:57 ` René Scharfe 2023-12-27 14:40 ` Phillip Wood 2023-12-27 23:48 ` Junio C Hamano 2023-12-28 16:05 ` René Scharfe 2024-01-02 18:55 ` Taylor Blau 2023-12-30 0:09 ` [Outreachy][PATCH v2] " Achu Luma 2024-01-01 10:40 ` [Outreachy][PATCH v3] " Achu Luma 2024-01-01 16:41 ` René Scharfe 2024-01-02 16:35 ` Junio C Hamano 2024-01-05 16:14 ` [Outreachy][PATCH v4] " Achu Luma 2024-01-07 12:45 ` René Scharfe 2024-01-08 22:32 ` Junio C Hamano 2024-01-09 10:35 ` Phillip Wood 2024-01-09 17:12 ` Junio C Hamano 2024-01-12 10:27 ` [Outreachy][PATCH v5] " Achu Luma 2024-01-15 10:39 ` Phillip Wood 2024-01-16 15:38 ` Junio C Hamano 2024-01-16 19:27 ` René Scharfe 2024-01-16 19:45 ` Christian Couder 2024-01-16 19:52 ` Junio C Hamano 2024-01-17 5:37 ` Josh Steadmon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).