| Message ID | 1ac2e48a7f2d41d60ff56890d8d87125f30c2f76.1724159966.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce clar testing framework | expand |
Hi Patrick On 20/08/2024 15:02, Patrick Steinhardt wrote: > Convert the ctype tests to use the new clar unit testing framework. > Introduce a new function `cl_failf()` This is a nice addition which somewhat mitigates the lack of an equivalent to test_msg() that adds addition context messages to test failures. > that allows us to print a > formatted error message, which we can use to point out which of the > characters was classified incorrectly. This results in output like this > on failure: > > # start of suite 1: ctype > ok 1 - ctype::isspace > not ok 2 - ctype::isdigit > --- > reason: | > Test failed. "Test failed." is not the reason for the test failure > 0x61 is classified incorrectly > at: > file: 't/unit-tests/ctype.c' > line: 38 > function: 'test_ctype__isdigit' > --- This is rather verbose compared to the current framework # check "isdigit(i) == !!memchr("123456789", i, len)" failed at t/unit-tests/t-ctype.c:36 # left: 1 # right: 0 # i: 0x30 not ok 2 - isdigit works The current tests also shows which characters are expected to return true and distinguishes between the two possible failure modes which are (a) misclassification and (b) returning a non-zero integer other than '1' as "true". The new test output does not allow the person running the test to distinguish between these two failure modes. > diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h > index 66ec2387cc6..4c461defe16 100644 > --- a/t/unit-tests/unit-test.h > +++ b/t/unit-tests/unit-test.h > @@ -1,3 +1,10 @@ > #include "git-compat-util.h" > #include "clar/clar.h" > #include "clar-decls.h" > +#include "strbuf.h" > + > +#define cl_failf(fmt, ...) do { \ > + char *desc = xstrfmt(fmt, __VA_ARGS__); \ In our current framework we avoid relying on the strbuf api and functions like xstrfmt() that use it as we want to be able to test strbuf.c with the framework. We'd be better with a macro wrapping a new function that uses a stack allocated buffer and p_snprintf() like clar__assert_equal(). That would allow us to upstream this change as well. > + clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \ > + free(desc); \ This is leaked on failure due to the use of longjmp. Best Wishes Phillip > +} while (0)
On Wed, Aug 28, 2024 at 02:18:02PM +0100, Phillip Wood wrote: > Hi Patrick > > On 20/08/2024 15:02, Patrick Steinhardt wrote: > > Convert the ctype tests to use the new clar unit testing framework. > > Introduce a new function `cl_failf()` > > This is a nice addition which somewhat mitigates the lack of an equivalent > to test_msg() that adds addition context messages to test failures. > > > that allows us to print a > > formatted error message, which we can use to point out which of the > > characters was classified incorrectly. This results in output like this > > on failure: > > > > # start of suite 1: ctype > > ok 1 - ctype::isspace > > not ok 2 - ctype::isdigit > > --- > > reason: | > > Test failed. > > "Test failed." is not the reason for the test failure The clar expects two strings, one "general" description and the details. I agree that it's a bit shoddy to have this as part of the reason, it really should be a separate "field". But it allows us to distinguish e.g. between test failures and tests which emitted a warning, only. > > 0x61 is classified incorrectly > > at: > > file: 't/unit-tests/ctype.c' > > line: 38 > > function: 'test_ctype__isdigit' > > --- > > This is rather verbose compared to the current framework I guess this is a matter of taste. I actually prefer the more verbose style. > # check "isdigit(i) == !!memchr("123456789", i, len)" failed at > t/unit-tests/t-ctype.c:36 > # left: 1 > # right: 0 > # i: 0x30 > not ok 2 - isdigit works > > The current tests also shows which characters are expected to return true > and distinguishes between the two possible failure modes which are (a) > misclassification and (b) returning a non-zero integer other than '1' as > "true". The new test output does not allow the person running the test to > distinguish between these two failure modes. This one is true though. Will amend. > > diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h > > index 66ec2387cc6..4c461defe16 100644 > > --- a/t/unit-tests/unit-test.h > > +++ b/t/unit-tests/unit-test.h > > @@ -1,3 +1,10 @@ > > #include "git-compat-util.h" > > #include "clar/clar.h" > > #include "clar-decls.h" > > +#include "strbuf.h" > > + > > +#define cl_failf(fmt, ...) do { \ > > + char *desc = xstrfmt(fmt, __VA_ARGS__); \ > > In our current framework we avoid relying on the strbuf api and functions > like xstrfmt() that use it as we want to be able to test strbuf.c with the > framework. We'd be better with a macro wrapping a new function that uses a > stack allocated buffer and p_snprintf() like clar__assert_equal(). That > would allow us to upstream this change as well. I don't think it's all that important to avoid a low-level API like `xstrfmt()`. But the second argument makes sense to me indeed. > > + clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \ > > + free(desc); \ > > This is leaked on failure due to the use of longjmp. As discussed elsewhere I don't think this leak matters all that much. But it's getting fixed with your proposal, so that's that. Patrick
diff --git a/Makefile b/Makefile index 56ce6c00e44..c841cf70063 100644 --- a/Makefile +++ b/Makefile @@ -1336,13 +1336,13 @@ THIRD_PARTY_SOURCES += sha1dc/% THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/% THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/% +UNIT_TESTS_SUITES += ctype UNIT_TESTS_SUITES += strvec UNIT_TESTS_PROG = $(UNIT_TEST_BIN)/unit-tests$(X) UNIT_TESTS_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TESTS_SUITES)) UNIT_TESTS_OBJS += $(UNIT_TEST_DIR)/clar/clar.o UNIT_TESTS_OBJS += $(UNIT_TEST_DIR)/unit-test.o -UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGRAMS += t-example-decorate UNIT_TEST_PROGRAMS += t-hash UNIT_TEST_PROGRAMS += t-hashmap diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/ctype.c similarity index 70% rename from t/unit-tests/t-ctype.c rename to t/unit-tests/ctype.c index e28a7f50f9a..5026378bfbc 100644 --- a/t/unit-tests/t-ctype.c +++ b/t/unit-tests/ctype.c @@ -1,16 +1,13 @@ -#include "test-lib.h" +#include "unit-test.h" #define TEST_CHAR_CLASS(class, string) do { \ size_t len = ARRAY_SIZE(string) - 1 + \ BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(string) > 0) + \ BUILD_ASSERT_OR_ZERO(sizeof(string[0]) == sizeof(char)); \ - if_test (#class " works") { \ - for (int i = 0; i < 256; i++) { \ - if (!check_int(class(i), ==, !!memchr(string, i, len)))\ - test_msg(" i: 0x%02x", i); \ - } \ - check(!class(EOF)); \ - } \ + for (int i = 0; i < 256; i++) \ + if (class(i) != !!memchr(string, i, len)) \ + cl_failf("0x%02x is classified incorrectly", i); \ + cl_assert(!class(EOF)); \ } while (0) #define DIGIT "0123456789" @@ -31,21 +28,72 @@ "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \ "\x7f" -int cmd_main(int argc, const char **argv) { +void test_ctype__isspace(void) +{ TEST_CHAR_CLASS(isspace, " \n\r\t"); +} + +void test_ctype__isdigit(void) +{ TEST_CHAR_CLASS(isdigit, DIGIT); +} + +void test_ctype__isalpha(void) +{ TEST_CHAR_CLASS(isalpha, LOWER UPPER); +} + +void test_ctype__isalnum(void) +{ TEST_CHAR_CLASS(isalnum, LOWER UPPER DIGIT); +} + +void test_ctype__is_glob_special(void) +{ TEST_CHAR_CLASS(is_glob_special, "*?[\\"); +} + +void test_ctype__is_regex_special(void) +{ TEST_CHAR_CLASS(is_regex_special, "$()*+.?[\\^{|"); +} + +void test_ctype__is_pathspec_magic(void) +{ TEST_CHAR_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~"); +} + +void test_ctype__isascii(void) +{ TEST_CHAR_CLASS(isascii, ASCII); +} + +void test_ctype__islower(void) +{ TEST_CHAR_CLASS(islower, LOWER); +} + +void test_ctype__isupper(void) +{ TEST_CHAR_CLASS(isupper, UPPER); +} + +void test_ctype__iscntrl(void) +{ TEST_CHAR_CLASS(iscntrl, CNTRL); +} + +void test_ctype__ispunct(void) +{ TEST_CHAR_CLASS(ispunct, PUNCT); +} + +void test_ctype__isxdigit(void) +{ TEST_CHAR_CLASS(isxdigit, DIGIT "abcdefABCDEF"); - TEST_CHAR_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); +} - return test_done(); +void test_ctype__isprint(void) +{ + TEST_CHAR_CLASS(isprint, LOWER UPPER DIGIT PUNCT " "); } diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h index 66ec2387cc6..4c461defe16 100644 --- a/t/unit-tests/unit-test.h +++ b/t/unit-tests/unit-test.h @@ -1,3 +1,10 @@ #include "git-compat-util.h" #include "clar/clar.h" #include "clar-decls.h" +#include "strbuf.h" + +#define cl_failf(fmt, ...) do { \ + char *desc = xstrfmt(fmt, __VA_ARGS__); \ + clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \ + free(desc); \ +} while (0)
Convert the ctype tests to use the new clar unit testing framework. Introduce a new function `cl_failf()` that allows us to print a formatted error message, which we can use to point out which of the characters was classified incorrectly. This results in output like this on failure: # start of suite 1: ctype ok 1 - ctype::isspace not ok 2 - ctype::isdigit --- reason: | Test failed. 0x61 is classified incorrectly at: file: 't/unit-tests/ctype.c' line: 38 function: 'test_ctype__isdigit' --- ok 3 - ctype::isalpha ok 4 - ctype::isalnum ok 5 - ctype::is_glob_special ok 6 - ctype::is_regex_special ok 7 - ctype::is_pathspec_magic ok 8 - ctype::isascii ok 9 - ctype::islower ok 10 - ctype::isupper ok 11 - ctype::iscntrl ok 12 - ctype::ispunct ok 13 - ctype::isxdigit ok 14 - ctype::isprint Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Makefile | 2 +- t/unit-tests/{t-ctype.c => ctype.c} | 70 ++++++++++++++++++++++++----- t/unit-tests/unit-test.h | 7 +++ 3 files changed, 67 insertions(+), 12 deletions(-) rename t/unit-tests/{t-ctype.c => ctype.c} (70%)