Re: [PATCH 6/6] t-strbuf: use TEST_RUN

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 02/07/2024 16:14, phillip.wood123@gmail.com wrote:
> > Getting rid of the untyped test arguments is 
> > definitely a benefit of this approach.
>
> That got me thinking how we might make type-safe setup()
> functions. The diff below shows how we could define a macro to
> generate the functions. DEFINE_SETUP_FN(char, ch) defines setup_ch()
> that takes a test function and a char that is passed to the test with
> the initialized strbuf. I'm not sure that's the way we want to go for
> this test file but I thought I'd post it in case it is useful for
> future tests.
>
> Best Wishes
>
> Phillip

This seems interesting if we want keep using setup() architecture. But a
bit too much if we have to keep doing this in every test where we need
setup(). Maybe some of it can be abstracted away in test-lib? Anyway, I was
a bit surprised that we didn't just use 'const char *' instead of 'const void *',
as we pass a string to all of them. So, I don't see the value of using 'void *'
in the first place in this test.

Thanks.

> ---- >8 ----
> diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c
> index 6027dafef70..8fc9a8b38df 100644
> --- a/t/unit-tests/t-strbuf.c
> +++ b/t/unit-tests/t-strbuf.c
> @@ -1,27 +1,60 @@
> #include "test-lib.h"
> #include "strbuf.h"
>   
> -/* wrapper that supplies tests with an empty, initialized strbuf */
> -static void setup(void (*f)(struct strbuf*, const void*),
> - const void *data)
> -{
> - struct strbuf buf = STRBUF_INIT;
> -
> - f(&buf, data);
> - strbuf_release(&buf);
> - check_uint(buf.len, ==, 0);
> - check_uint(buf.alloc, ==, 0);
> -}
> -
> -/* wrapper that supplies tests with a populated, initialized strbuf */
> -static void setup_populated(void (*f)(struct strbuf*, const void*),
> - const char *init_str, const void *data)
> -{
> - struct strbuf buf = STRBUF_INIT;
> -
> - strbuf_addstr(&buf, init_str);
> - check_uint(buf.len, ==, strlen(init_str));
> - f(&buf, data);
> +/*
> + * Define a type safe wrapper function that supplies test functions
> + * with an initialized strbuf populated with an optional string and
> + * some data and then frees the strbuf when the test function
> + * returns. For example given the test function
> + *
> + * t_foo(struct strbuf *buf, struct foo *data)
> + *
> + * the type safe wrapper function
> + *
> + * setup_foo(void(*)(struct strbuf*, const struct foo*),
> + * const char *init_str, const struct foo*)
> + *
> + * can be defined with
> + *
> + * DEFINE_SETUP_FN(const struct foo*, foo)
> + *
> + * and used to run t_foo() with
> + *
> + * TEST(setup_foo(t_foo, "initial string", &my_foo), "test foo");
> + */
> +#define DEFINE_SETUP_FN(type, suffix) \
> + static void marshal_##suffix(void(*test_fn)(void), \
> + struct strbuf *buf, const void *ctx) \
> + { \
> + type data = *(type *)ctx; \
> + ((void(*)(struct strbuf*, type)) test_fn)(buf, data); \
> + } \
> + \
> + static void setup_##suffix(void(*test_fn)(struct strbuf*, type), \
> + const char *init_str, type data) \
> + { \
> + void *ctx = &data; \
> + do_setup(init_str, (void(*)(void)) test_fn, ctx, \
> + marshal_##suffix); \
> + }
> +
> +/*
> + * Helper function for DEFINE_SETUP_FN() that initializes the strbuf,
> + * calls the test function and releases the strbuf
> + */
> +static void do_setup(const char* init_str, void(*f)(void), const void
> *ctx,
> + void(*marshal)(void(*)(void), struct strbuf*, const void*))
> +{
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (init_str) {
> + strbuf_addstr(&buf, init_str);
> + if (!check_uint(buf.len, ==, strlen(init_str))) {
> + strbuf_release(&buf);
> + return;
> + }
> + }
> + marshal(f, &buf, ctx);
> strbuf_release(&buf);
> check_uint(buf.len, ==, 0);
> check_uint(buf.alloc, ==, 0);
> @@ -66,10 +99,9 @@ static void t_dynamic_init(void)
> strbuf_release(&buf);
> }
>   
> -static void t_addch(struct strbuf *buf, const void *data)
> +DEFINE_SETUP_FN(char, ch)
> +static void t_addch(struct strbuf *buf, char ch)
> {
> - const char *p_ch = data;
> - const char ch = *p_ch;
> size_t orig_alloc = buf->alloc;
> size_t orig_len = buf->len;
>   
> @@ -85,9 +117,9 @@ static void t_addch(struct strbuf *buf, const void
> *data)
> check_char(buf->buf[buf->len], ==, '\0');
> }
>   
> -static void t_addstr(struct strbuf *buf, const void *data)
> +DEFINE_SETUP_FN(const char*, str)
> +static void t_addstr(struct strbuf *buf, const char *text)
> {
> - const char *text = data;
> size_t len = strlen(text);
> size_t orig_alloc = buf->alloc;
> size_t orig_len = buf->len;
> @@ -110,12 +142,12 @@ int cmd_main(int argc, const char **argv)
> 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"),
> + TEST(setup_ch(t_addch, NULL, 'a'), "strbuf_addch adds char");
> + TEST(setup_ch(t_addch, NULL, '\0'), "strbuf_addch adds NUL char");
> + TEST(setup_ch(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"),
> + TEST(setup_str(t_addstr, NULL, "hello there"), "strbuf_addstr adds
> string");
> + TEST(setup_str(t_addstr, "initial value", "hello there"),
> "strbuf_addstr appends string to initial value");
>   
> return test_done();