| Message ID | 20250131221420.38161-4-kuforiji98@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] t/unit-tests: convert hashmap test to use clar test framework | expand |
Hi Seyi On 31/01/2025 22:14, Seyi Kuforiji wrote: > Adapt strbuf test script to clar framework by using clar assertions > where necessary. This patch looks correct but it looses the nice messages we get from check_char() and check_uint(a, </<=/>/>=, b). I think it would be worth adding equivalent assertions to clar to avoid that. > /* wrapper that supplies tests with an empty, initialized strbuf */ > @@ -9,8 +9,8 @@ static void setup(void (*f)(struct strbuf*, const void*), > > f(&buf, data); > strbuf_release(&buf); > - check_uint(buf.len, ==, 0); > - check_uint(buf.alloc, ==, 0); > + cl_assert_equal_i(buf.len, 0); > + cl_assert_equal_i(buf.alloc, 0); It is a shame that we do not have an assertion for checking two unsigned integers are equal. In this case it probably does not matter but large unsigned values will be printed as negative numbers which could be rather confusing for someone trying to debug a test failure. > -static int assert_sane_strbuf(struct strbuf *buf) > +static void assert_sane_strbuf(struct strbuf *buf) > { > /* Initialized strbufs should always have a non-NULL buffer */ > - if (!check(!!buf->buf)) > - return 0; > + cl_assert(buf->buf != NULL); > /* Buffers should always be NUL-terminated */ > - if (!check_char(buf->buf[buf->len], ==, '\0')) > - return 0; > + cl_assert(buf->buf[buf->len] == '\0'); It is unfortunate that this looses the helpful diagnostic message that shows the values of the two chars being compared that is printed by check_char() when the check fails. I think it would be worth porting check_char() arcoss to clar. When we decided to move to the new test framework we planned to add new assertions to clar to match our existing framework. > /* > - * Freshly-initialized strbufs may not have a dynamically allocated > - * buffer > - */ > - if (buf->len == 0 && buf->alloc == 0) > - return 1; > - /* alloc must be at least one byte larger than len */ > - return check_uint(buf->len, <, buf->alloc); > + * In case the buffer contains anything, `alloc` must alloc must > + * be at least one byte larger than `len`. > + */ > + if (buf->len) > + cl_assert(buf->len < buf->alloc); This is another case where we loose a helpful diagnostic message because clar lacks cl_assert_lt_u(a, b) to check that unsigned integer a is less than unsigned integer b. I think it would be worth adding the assertions that are missing show that we have the equivalent of check_int(a, <, b) check_int(a, <=, b) check_int(a, >, b) check_int(a, >=, b) and their unsigned companions so we do not regress the diagnostic output when a test fails. Looking at the comment that is deleted above I wonder if we should add else cl_assert_equal_i(buf->alloc, 0); To check that alloc is zero when len is zero. Best Wishes Phillip > } > > -static void t_static_init(void) > +void test_strbuf__static_init(void) > { > struct strbuf buf = STRBUF_INIT; > > - check_uint(buf.len, ==, 0); > - check_uint(buf.alloc, ==, 0); > - check_char(buf.buf[0], ==, '\0'); > + cl_assert_equal_i(buf.len, 0); > + cl_assert_equal_i(buf.alloc, 0); > + cl_assert(buf.buf[0] == '\0'); > } > > -static void t_dynamic_init(void) > +void test_strbuf__dynamic_init(void) > { > struct strbuf buf; > > strbuf_init(&buf, 1024); > - check(assert_sane_strbuf(&buf)); > - check_uint(buf.len, ==, 0); > - check_uint(buf.alloc, >=, 1024); > - check_char(buf.buf[0], ==, '\0'); > + assert_sane_strbuf(&buf); > + cl_assert_equal_i(buf.len, 0); > + cl_assert(buf.alloc >= 1024); > + cl_assert(buf.buf[0] == '\0'); > strbuf_release(&buf); > } > > @@ -73,16 +69,12 @@ static void t_addch(struct strbuf *buf, const void *data) > size_t orig_alloc = buf->alloc; > size_t orig_len = buf->len; > > - if (!check(assert_sane_strbuf(buf))) > - return; > + assert_sane_strbuf(buf); > strbuf_addch(buf, ch); > - if (!check(assert_sane_strbuf(buf))) > - return; > - if (!(check_uint(buf->len, ==, orig_len + 1) && > - check_uint(buf->alloc, >=, orig_alloc))) > - return; /* avoid de-referencing buf->buf */ > - check_char(buf->buf[buf->len - 1], ==, ch); > - check_char(buf->buf[buf->len], ==, '\0'); > + assert_sane_strbuf(buf); > + cl_assert_equal_i(buf->len, orig_len + 1); > + cl_assert(buf->alloc >= orig_alloc); > + cl_assert(buf->buf[buf->len] == '\0'); > } > > static void t_addstr(struct strbuf *buf, const void *data) > @@ -92,31 +84,36 @@ static void t_addstr(struct strbuf *buf, const void *data) > size_t orig_alloc = buf->alloc; > size_t orig_len = buf->len; > > - if (!check(assert_sane_strbuf(buf))) > - return; > + assert_sane_strbuf(buf); > strbuf_addstr(buf, text); > - if (!check(assert_sane_strbuf(buf))) > - return; > - if (!(check_uint(buf->len, ==, orig_len + len) && > - check_uint(buf->alloc, >=, orig_alloc) && > - check_uint(buf->alloc, >, orig_len + len) && > - check_char(buf->buf[orig_len + len], ==, '\0'))) > - return; > - check_str(buf->buf + orig_len, text); > + assert_sane_strbuf(buf); > + cl_assert_equal_i(buf->len, orig_len + len); > + cl_assert(buf->alloc >= orig_alloc); > + cl_assert(buf->buf[buf->len] == '\0'); > + cl_assert_equal_s(buf->buf + orig_len, text); > } > > -int cmd_main(int argc UNUSED, const char **argv UNUSED) > +void test_strbuf__add_single_char(void) > { > - if (!TEST(t_static_init(), "static initialization works")) > - test_skip_all("STRBUF_INIT is broken"); > - TEST(t_dynamic_init(), "dynamic initialization works"); > - TEST(setup(t_addch, "a"), "strbuf_addch adds char"); > - TEST(setup(t_addch, ""), "strbuf_addch adds NUL char"); > - TEST(setup_populated(t_addch, "initial value", "a"), > - "strbuf_addch appends to initial value"); > - TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string"); > - TEST(setup_populated(t_addstr, "initial value", "hello there"), > - "strbuf_addstr appends string to initial value"); > - > - return test_done(); > + setup(t_addch, "a"); > +} > + > +void test_strbuf__add_empty_char(void) > +{ > + setup(t_addch, ""); > +} > + > +void test_strbuf__add_append_char(void) > +{ > + setup_populated(t_addch, "initial value", "a"); > +} > + > +void test_strbuf__add_single_str(void) > +{ > + setup(t_addstr, "hello there"); > +} > + > +void test_strbuf__add_append_str(void) > +{ > + setup_populated(t_addstr, "initial value", "hello there"); > }
diff --git a/Makefile b/Makefile index 732d765f1c..358193597f 100644 --- a/Makefile +++ b/Makefile @@ -1344,6 +1344,7 @@ CLAR_TEST_SUITES += u-hashmap CLAR_TEST_SUITES += u-mem-pool CLAR_TEST_SUITES += u-prio-queue CLAR_TEST_SUITES += u-reftable-tree +CLAR_TEST_SUITES += u-strbuf CLAR_TEST_SUITES += u-strvec CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X) CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES)) @@ -1361,7 +1362,6 @@ UNIT_TEST_PROGRAMS += t-reftable-reader UNIT_TEST_PROGRAMS += t-reftable-readwrite UNIT_TEST_PROGRAMS += t-reftable-record UNIT_TEST_PROGRAMS += t-reftable-stack -UNIT_TEST_PROGRAMS += t-strbuf UNIT_TEST_PROGRAMS += t-strcmp-offset UNIT_TEST_PROGRAMS += t-trailer UNIT_TEST_PROGRAMS += t-urlmatch-normalization diff --git a/t/meson.build b/t/meson.build index c7e08eca6f..6cb72842b1 100644 --- a/t/meson.build +++ b/t/meson.build @@ -6,6 +6,7 @@ clar_test_suites = [ 'unit-tests/u-mem-pool.c', 'unit-tests/u-prio-queue.c', 'unit-tests/u-reftable-tree.c', + 'unit-tests/u-strbuf.c', 'unit-tests/u-strvec.c', ] @@ -57,7 +58,6 @@ unit_test_programs = [ 'unit-tests/t-reftable-readwrite.c', 'unit-tests/t-reftable-record.c', 'unit-tests/t-reftable-stack.c', - 'unit-tests/t-strbuf.c', 'unit-tests/t-strcmp-offset.c', 'unit-tests/t-trailer.c', 'unit-tests/t-urlmatch-normalization.c', diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/u-strbuf.c similarity index 35% rename from t/unit-tests/t-strbuf.c rename to t/unit-tests/u-strbuf.c index 3f4044d697..caa5d78aa3 100644 --- a/t/unit-tests/t-strbuf.c +++ b/t/unit-tests/u-strbuf.c @@ -1,4 +1,4 @@ -#include "test-lib.h" +#include "unit-test.h" #include "strbuf.h" /* wrapper that supplies tests with an empty, initialized strbuf */ @@ -9,8 +9,8 @@ static void setup(void (*f)(struct strbuf*, const void*), f(&buf, data); strbuf_release(&buf); - check_uint(buf.len, ==, 0); - check_uint(buf.alloc, ==, 0); + cl_assert_equal_i(buf.len, 0); + cl_assert_equal_i(buf.alloc, 0); } /* wrapper that supplies tests with a populated, initialized strbuf */ @@ -20,49 +20,45 @@ static void setup_populated(void (*f)(struct strbuf*, const void*), struct strbuf buf = STRBUF_INIT; strbuf_addstr(&buf, init_str); - check_uint(buf.len, ==, strlen(init_str)); + cl_assert_equal_i(buf.len, strlen(init_str)); f(&buf, data); strbuf_release(&buf); - check_uint(buf.len, ==, 0); - check_uint(buf.alloc, ==, 0); + cl_assert_equal_i(buf.len, 0); + cl_assert_equal_i(buf.alloc, 0); } -static int assert_sane_strbuf(struct strbuf *buf) +static void assert_sane_strbuf(struct strbuf *buf) { /* Initialized strbufs should always have a non-NULL buffer */ - if (!check(!!buf->buf)) - return 0; + cl_assert(buf->buf != NULL); /* Buffers should always be NUL-terminated */ - if (!check_char(buf->buf[buf->len], ==, '\0')) - return 0; + cl_assert(buf->buf[buf->len] == '\0'); /* - * Freshly-initialized strbufs may not have a dynamically allocated - * buffer - */ - if (buf->len == 0 && buf->alloc == 0) - return 1; - /* alloc must be at least one byte larger than len */ - return check_uint(buf->len, <, buf->alloc); + * In case the buffer contains anything, `alloc` must alloc must + * be at least one byte larger than `len`. + */ + if (buf->len) + cl_assert(buf->len < buf->alloc); } -static void t_static_init(void) +void test_strbuf__static_init(void) { struct strbuf buf = STRBUF_INIT; - check_uint(buf.len, ==, 0); - check_uint(buf.alloc, ==, 0); - check_char(buf.buf[0], ==, '\0'); + cl_assert_equal_i(buf.len, 0); + cl_assert_equal_i(buf.alloc, 0); + cl_assert(buf.buf[0] == '\0'); } -static void t_dynamic_init(void) +void test_strbuf__dynamic_init(void) { struct strbuf buf; strbuf_init(&buf, 1024); - check(assert_sane_strbuf(&buf)); - check_uint(buf.len, ==, 0); - check_uint(buf.alloc, >=, 1024); - check_char(buf.buf[0], ==, '\0'); + assert_sane_strbuf(&buf); + cl_assert_equal_i(buf.len, 0); + cl_assert(buf.alloc >= 1024); + cl_assert(buf.buf[0] == '\0'); strbuf_release(&buf); } @@ -73,16 +69,12 @@ static void t_addch(struct strbuf *buf, const void *data) size_t orig_alloc = buf->alloc; size_t orig_len = buf->len; - if (!check(assert_sane_strbuf(buf))) - return; + assert_sane_strbuf(buf); strbuf_addch(buf, ch); - if (!check(assert_sane_strbuf(buf))) - return; - if (!(check_uint(buf->len, ==, orig_len + 1) && - check_uint(buf->alloc, >=, orig_alloc))) - return; /* avoid de-referencing buf->buf */ - check_char(buf->buf[buf->len - 1], ==, ch); - check_char(buf->buf[buf->len], ==, '\0'); + assert_sane_strbuf(buf); + cl_assert_equal_i(buf->len, orig_len + 1); + cl_assert(buf->alloc >= orig_alloc); + cl_assert(buf->buf[buf->len] == '\0'); } static void t_addstr(struct strbuf *buf, const void *data) @@ -92,31 +84,36 @@ static void t_addstr(struct strbuf *buf, const void *data) size_t orig_alloc = buf->alloc; size_t orig_len = buf->len; - if (!check(assert_sane_strbuf(buf))) - return; + assert_sane_strbuf(buf); strbuf_addstr(buf, text); - if (!check(assert_sane_strbuf(buf))) - return; - if (!(check_uint(buf->len, ==, orig_len + len) && - check_uint(buf->alloc, >=, orig_alloc) && - check_uint(buf->alloc, >, orig_len + len) && - check_char(buf->buf[orig_len + len], ==, '\0'))) - return; - check_str(buf->buf + orig_len, text); + assert_sane_strbuf(buf); + cl_assert_equal_i(buf->len, orig_len + len); + cl_assert(buf->alloc >= orig_alloc); + cl_assert(buf->buf[buf->len] == '\0'); + cl_assert_equal_s(buf->buf + orig_len, text); } -int cmd_main(int argc UNUSED, const char **argv UNUSED) +void test_strbuf__add_single_char(void) { - if (!TEST(t_static_init(), "static initialization works")) - test_skip_all("STRBUF_INIT is broken"); - TEST(t_dynamic_init(), "dynamic initialization works"); - TEST(setup(t_addch, "a"), "strbuf_addch adds char"); - TEST(setup(t_addch, ""), "strbuf_addch adds NUL char"); - TEST(setup_populated(t_addch, "initial value", "a"), - "strbuf_addch appends to initial value"); - TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string"); - TEST(setup_populated(t_addstr, "initial value", "hello there"), - "strbuf_addstr appends string to initial value"); - - return test_done(); + setup(t_addch, "a"); +} + +void test_strbuf__add_empty_char(void) +{ + setup(t_addch, ""); +} + +void test_strbuf__add_append_char(void) +{ + setup_populated(t_addch, "initial value", "a"); +} + +void test_strbuf__add_single_str(void) +{ + setup(t_addstr, "hello there"); +} + +void test_strbuf__add_append_str(void) +{ + setup_populated(t_addstr, "initial value", "hello there"); }
Adapt strbuf test script to clar framework by using clar assertions where necessary. Mentored-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com> --- Makefile | 2 +- t/meson.build | 2 +- t/unit-tests/{t-strbuf.c => u-strbuf.c} | 115 ++++++++++++------------ 3 files changed, 58 insertions(+), 61 deletions(-) rename t/unit-tests/{t-strbuf.c => u-strbuf.c} (35%)