| Message ID | 1bf053ae-957e-4e9a-90f0-11cc76848ce9@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unit-tests: add and use TEST_RUN to simplify tests | expand |
On 2024.06.29 17:47, René Scharfe wrote: > The macro TEST takes a single expression. If a test requires multiple > statements then they need to be placed in a function that's called in > the TEST expression. The functions setup() and setup_populated() here > are used for that purpose and take another function as an argument, > making the control flow hard to follow. > > Remove the overhead of these functions by using TEST_RUN instead. Move > their duplicate post-condition checks into a new helper, t_release(), > and let t_addch() and t_addstr() accept properly typed input parameters > instead of void pointers. > > Use the fully checking t_addstr() for adding initial values instead of > only doing only a length comparison -- there's no need for skipping the > other checks. > > This results in test cases that look much more like strbuf usage in > production code, only with checked strbuf functions replaced by checking > wrappers. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > t/unit-tests/t-strbuf.c | 79 +++++++++++++++++++++-------------------- > 1 file changed, 41 insertions(+), 38 deletions(-) > > diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c > index 6027dafef7..c8e39ddda7 100644 > --- a/t/unit-tests/t-strbuf.c > +++ b/t/unit-tests/t-strbuf.c > @@ -1,32 +1,6 @@ > #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); > - strbuf_release(&buf); > - check_uint(buf.len, ==, 0); > - check_uint(buf.alloc, ==, 0); > -} > - > static int assert_sane_strbuf(struct strbuf *buf) > { > /* Initialized strbufs should always have a non-NULL buffer */ > @@ -66,10 +40,8 @@ static void t_dynamic_init(void) > strbuf_release(&buf); > } > > -static void t_addch(struct strbuf *buf, const void *data) > +static void t_addch(struct strbuf *buf, int 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 +57,8 @@ 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) > +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; > @@ -105,18 +76,50 @@ static void t_addstr(struct strbuf *buf, const void *data) > check_str(buf->buf + orig_len, text); > } > > +static void t_release(struct strbuf *sb) > +{ > + strbuf_release(sb); > + check_uint(sb->len, ==, 0); > + check_uint(sb->alloc, ==, 0); > +} > + > 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"); IIUC you're leaving t_static_init() as-is so that we can determine whether or not to skip the rest of the tests, but is there a reason you didn't convert t_dynamic_init() here? > - 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"); > + > + if (TEST_RUN("strbuf_addch adds char")) { > + struct strbuf sb = STRBUF_INIT; > + t_addch(&sb, 'a'); > + t_release(&sb); > + } > + > + if (TEST_RUN("strbuf_addch adds NUL char")) { > + struct strbuf sb = STRBUF_INIT; > + t_addch(&sb, '\0'); > + t_release(&sb); > + } > + > + if (TEST_RUN("strbuf_addch appends to initial value")) { > + struct strbuf sb = STRBUF_INIT; > + t_addstr(&sb, "initial value"); > + t_addch(&sb, 'a'); > + t_release(&sb); > + } > + > + if (TEST_RUN("strbuf_addstr adds string")) { > + struct strbuf sb = STRBUF_INIT; > + t_addstr(&sb, "hello there"); > + t_release(&sb); > + } > + > + if (TEST_RUN("strbuf_addstr appends string to initial value")) { > + struct strbuf sb = STRBUF_INIT; > + t_addstr(&sb, "initial value"); > + t_addstr(&sb, "hello there"); > + t_release(&sb); > + } > > return test_done(); > } > -- > 2.45.2 I think this commit in particular shows how TEST_RUN() is more convenient than TEST(). (Although, arguably we shouldn't have allowed the setup() + callback situation to start with.)
Am 01.07.24 um 21:58 schrieb Josh Steadmon: > On 2024.06.29 17:47, René Scharfe wrote: >> The macro TEST takes a single expression. If a test requires multiple >> statements then they need to be placed in a function that's called in >> the TEST expression. The functions setup() and setup_populated() here >> are used for that purpose and take another function as an argument, >> making the control flow hard to follow. >> >> Remove the overhead of these functions by using TEST_RUN instead. Move >> their duplicate post-condition checks into a new helper, t_release(), >> and let t_addch() and t_addstr() accept properly typed input parameters >> instead of void pointers. >> >> Use the fully checking t_addstr() for adding initial values instead of >> only doing only a length comparison -- there's no need for skipping the >> other checks. >> >> This results in test cases that look much more like strbuf usage in >> production code, only with checked strbuf functions replaced by checking >> wrappers. >> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> t/unit-tests/t-strbuf.c | 79 +++++++++++++++++++++-------------------- >> 1 file changed, 41 insertions(+), 38 deletions(-) >> >> diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c >> index 6027dafef7..c8e39ddda7 100644 >> --- a/t/unit-tests/t-strbuf.c >> +++ b/t/unit-tests/t-strbuf.c >> @@ -1,32 +1,6 @@ >> #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); >> - strbuf_release(&buf); >> - check_uint(buf.len, ==, 0); >> - check_uint(buf.alloc, ==, 0); >> -} >> - >> static int assert_sane_strbuf(struct strbuf *buf) >> { >> /* Initialized strbufs should always have a non-NULL buffer */ >> @@ -66,10 +40,8 @@ static void t_dynamic_init(void) >> strbuf_release(&buf); >> } >> >> -static void t_addch(struct strbuf *buf, const void *data) >> +static void t_addch(struct strbuf *buf, int 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 +57,8 @@ 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) >> +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; >> @@ -105,18 +76,50 @@ static void t_addstr(struct strbuf *buf, const void *data) >> check_str(buf->buf + orig_len, text); >> } >> >> +static void t_release(struct strbuf *sb) >> +{ >> + strbuf_release(sb); >> + check_uint(sb->len, ==, 0); >> + check_uint(sb->alloc, ==, 0); >> +} >> + >> 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"); > > IIUC you're leaving t_static_init() as-is so that we can determine > whether or not to skip the rest of the tests, Right. And that place might be a use case for an official version of test__run_end(), but it's probably better to gather a few more such candidates before drawing conclusions. > but is there a reason you > didn't convert t_dynamic_init() here? Good question, I don't remember. Perhaps an oversight or laziness. Or just a general feeling to leave the init tests alone since the first one doesn't map to TEST_RUN easily. So, in a word: No. >> - 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"); >> + >> + if (TEST_RUN("strbuf_addch adds char")) { >> + struct strbuf sb = STRBUF_INIT; >> + t_addch(&sb, 'a'); >> + t_release(&sb); >> + } >> + >> + if (TEST_RUN("strbuf_addch adds NUL char")) { >> + struct strbuf sb = STRBUF_INIT; >> + t_addch(&sb, '\0'); >> + t_release(&sb); >> + } >> + >> + if (TEST_RUN("strbuf_addch appends to initial value")) { >> + struct strbuf sb = STRBUF_INIT; >> + t_addstr(&sb, "initial value"); >> + t_addch(&sb, 'a'); >> + t_release(&sb); >> + } >> + >> + if (TEST_RUN("strbuf_addstr adds string")) { >> + struct strbuf sb = STRBUF_INIT; >> + t_addstr(&sb, "hello there"); >> + t_release(&sb); >> + } >> + >> + if (TEST_RUN("strbuf_addstr appends string to initial value")) { >> + struct strbuf sb = STRBUF_INIT; >> + t_addstr(&sb, "initial value"); >> + t_addstr(&sb, "hello there"); >> + t_release(&sb); >> + } >> >> return test_done(); >> } >> -- >> 2.45.2 > > I think this commit in particular shows how TEST_RUN() is more > convenient than TEST(). (Although, arguably we shouldn't have allowed > the setup() + callback situation to start with.) Great, thanks! René
Hi Josh and René On 01/07/2024 20:58, Josh Steadmon wrote: > On 2024.06.29 17:47, René Scharfe wrote: > > I think this commit in particular shows how TEST_RUN() is more > convenient than TEST(). (Although, arguably we shouldn't have allowed > the setup() + callback situation to start with.) I think the counterargument to that is that using TEST_RUN() makes the tests noisier and more error prone because each one has to be wrapped in an if() statement and has more boiler plate initializing and freeing the strbuf rather than getting that for free by calling the test function via setup(). Having said that I don't mind the changes in this patch if that's the way others want to go. Getting rid of the untyped test arguments is definitely a benefit of this approach. Best Wishes Phillip
Josh Steadmon <steadmon@google.com> wrote: > > - 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"); > > + > > + if (TEST_RUN("strbuf_addch adds char")) { > > + struct strbuf sb = STRBUF_INIT; > > + t_addch(&sb, 'a'); > > + t_release(&sb); > > + } > > + > > + if (TEST_RUN("strbuf_addch adds NUL char")) { > > + struct strbuf sb = STRBUF_INIT; > > + t_addch(&sb, '\0'); > > + t_release(&sb); > > + } > > + > > + if (TEST_RUN("strbuf_addch appends to initial value")) { > > + struct strbuf sb = STRBUF_INIT; > > + t_addstr(&sb, "initial value"); > > + t_addch(&sb, 'a'); > > + t_release(&sb); > > + } > > + > > + if (TEST_RUN("strbuf_addstr adds string")) { > > + struct strbuf sb = STRBUF_INIT; > > + t_addstr(&sb, "hello there"); > > + t_release(&sb); > > + } > > + > > + if (TEST_RUN("strbuf_addstr appends string to initial value")) { > > + struct strbuf sb = STRBUF_INIT; > > + t_addstr(&sb, "initial value"); > > + t_addstr(&sb, "hello there"); > > + t_release(&sb); > > + } > > > > return test_done(); > > } > > -- > > 2.45.2 > > I think this commit in particular shows how TEST_RUN() is more > convenient than TEST(). (Although, arguably we shouldn't have allowed > the setup() + callback situation to start with.) Could you expand a bit on why the setup() + callback thing shouldn't be allowed? I think it is a nice way of avoiding boilerplate and having independent state. And, I see the true potential of TEST_RUN() in testcases defined through macros rather than replacing functions. I actually think that the previous version with the functions was not particularly bad, and I agree with Phillip that the previous version's main() provided nice overview of the tests and it was easier to verify the independence between each testcase. Perhaps, the code snippets inside the functions are small enough to perceive TEST_RUN() as more convenient than TEST() in this test, but, for future reference, I definitely don't think TEST_RUN() should be looked at as a replacement for TEST(), and more like 'when we have to use macro magic which requires us to use internal test__run_* functions, using TEST_RUN() is more convenient'. Patch [3/6] is a good example of that. But, I also don't mind if patches 4, 5, or 6 get merged as I don't see any difference between using TEST_RUN() or TEST() in those patches, besides moving everything inside main(). Thanks.
Am 02.07.24 um 17:14 schrieb phillip.wood123@gmail.com: > Hi Josh and René > > On 01/07/2024 20:58, Josh Steadmon wrote: >> On 2024.06.29 17:47, René Scharfe wrote: >> >> I think this commit in particular shows how TEST_RUN() is more >> convenient than TEST(). (Although, arguably we shouldn't have allowed >> the setup() + callback situation to start with.) > > I think the counterargument to that is that using TEST_RUN() makes > the tests noisier and more error prone because each one has to be > wrapped in an if() statement and has more boiler plate initializing > and freeing the strbuf rather than getting that for free by calling > the test function via setup(). I guess these are two sides of the same coin. Explicit initialization and cleanup is closer to what real strbuf users do, more idiomatic. Which helps to see what the tests are actually doing. The wrapping if is less annoying than a wrapping function that I have to name and call. The condition is just !ctx.skip_all, though, (for now at least) so we could do something like: #define TEST_START(...) if (!TEST_RUN(__VA_ARGS__)) return test_done() ... and tests could look like that then: TEST_START("this and that"); check(this); check(that); TEST_START("whatever"); check(whatever); Perhaps a bit too much magic, though? René
Am 02.07.24 um 19:29 schrieb Ghanshyam Thakkar: > Josh Steadmon <steadmon@google.com> wrote: >>> - 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"); >>> + >>> + if (TEST_RUN("strbuf_addch adds char")) { >>> + struct strbuf sb = STRBUF_INIT; >>> + t_addch(&sb, 'a'); >>> + t_release(&sb); >>> + } >>> + >>> + if (TEST_RUN("strbuf_addch adds NUL char")) { >>> + struct strbuf sb = STRBUF_INIT; >>> + t_addch(&sb, '\0'); >>> + t_release(&sb); >>> + } >>> + >>> + if (TEST_RUN("strbuf_addch appends to initial value")) { >>> + struct strbuf sb = STRBUF_INIT; >>> + t_addstr(&sb, "initial value"); >>> + t_addch(&sb, 'a'); >>> + t_release(&sb); >>> + } >>> + >>> + if (TEST_RUN("strbuf_addstr adds string")) { >>> + struct strbuf sb = STRBUF_INIT; >>> + t_addstr(&sb, "hello there"); >>> + t_release(&sb); >>> + } >>> + >>> + if (TEST_RUN("strbuf_addstr appends string to initial value")) { >>> + struct strbuf sb = STRBUF_INIT; >>> + t_addstr(&sb, "initial value"); >>> + t_addstr(&sb, "hello there"); >>> + t_release(&sb); >>> + } >>> >>> return test_done(); >>> } >>> -- >>> 2.45.2 >> >> I think this commit in particular shows how TEST_RUN() is more >> convenient than TEST(). (Although, arguably we shouldn't have allowed >> the setup() + callback situation to start with.) > > Could you expand a bit on why the setup() + callback thing shouldn't be > allowed? I think it is a nice way of avoiding boilerplate and having > independent state. And, I see the true potential of TEST_RUN() in > testcases defined through macros rather than replacing functions. I > actually think that the previous version with the functions was not > particularly bad, and I agree with Phillip that the previous version's > main() provided nice overview of the tests and it was easier to > verify the independence between each testcase. Each test uses its own strbuf and the t_ functions don't use global or static variables, so how does the doubt about their independence creep in? With tests specified as above I now notice that we never check the resulting string. t_addch and t_addstr check the tail matches their argument, but strbuf_addch and strbuf_addstr could overwrite the old content and we wouldn't notice. Perhaps it's not worth checking, but now that tests look more like test_expect_success and all steps are visible I somehow miss the equivalent of test_cmp; didn't see that in the original version. > Perhaps, the code snippets inside the functions are small enough to > perceive TEST_RUN() as more convenient than TEST() in this test, but, > for future reference, I definitely don't think TEST_RUN() should be > looked at as a replacement for TEST(), and more like 'when we have to > use macro magic which requires us to use internal test__run_* > functions, using TEST_RUN() is more convenient. Patch [3/6] is a good> example of that. Sure, not all tests will be improved by using TEST_RUN. If TEST fits better, use it. > But, I also don't mind if patches 4, 5, or 6 get > merged as I don't see any difference between using TEST_RUN() or > TEST() in those patches, besides moving everything inside main(). The difference is that in the original version test description and definition are separated, only linked by a function name. The new version brings them together and does away with function name. A small change, for sure, just to get rid of the artificial divide and the need for that link. René
René Scharfe <l.s.r@web.de> wrote: > Am 02.07.24 um 19:29 schrieb Ghanshyam Thakkar: > > Josh Steadmon <steadmon@google.com> wrote: > >>> - 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"); > >>> + > >>> + if (TEST_RUN("strbuf_addch adds char")) { > >>> + struct strbuf sb = STRBUF_INIT; > >>> + t_addch(&sb, 'a'); > >>> + t_release(&sb); > >>> + } > >>> + > >>> + if (TEST_RUN("strbuf_addch adds NUL char")) { > >>> + struct strbuf sb = STRBUF_INIT; > >>> + t_addch(&sb, '\0'); > >>> + t_release(&sb); > >>> + } > >>> + > >>> + if (TEST_RUN("strbuf_addch appends to initial value")) { > >>> + struct strbuf sb = STRBUF_INIT; > >>> + t_addstr(&sb, "initial value"); > >>> + t_addch(&sb, 'a'); > >>> + t_release(&sb); > >>> + } > >>> + > >>> + if (TEST_RUN("strbuf_addstr adds string")) { > >>> + struct strbuf sb = STRBUF_INIT; > >>> + t_addstr(&sb, "hello there"); > >>> + t_release(&sb); > >>> + } > >>> + > >>> + if (TEST_RUN("strbuf_addstr appends string to initial value")) { > >>> + struct strbuf sb = STRBUF_INIT; > >>> + t_addstr(&sb, "initial value"); > >>> + t_addstr(&sb, "hello there"); > >>> + t_release(&sb); > >>> + } > >>> > >>> return test_done(); > >>> } > >>> -- > >>> 2.45.2 > >> > >> I think this commit in particular shows how TEST_RUN() is more > >> convenient than TEST(). (Although, arguably we shouldn't have allowed > >> the setup() + callback situation to start with.) > > > > Could you expand a bit on why the setup() + callback thing shouldn't be > > allowed? I think it is a nice way of avoiding boilerplate and having > > independent state. And, I see the true potential of TEST_RUN() in > > testcases defined through macros rather than replacing functions. I > > actually think that the previous version with the functions was not > > particularly bad, and I agree with Phillip that the previous version's > > main() provided nice overview of the tests and it was easier to > > verify the independence between each testcase. > > Each test uses its own strbuf and the t_ functions don't use global or > static variables, so how does the doubt about their independence creep > in? Ah, apologies. I should clarify that I meant in general terms about the future uses of TEST_RUN() and not about this particular patch. But I see it being less of a problem now that I think about it more. And for the record, I see no problems in this patch. But on a side note, with what Phillip was suggesting to remove having TEST_RUN() inside if(), it would definitely make verifying state independence more harder. <snip> > > But, I also don't mind if patches 4, 5, or 6 get > > merged as I don't see any difference between using TEST_RUN() or > > TEST() in those patches, besides moving everything inside main(). > > The difference is that in the original version test description and > definition are separated, only linked by a function name. The new > version brings them together and does away with function name. A small > change, for sure, just to get rid of the artificial divide and the need > for that link. Yeah, but I didn't mind that divide (and I don't mind bringing them together either). :) Thanks.
On 02/07/2024 21:55, René Scharfe wrote: > Am 02.07.24 um 17:14 schrieb phillip.wood123@gmail.com: >> Hi Josh and René >> >> On 01/07/2024 20:58, Josh Steadmon wrote: >>> On 2024.06.29 17:47, René Scharfe wrote: >>> >>> I think this commit in particular shows how TEST_RUN() is more >>> convenient than TEST(). (Although, arguably we shouldn't have allowed >>> the setup() + callback situation to start with.) >> >> I think the counterargument to that is that using TEST_RUN() makes >> the tests noisier and more error prone because each one has to be >> wrapped in an if() statement and has more boiler plate initializing >> and freeing the strbuf rather than getting that for free by calling >> the test function via setup(). > > I guess these are two sides of the same coin. Explicit initialization > and cleanup is closer to what real strbuf users do, more idiomatic. > Which helps to see what the tests are actually doing. > > The wrapping if is less annoying than a wrapping function that I have > to name and call. The condition is just !ctx.skip_all, though, (for > now at least) so we could do something like: > > #define TEST_START(...) if (!TEST_RUN(__VA_ARGS__)) return test_done() > > ... and tests could look like that then: > > TEST_START("this and that"); > check(this); > check(that); > > TEST_START("whatever"); > check(whatever); > > Perhaps a bit too much magic, though? I think so. It's an interesting idea, but at some point someone will probably want to be able to run a subset of the tests in a file like "--run" does for the integration tests so I don't think we want to assume we can skip all the tests just because any particular test should be skipped. Best Wishes Phillip > René
On 2024.07.02 22:59, Ghanshyam Thakkar wrote: > Josh Steadmon <steadmon@google.com> wrote: > > > - 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"); > > > + > > > + if (TEST_RUN("strbuf_addch adds char")) { > > > + struct strbuf sb = STRBUF_INIT; > > > + t_addch(&sb, 'a'); > > > + t_release(&sb); > > > + } > > > + > > > + if (TEST_RUN("strbuf_addch adds NUL char")) { > > > + struct strbuf sb = STRBUF_INIT; > > > + t_addch(&sb, '\0'); > > > + t_release(&sb); > > > + } > > > + > > > + if (TEST_RUN("strbuf_addch appends to initial value")) { > > > + struct strbuf sb = STRBUF_INIT; > > > + t_addstr(&sb, "initial value"); > > > + t_addch(&sb, 'a'); > > > + t_release(&sb); > > > + } > > > + > > > + if (TEST_RUN("strbuf_addstr adds string")) { > > > + struct strbuf sb = STRBUF_INIT; > > > + t_addstr(&sb, "hello there"); > > > + t_release(&sb); > > > + } > > > + > > > + if (TEST_RUN("strbuf_addstr appends string to initial value")) { > > > + struct strbuf sb = STRBUF_INIT; > > > + t_addstr(&sb, "initial value"); > > > + t_addstr(&sb, "hello there"); > > > + t_release(&sb); > > > + } > > > > > > return test_done(); > > > } > > > -- > > > 2.45.2 > > > > I think this commit in particular shows how TEST_RUN() is more > > convenient than TEST(). (Although, arguably we shouldn't have allowed > > the setup() + callback situation to start with.) > > Could you expand a bit on why the setup() + callback thing shouldn't be > allowed? I think it is a nice way of avoiding boilerplate and having > independent state. It's a matter of taste rather than strict principles, I suppose, but I think that test cases should focus on being readable rather than avoiding duplication. Production code can follow a Don't Repeat Yourself philosophy, but when a test breaks, it's better to be able to look at only the failing test and quickly see what's wrong, rather than having to work out which functions call which callbacks and what gets verified where. Also, since we don't have tests for our tests, it's important that the tests are easy for people to manually verify them. I also agree with René's point about the test cases matching real-world API usage. I do agree that having separate functions improves isolation, but hopefully we can encourage test authors to define local variables in their TEST_RUN blocks. > And, I see the true potential of TEST_RUN() in > testcases defined through macros rather than replacing functions. I > actually think that the previous version with the functions was not > particularly bad, and I agree with Phillip that the previous version's > main() provided nice overview of the tests and it was easier to > verify the independence between each testcase. > > Perhaps, the code snippets inside the functions are small enough to > perceive TEST_RUN() as more convenient than TEST() in this test, but, > for future reference, I definitely don't think TEST_RUN() should be > looked at as a replacement for TEST(), and more like 'when we have to > use macro magic which requires us to use internal test__run_* > functions, using TEST_RUN() is more convenient'. Patch [3/6] is a good > example of that. But, I also don't mind if patches 4, 5, or 6 get > merged as I don't see any difference between using TEST_RUN() or > TEST() in those patches, besides moving everything inside main(). > > Thanks.
Josh Steadmon <steadmon@google.com> wrote: > On 2024.07.02 22:59, Ghanshyam Thakkar wrote: > > Josh Steadmon <steadmon@google.com> wrote: > > > I think this commit in particular shows how TEST_RUN() is more > > > convenient than TEST(). (Although, arguably we shouldn't have allowed > > > the setup() + callback situation to start with.) > > > > Could you expand a bit on why the setup() + callback thing shouldn't be > > allowed? I think it is a nice way of avoiding boilerplate and having > > independent state. > > It's a matter of taste rather than strict principles, I suppose, but I > think that test cases should focus on being readable rather than > avoiding duplication. Production code can follow a Don't Repeat Yourself > philosophy, but when a test breaks, it's better to be able to look at > only the failing test and quickly see what's wrong, rather than having > to work out which functions call which callbacks and what gets verified > where. Also, since we don't have tests for our tests, it's important > that the tests are easy for people to manually verify them. Makes sense. But the manner in which callbacks are used in these tests, which usually only initialize a certain datastructure and then release them at the end, I fail to see them as more harder to manually verify. But it is very easy to stretch the line and perhaps do more than initialization in the setup(), so I can see how disallowing them would get rid of the need of judging every time wether they are readable or not. Maybe a 'best practices' document would help in that regard? > I also agree with René's point about the test cases matching real-world > API usage. > > I do agree that having separate functions improves isolation, but > hopefully we can encourage test authors to define local variables in > their TEST_RUN blocks. Makes sense. Thanks.
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 ---- >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();
Am 10.07.24 um 15:55 schrieb Phillip Wood: > 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. I'm sympathetic with the idea of enhancing the weak type system of C to avoid mistakes. Hiding the use of void pointers behind typed wrappers can be useful. Not using void pointers is even better and clearer, though, where possible. Thought about using such wrappers around qsort(3) and friends long ago as well as parse_options() more recently, but didn't arrive at mergeable patches, yet. Initialization and destruction seem easy and familiar enough to not warrant the use of void pointers. No need to get fancy just for basic necessities IMHO. René
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();
diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c index 6027dafef7..c8e39ddda7 100644 --- a/t/unit-tests/t-strbuf.c +++ b/t/unit-tests/t-strbuf.c @@ -1,32 +1,6 @@ #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); - strbuf_release(&buf); - check_uint(buf.len, ==, 0); - check_uint(buf.alloc, ==, 0); -} - static int assert_sane_strbuf(struct strbuf *buf) { /* Initialized strbufs should always have a non-NULL buffer */ @@ -66,10 +40,8 @@ static void t_dynamic_init(void) strbuf_release(&buf); } -static void t_addch(struct strbuf *buf, const void *data) +static void t_addch(struct strbuf *buf, int 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 +57,8 @@ 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) +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; @@ -105,18 +76,50 @@ static void t_addstr(struct strbuf *buf, const void *data) check_str(buf->buf + orig_len, text); } +static void t_release(struct strbuf *sb) +{ + strbuf_release(sb); + check_uint(sb->len, ==, 0); + check_uint(sb->alloc, ==, 0); +} + 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"), - "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"); + + if (TEST_RUN("strbuf_addch adds char")) { + struct strbuf sb = STRBUF_INIT; + t_addch(&sb, 'a'); + t_release(&sb); + } + + if (TEST_RUN("strbuf_addch adds NUL char")) { + struct strbuf sb = STRBUF_INIT; + t_addch(&sb, '\0'); + t_release(&sb); + } + + if (TEST_RUN("strbuf_addch appends to initial value")) { + struct strbuf sb = STRBUF_INIT; + t_addstr(&sb, "initial value"); + t_addch(&sb, 'a'); + t_release(&sb); + } + + if (TEST_RUN("strbuf_addstr adds string")) { + struct strbuf sb = STRBUF_INIT; + t_addstr(&sb, "hello there"); + t_release(&sb); + } + + if (TEST_RUN("strbuf_addstr appends string to initial value")) { + struct strbuf sb = STRBUF_INIT; + t_addstr(&sb, "initial value"); + t_addstr(&sb, "hello there"); + t_release(&sb); + } return test_done(); }
The macro TEST takes a single expression. If a test requires multiple statements then they need to be placed in a function that's called in the TEST expression. The functions setup() and setup_populated() here are used for that purpose and take another function as an argument, making the control flow hard to follow. Remove the overhead of these functions by using TEST_RUN instead. Move their duplicate post-condition checks into a new helper, t_release(), and let t_addch() and t_addstr() accept properly typed input parameters instead of void pointers. Use the fully checking t_addstr() for adding initial values instead of only doing only a length comparison -- there's no need for skipping the other checks. This results in test cases that look much more like strbuf usage in production code, only with checked strbuf functions replaced by checking wrappers. Signed-off-by: René Scharfe <l.s.r@web.de> --- t/unit-tests/t-strbuf.c | 79 +++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 38 deletions(-) -- 2.45.2