| Message ID | 8175f239-8d4e-49f7-ae0d-dba7df8c365d@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unit-tests: add and use TEST_RUN to simplify tests | expand |
Hi René On 29/06/2024 16:43, René Scharfe wrote: > The macro TEST only allows defining a test that consists of a single > expression. Add the new macro, TEST_RUN, which provides a way to define > unit tests that are made up of one or more statements. A test started > with it implicitly ends when the next test is started or test_done() is > called. > > TEST_RUN allows defining self-contained tests en bloc, a bit like > test_expect_success does for regular tests. Unlike TEST it does not > require defining wrapper functions for test statements. There are pros and cons to not requiring one function per test. It can be a pain to have to write separate functions but it keeps each test self contained which hopefully makes it harder to have accidental dependencies between tests. Having separate functions for each test makes it easy to initialize and free resources for every test by writing a setup() function that initializes the resources, calls the test function and then frees the resources. The changes in patch 6 to use TEST_RUN() mean that each test now has more boilerplate to initialize and free the strbuf. Having each test in its own function also makes main() shorter and which means can quickly get an overview of all the test cases from it. On the other hand having all the tests defined in main() using TEST_RUN() means we can just write the test body without having to define a separate function and then call it with TEST() > No public method is provided for ending a test explicitly, yet; let's > see if we'll ever need one. This means that we do not error out if there are accidentally nested tests. That probably does not matter too much. > +int test__run(const char *location, const char *format, ...) > +{ > + va_list ap; > + char *desc; > + > + test__run_maybe_end(); > + > + va_start(ap, format); > + desc = xstrvfmt(format, ap); This uses an strbuf under the hood. So far we've avoided doing that as we want to be able to test the strbuf implementation with this test framework. We don't need to support arbitrary length strings here so we could use a fixed array and xsnprinf() instead. > +/* > + * Start a test, returns 1 if the test was actually started or 0 if it > + * was skipped. The test ends when the next test starts or test_done() > + * is called. > + */ > +#define TEST_RUN(...) test__run(TEST_LOCATION(), __VA_ARGS__) Looking ahead the plan seems to be to change most instances of TEST() to TEST_RUN(). If we are going to go that way perhaps we should steal TEST() for this macro and rename the existing TEST() macro. I'm not very enthusiastic about requiring the test author to wrap TEST_RUN() in an if() statement rather than just doing that for them. It makes it explicit but from the test author's point of view it just feels like pointless boilerplate. > /* > * test_done() must be called at the end of main(). It will print the > * plan if plan() was not called at the beginning of the test program > @@ -156,6 +163,7 @@ extern union test__tmp test__tmp[2]; > int test__run_begin(void); > __attribute__((format (printf, 3, 4))) > int test__run_end(int, const char *, const char *, ...); We should add __attribute__((format (printf, 2, 3), warn_unused_result)) here to catch any errors in the format string / arguments and to warn if TEST_RUN() isn't wrapped in an if() statement. Best Wishes Phillip > +int test__run(const char *location, const char *format, ...); > void test__todo_begin(void); > int test__todo_end(const char *, const char *, int); > > -- > 2.45.2
phillip.wood123@gmail.com writes: >> No public method is provided for ending a test explicitly, yet; let's >> see if we'll ever need one. > > This means that we do not error out if there are accidentally nested > tests. That probably does not matter too much. Isn't it more like it is impossible to create nested tests, since a "begin" implicitly ends the current one before starting the new one? >> @@ -156,6 +163,7 @@ extern union test__tmp test__tmp[2]; >> int test__run_begin(void); >> __attribute__((format (printf, 3, 4))) >> int test__run_end(int, const char *, const char *, ...); > > We should add > > __attribute__((format (printf, 2, 3), warn_unused_result)) > > here to catch any errors in the format string / arguments and to warn > if TEST_RUN() isn't wrapped in an if() statement. Nice. Especially the "unused" check is valuable. Thanks.
Am 02.07.24 um 17:51 schrieb Junio C Hamano: > phillip.wood123@gmail.com writes: > >>> @@ -156,6 +163,7 @@ extern union test__tmp test__tmp[2]; >>> int test__run_begin(void); >>> __attribute__((format (printf, 3, 4))) >>> int test__run_end(int, const char *, const char *, ...); >> >> We should add >> >> __attribute__((format (printf, 2, 3), warn_unused_result)) >> >> here to catch any errors in the format string / arguments and to warn >> if TEST_RUN() isn't wrapped in an if() statement. > > Nice. Especially the "unused" check is valuable. This confused me for a moment. I assume you both mean the new function test__run, whose addition was cut from the citation: +int test__run(const char *location, const char *format, ...); For that one the numbers make sense and the idea is good. :) René
Am 02.07.24 um 17:13 schrieb phillip.wood123@gmail.com: > Hi René > > On 29/06/2024 16:43, René Scharfe wrote: >> The macro TEST only allows defining a test that consists of a >> single expression. Add the new macro, TEST_RUN, which provides a >> way to define unit tests that are made up of one or more >> statements. A test started with it implicitly ends when the next >> test is started or test_done() is called. >> >> TEST_RUN allows defining self-contained tests en bloc, a bit like >> test_expect_success does for regular tests. Unlike TEST it does >> not require defining wrapper functions for test statements. > > There are pros and cons to not requiring one function per test. It > can be a pain to have to write separate functions but it keeps each > test self contained which hopefully makes it harder to have > accidental dependencies between tests. Having separate functions for > each test makes it easy to initialize and free resources for every > test by writing a setup() function that initializes the resources, > calls the test function and then frees the resources. Right. We should use TEST and TEST_RUN when appropriate. > The changes in patch 6 to use TEST_RUN() mean that each test now has > more boilerplate to initialize and free the strbuf. This makes them more similar to strbuf usage in the wild. Using the API idiomatically just makes more sense to me. Not hiding initialization and release makes the tests visibly independent. This is not enforced by TEST_RUN, but made possible by it. > Having each test in its own function also makes main() shorter and > which means can quickly get an overview of all the test cases from > it. That's true, now you need to grep for TEST_RUN to get such an overview. On the other hand I find the start of the description in TEST invocations somewhat hard to locate, as they are not vertically aligned due to the preceding variable-length function name. Just saying.. >> +int test__run(const char *location, const char *format, ...) >> +{ >> + va_list ap; >> + char *desc; >> + >> + test__run_maybe_end(); >> + >> + va_start(ap, format); >> + desc = xstrvfmt(format, ap); > > This uses an strbuf under the hood. So far we've avoided doing that > as we want to be able to test the strbuf implementation with this > test framework. We don't need to support arbitrary length strings > here so we could use a fixed array and xsnprinf() instead. Fair point. xsnprinf() might be a bit too strict, as it doesn't handle short buffers gracefully. Perhaps that's OK; a developer getting hit by that could simply increase the buffer size. We could also let xstrvfmt() call vsnprintf(3) directly. The code duplication would be a bit grating, but perhaps there's some good base function hidden in there somewhere. > Looking ahead the plan seems to be to change most instances of TEST() > to TEST_RUN(). If we are going to go that way perhaps we should steal > TEST() for this macro and rename the existing TEST() macro. Not my plan, at least -- I'm content with just having the *ability* to keep all parts of a test together. If we wanted to do that, though, then TEST_RUN would have to be complemented with a blessed way to check ctx.result in order to handle the t_static_init test of t-strbuf, which I mentioned in my earlier reply to Josh. Err, no, we can simply check the check_* results, like check_strvec_loc does: diff --git a/t/unit-tests/t-strbuf.c b/t/unit-tests/t-strbuf.c index c8e39ddda7..c765fab53a 100644 --- a/t/unit-tests/t-strbuf.c +++ b/t/unit-tests/t-strbuf.c @@ -19,15 +19,6 @@ static int assert_sane_strbuf(struct strbuf *buf) return check_uint(buf->len, <, buf->alloc); } -static void t_static_init(void) -{ - struct strbuf buf = STRBUF_INIT; - - check_uint(buf.len, ==, 0); - check_uint(buf.alloc, ==, 0); - check_char(buf.buf[0], ==, '\0'); -} - static void t_dynamic_init(void) { struct strbuf buf; @@ -85,8 +76,14 @@ static void t_release(struct strbuf *sb) int cmd_main(int argc, const char **argv) { - if (!TEST(t_static_init(), "static initialization works")) - test_skip_all("STRBUF_INIT is broken"); + if (TEST_RUN("static initialization works")) { + struct strbuf buf = STRBUF_INIT; + if (!check_uint(buf.len, ==, 0) || + !check_uint(buf.alloc, ==, 0) || + !check_char(buf.buf[0], ==, '\0')) + test_skip_all("STRBUF_INIT is broken"); + } + TEST(t_dynamic_init(), "dynamic initialization works"); if (TEST_RUN("strbuf_addch adds char")) { > I'm not very enthusiastic about requiring the test author to wrap > TEST_RUN() in an if() statement rather than just doing that for them. > It makes it explicit but from the test author's point of view it just > feels like pointless boilerplate. Hmm. We can add more magic, but I suspect that it might confuse developers and editors. René
Hi René On 02/07/2024 21:55, René Scharfe wrote: > Am 02.07.24 um 17:13 schrieb phillip.wood123@gmail.com: >> On 29/06/2024 16:43, René Scharfe wrote: >>> The macro TEST only allows defining a test that consists of a >>> single expression. Add the new macro, TEST_RUN, which provides a >>> way to define unit tests that are made up of one or more >>> statements. A test started with it implicitly ends when the next >>> test is started or test_done() is called. >>> >>> TEST_RUN allows defining self-contained tests en bloc, a bit like >>> test_expect_success does for regular tests. Unlike TEST it does >>> not require defining wrapper functions for test statements. >> >> There are pros and cons to not requiring one function per test. It >> can be a pain to have to write separate functions but it keeps each >> test self contained which hopefully makes it harder to have >> accidental dependencies between tests. Having separate functions for >> each test makes it easy to initialize and free resources for every >> test by writing a setup() function that initializes the resources, >> calls the test function and then frees the resources. > > Right. We should use TEST and TEST_RUN when appropriate. > >> The changes in patch 6 to use TEST_RUN() mean that each test now has >> more boilerplate to initialize and free the strbuf. > This makes them more similar to strbuf usage in the wild. Using > the API idiomatically just makes more sense to me. I see what you mean. I think it only looks idiomatic if you're already familiar with the api though as the test bodies call wrappers rather than using the strbuf api directly. I think that reduces its value as an example of idomatic usage for someone who is not familiar with the strbuf api. > Not hiding > initialization and release makes the tests visibly independent. > This is not enforced by TEST_RUN, but made possible by it. > >> Having each test in its own function also makes main() shorter and >> which means can quickly get an overview of all the test cases from >> it. > > That's true, now you need to grep for TEST_RUN to get such an > overview. > > On the other hand I find the start of the description in TEST > invocations somewhat hard to locate, as they are not vertically > aligned due to the preceding variable-length function name. Just > saying.. Yes I really wanted the first argument of TEST to be the description but that isn't easy to do while supporting printf style format strings. >>> +int test__run(const char *location, const char *format, ...) >>> +{ >>> + va_list ap; >>> + char *desc; >>> + >>> + test__run_maybe_end(); >>> + >>> + va_start(ap, format); >>> + desc = xstrvfmt(format, ap); >> >> This uses an strbuf under the hood. So far we've avoided doing that >> as we want to be able to test the strbuf implementation with this >> test framework. We don't need to support arbitrary length strings >> here so we could use a fixed array and xsnprinf() instead. > > Fair point. xsnprinf() might be a bit too strict, as it doesn't > handle short buffers gracefully. Perhaps that's OK; a developer > getting hit by that could simply increase the buffer size. I think so. > We could also let xstrvfmt() call vsnprintf(3) directly. The code > duplication would be a bit grating, but perhaps there's some good > base function hidden in there somewhere. Oh, interesting - maybe something like char* xstrvfmt(const char *fmt, ...) { va_list ap, aq; va_start(ap, fmt); va_copy(aq, ap); len = vnsprintf(NULL, 0, fmt, ap); if (len < 0) BUG(...) buf = xmalloc(len + 1); if (vnsprintf(buf, len + 1, fmt, aq) != len) BUG(...) va_end(aq); va_end(ap); return buf; } >> Looking ahead the plan seems to be to change most instances of TEST() >> to TEST_RUN(). If we are going to go that way perhaps we should steal >> TEST() for this macro and rename the existing TEST() macro. > > Not my plan, at least -- I'm content with just having the *ability* > to keep all parts of a test together. That sounds sensible to me > + if (TEST_RUN("static initialization works")) { > + struct strbuf buf = STRBUF_INIT; > + if (!check_uint(buf.len, ==, 0) || > + !check_uint(buf.alloc, ==, 0) || > + !check_char(buf.buf[0], ==, '\0')) > + test_skip_all("STRBUF_INIT is broken"); > + } that's a nice use of test_skip_all() >> I'm not very enthusiastic about requiring the test author to wrap >> TEST_RUN() in an if() statement rather than just doing that for them. >> It makes it explicit but from the test author's point of view it just >> feels like pointless boilerplate. > > Hmm. We can add more magic, but I suspect that it might confuse > developers and editors. To me its confusing to have to wrap TEST_RUN() in an if() statement until one realizes that the test might be skipped. If we document that the test body should be enclosed in braces I don't think it should confuse developers or editors and will keep the tests a bit cleaner. Best Wishes Phillip
Am 05.07.24 um 11:42 schrieb phillip.wood123@gmail.com: > On 02/07/2024 21:55, René Scharfe wrote: >> Am 02.07.24 um 17:13 schrieb phillip.wood123@gmail.com: >> >>> The changes in patch 6 to use TEST_RUN() mean that each test now has >>> more boilerplate to initialize and free the strbuf. >> >> This makes them more similar to strbuf usage in the wild. Using >> the API idiomatically just makes more sense to me. > > I see what you mean. I think it only looks idiomatic if you're > already familiar with the api though as the test bodies call wrappers > rather than using the strbuf api directly. I think that reduces its > value as an example of idomatic usage for someone who is not familiar > with the strbuf api. In early versions I used the original names by adding these just before main(): #define strbuf_addch(x, y) t_addch(x, y) #define strbuf_addstr(x, y) t_addstr(x, y) #define strbuf_release(x) t_release(x) This allowed normal looking code to be used in tests, with checks injected behind the scenes. Rejected it for v1 because it offers no structural improvements, just optics. It does allow to forget the checked versions when writing tests, though, so perhaps it's still worth doing. >> We could also let xstrvfmt() call vsnprintf(3) directly. The code >> duplication would be a bit grating, but perhaps there's some good >> base function hidden in there somewhere. > > Oh, interesting - maybe something like > > char* xstrvfmt(const char *fmt, ...) > { > va_list ap, aq; > > va_start(ap, fmt); > va_copy(aq, ap); > len = vnsprintf(NULL, 0, fmt, ap); > if (len < 0) > BUG(...) > buf = xmalloc(len + 1); > if (vnsprintf(buf, len + 1, fmt, aq) != len) > BUG(...) > va_end(aq); > va_end(ap); > > return buf; > } Yes. Though the current version allocates 65 bytes in the first try and only needs to call vnsprintf(3) once if the output fits in. No longer doing that might affect the performance of some callers in a noticeable way. >>> I'm not very enthusiastic about requiring the test author to wrap >>> TEST_RUN() in an if() statement rather than just doing that for them. >>> It makes it explicit but from the test author's point of view it just >>> feels like pointless boilerplate. >> >> Hmm. We can add more magic, but I suspect that it might confuse >> developers and editors. > > To me its confusing to have to wrap TEST_RUN() in an if() statement > until one realizes that the test might be skipped. If we document > that the test body should be enclosed in braces I don't think it > should confuse developers or editors and will keep the tests a bit > cleaner. You don't need braces in either case. I.e. something like if (TEST_RUN("foo")) foo(); works fine. And #define IF_TEST_RUN(...) if (TEST_RUN(__VA_ARGS__)) IF_TEST_RUN("foo") foo(); works fine as well. The confusion that I'm afraid of is that we'd effectively invent a new control flow keyword here, which is unusual. There are precedents, though: foreach macros like for_each_string_list_item. We tell clang-format about them using its option ForEachMacros. I see it also has an option IfMacros since version 13 (unused by us so far). Hmm. René
Am 05.07.24 um 20:01 schrieb René Scharfe: > Am 05.07.24 um 11:42 schrieb phillip.wood123@gmail.com: >> On 02/07/2024 21:55, René Scharfe wrote: >>> Am 02.07.24 um 17:13 schrieb phillip.wood123@gmail.com: >>> >>>> I'm not very enthusiastic about requiring the test author to wrap >>>> TEST_RUN() in an if() statement rather than just doing that for them. >>>> It makes it explicit but from the test author's point of view it just >>>> feels like pointless boilerplate. >>> >>> Hmm. We can add more magic, but I suspect that it might confuse >>> developers and editors. >> >> To me its confusing to have to wrap TEST_RUN() in an if() statement >> until one realizes that the test might be skipped. If we document >> that the test body should be enclosed in braces I don't think it >> should confuse developers or editors and will keep the tests a bit >> cleaner. > > You don't need braces in either case. I.e. something like > > if (TEST_RUN("foo")) > foo(); > > works fine. And > > #define IF_TEST_RUN(...) if (TEST_RUN(__VA_ARGS__)) > IF_TEST_RUN("foo") > foo(); > > works fine as well. > > The confusion that I'm afraid of is that we'd effectively invent a new > control flow keyword here, which is unusual. There are precedents, > though: foreach macros like for_each_string_list_item. We tell > clang-format about them using its option ForEachMacros. I see it also > has an option IfMacros since version 13 (unused by us so far). Hmm. Hmm, again. I can see the appeal. How to call it? Given that the foreach macros have lower-case names, perhaps to indicate that they act as control flow statements, not function-like macro calls, would we want lower case here as well? #define test(...) if (TEST_RUN(__VA_ARGS__)) test ("passing test") check(1); test ("split single item") { struct strvec vec = STRVEC_INIT; strvec_split(&vec, "foo"); check_strvec(&vec, "foo", NULL); strvec_clear(&vec); } Can't get much cleaner than that. Requires learning that this macro is not function-like, but it's probably not too much to ask. René
Hi René On 05/07/2024 19:01, René Scharfe wrote: > Am 05.07.24 um 11:42 schrieb phillip.wood123@gmail.com: >> On 02/07/2024 21:55, René Scharfe wrote: >>> Am 02.07.24 um 17:13 schrieb phillip.wood123@gmail.com: >>> >>>> The changes in patch 6 to use TEST_RUN() mean that each test now has >>>> more boilerplate to initialize and free the strbuf. >>> >>> This makes them more similar to strbuf usage in the wild. Using >>> the API idiomatically just makes more sense to me. >> >> I see what you mean. I think it only looks idiomatic if you're >> already familiar with the api though as the test bodies call wrappers >> rather than using the strbuf api directly. I think that reduces its >> value as an example of idomatic usage for someone who is not familiar >> with the strbuf api. > > In early versions I used the original names by adding these just before > main(): > > #define strbuf_addch(x, y) t_addch(x, y) > #define strbuf_addstr(x, y) t_addstr(x, y) > #define strbuf_release(x) t_release(x) > > This allowed normal looking code to be used in tests, with checks > injected behind the scenes. Rejected it for v1 because it offers no > structural improvements, just optics. It does allow to forget the > checked versions when writing tests, though, so perhaps it's still > worth doing. It also obscures what the test is doing though. It's nice if unit tests show how to use an API but I don't think we should let that be our overriding concern. >>> We could also let xstrvfmt() call vsnprintf(3) directly. The code >>> duplication would be a bit grating, but perhaps there's some good >>> base function hidden in there somewhere. >> >> Oh, interesting - maybe something like >> >> char* xstrvfmt(const char *fmt, ...) >> { >> va_list ap, aq; >> >> va_start(ap, fmt); >> va_copy(aq, ap); >> len = vnsprintf(NULL, 0, fmt, ap); >> if (len < 0) >> BUG(...) >> buf = xmalloc(len + 1); >> if (vnsprintf(buf, len + 1, fmt, aq) != len) >> BUG(...) >> va_end(aq); >> va_end(ap); >> >> return buf; >> } > > Yes. Though the current version allocates 65 bytes in the first try > and only needs to call vnsprintf(3) once if the output fits in. No > longer doing that might affect the performance of some callers in a > noticeable way. Good point >>>> I'm not very enthusiastic about requiring the test author to wrap >>>> TEST_RUN() in an if() statement rather than just doing that for them. >>>> It makes it explicit but from the test author's point of view it just >>>> feels like pointless boilerplate. >>> >>> Hmm. We can add more magic, but I suspect that it might confuse >>> developers and editors. >> >> To me its confusing to have to wrap TEST_RUN() in an if() statement >> until one realizes that the test might be skipped. If we document >> that the test body should be enclosed in braces I don't think it >> should confuse developers or editors and will keep the tests a bit >> cleaner. > > You don't need braces in either case. I.e. something like > > if (TEST_RUN("foo")) > foo(); > > works fine. And > > #define IF_TEST_RUN(...) if (TEST_RUN(__VA_ARGS__)) > IF_TEST_RUN("foo") > foo(); > > works fine as well. Indeed. The reason I suggested requiring braces was to help editors indent the code correctly and because it gives a nice visual cue to see what is in each test. Best Wishes Phillip > The confusion that I'm afraid of is that we'd effectively invent a new > control flow keyword here, which is unusual. There are precedents, > though: foreach macros like for_each_string_list_item. We tell > clang-format about them using its option ForEachMacros. I see it also > has an option IfMacros since version 13 (unused by us so far). Hmm. > > René
Hi René On 07/07/2024 08:20, René Scharfe wrote: > Hmm, again. I can see the appeal. How to call it? Given that the > foreach macros have lower-case names, perhaps to indicate that they act > as control flow statements, not function-like macro calls, would we want > lower case here as well? I'd automatically assumed we'd want an uppercase name to signal that it was a pre-processor macro but I've not really spent any time thinking about it. > #define test(...) if (TEST_RUN(__VA_ARGS__)) > > test ("passing test") > check(1); > > test ("split single item") { > struct strvec vec = STRVEC_INIT; > strvec_split(&vec, "foo"); > check_strvec(&vec, "foo", NULL); > strvec_clear(&vec); > } > > Can't get much cleaner than that. Yes, I like it > Requires learning that this macro is > not function-like, but it's probably not too much to ask. For someone new to the project it should hopefully be pretty clear how to use it from the existing tests once we have a few more test files that use it. Maybe an uppercase name would signal that it is special? Best Wishes Phillip
phillip.wood123@gmail.com writes: > Hi René > > On 07/07/2024 08:20, René Scharfe wrote: >> Hmm, again. I can see the appeal. How to call it? Given that the >> foreach macros have lower-case names, perhaps to indicate that they act >> as control flow statements, not function-like macro calls, would we want >> lower case here as well? > > I'd automatically assumed we'd want an uppercase name to signal that > it was a pre-processor macro but I've not really spent any time > thinking about it. > >> #define test(...) if (TEST_RUN(__VA_ARGS__)) >> test ("passing test") >> check(1); >> test ("split single item") { >> struct strvec vec = STRVEC_INIT; >> strvec_split(&vec, "foo"); >> check_strvec(&vec, "foo", NULL); >> strvec_clear(&vec); >> } >> Can't get much cleaner than that. > > Yes, I like it Yeah, if you squint your eyes hard, it starts to look a bit like test_expect_success we use in the test suite ;-) Isn't this introducing a new control structure to the language? A macro that is a conditional switch (aka "if"-like statement), having "if" in the name somewhere, and a macro that wrapts a loop around a block (aka "for/while" like statement), having "for" in the name somewhere, might be less confusing for the uninitiated. > For someone new to the project it should hopefully be pretty clear how > to use it from the existing tests once we have a few more test files > that use it. Maybe an uppercase name would signal that it is special? As to the cases, I personally prefer lowercase names whose semantics is very clear, just like e.g., kh_foreach() makes it clear with "foreach" that it iterates over the set. But the above "test" may not qualify---it is not making it sufficiently clear what control structure it is creating with its name. Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> I'd automatically assumed we'd want an uppercase name to signal that >> it was a pre-processor macro but I've not really spent any time >> thinking about it. >> >>> #define test(...) if (TEST_RUN(__VA_ARGS__)) >>> test ("passing test") >>> check(1); > ... > Isn't this introducing a new control structure to the language? > > A macro that is a conditional switch (aka "if"-like statement), > having "if" in the name somewhere, and a macro that wrapts a loop > around a block (aka "for/while" like statement), having "for" in the > name somewhere, might be less confusing for the uninitiated. So, perhaps test_if_XXXXXX() but it is not quite clear to me when TEST_RUN() wants to return true, so I cannot come up with an appropriate value to fill the XXXXXX part. If this is about honoring GIT_SKIP_TESTS or something similar, then I may suggest test_if_enabled(), but that does not seem like it. So...
On 11/07/2024 16:34, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > So, perhaps test_if_XXXXXX() but it is not quite clear to me when > TEST_RUN() wants to return true, so I cannot come up with an > appropriate value to fill the XXXXXX part. If this is about > honoring GIT_SKIP_TESTS or something similar, then I may suggest > test_if_enabled(), but that does not seem like it. So... TEST_RUN() returns true if the test has not been skipped i.e. the test should be run. At the moment the only way to skip a test is to call test_skip_all() in a previous test. In the future I expect we'll add something like the "--run" option we have for the integration tests. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > On 11/07/2024 16:34, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> So, perhaps test_if_XXXXXX() but it is not quite clear to me when >> TEST_RUN() wants to return true, so I cannot come up with an >> appropriate value to fill the XXXXXX part. If this is about >> honoring GIT_SKIP_TESTS or something similar, then I may suggest >> test_if_enabled(), but that does not seem like it. So... > > TEST_RUN() returns true if the test has not been skipped i.e. the test > should be run. At the moment the only way to skip a test is to call > test_skip_all() in a previous test. In the future I expect we'll add > something like the "--run" option we have for the integration tests. Thanks, so test_if_enabled() is not all that off the mark after all.
diff --git a/t/helper/test-example-tap.c b/t/helper/test-example-tap.c index d072ad559f..7b02177a9f 100644 --- a/t/helper/test-example-tap.c +++ b/t/helper/test-example-tap.c @@ -92,5 +92,38 @@ int cmd__example_tap(int argc, const char **argv) test_res = TEST(t_empty(), "test with no checks"); TEST(check_int(test_res, ==, 0), "test with no checks returns 0"); + if (TEST_RUN("TEST_RUN passing test")) + check_int(1, ==, 1); + if (TEST_RUN("TEST_RUN failing test")) + check_int(1, ==, 2); + if (TEST_RUN("TEST_RUN passing TEST_TODO()")) + TEST_TODO(check(0)); + if (TEST_RUN("TEST_RUN failing TEST_TODO()")) + TEST_TODO(check(1)); + if (TEST_RUN("TEST_RUN test_skip()")) { + check(0); + test_skip("missing prerequisite"); + check(1); + } + if (TEST_RUN("TEST_RUN test_skip() inside TEST_TODO()")) + TEST_TODO((test_skip("missing prerequisite"), 1)); + if (TEST_RUN("TEST_RUN TEST_TODO() after failing check")) { + check(0); + TEST_TODO(check(0)); + } + if (TEST_RUN("TEST_RUN failing check after TEST_TODO()")) { + check(1); + TEST_TODO(check(0)); + check(0); + } + if (TEST_RUN("TEST_RUN messages from failing string and char comparison")) { + check_str("\thello\\", "there\"\n"); + check_str("NULL", NULL); + check_char('a', ==, '\n'); + check_char('\\', ==, '\''); + } + if (TEST_RUN("TEST_RUN test with no checks")) + ; /* nothing */ + return test_done(); } diff --git a/t/t0080/expect b/t/t0080/expect index 0cfa0dc6d8..92526e1b06 100644 --- a/t/t0080/expect +++ b/t/t0080/expect @@ -40,4 +40,37 @@ not ok 17 - messages from failing string and char comparison # BUG: test has no checks at t/helper/test-example-tap.c:92 not ok 18 - test with no checks ok 19 - test with no checks returns 0 -1..19 +ok 20 - TEST_RUN passing test +# check "1 == 2" failed at t/helper/test-example-tap.c:98 +# left: 1 +# right: 2 +not ok 21 - TEST_RUN failing test +not ok 22 - TEST_RUN passing TEST_TODO() # TODO +# todo check 'check(1)' succeeded at t/helper/test-example-tap.c:102 +not ok 23 - TEST_RUN failing TEST_TODO() +# check "0" failed at t/helper/test-example-tap.c:104 +# skipping test - missing prerequisite +# skipping check '1' at t/helper/test-example-tap.c:106 +ok 24 - TEST_RUN test_skip() # SKIP +# skipping test - missing prerequisite +ok 25 - TEST_RUN test_skip() inside TEST_TODO() # SKIP +# check "0" failed at t/helper/test-example-tap.c:111 +not ok 26 - TEST_RUN TEST_TODO() after failing check +# check "0" failed at t/helper/test-example-tap.c:117 +not ok 27 - TEST_RUN failing check after TEST_TODO() +# check "!strcmp("\thello\\", "there\"\n")" failed at t/helper/test-example-tap.c:120 +# left: "\011hello\\" +# right: "there\"\012" +# check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:121 +# left: "NULL" +# right: NULL +# check "'a' == '\n'" failed at t/helper/test-example-tap.c:122 +# left: 'a' +# right: '\012' +# check "'\\' == '\''" failed at t/helper/test-example-tap.c:123 +# left: '\\' +# right: '\'' +not ok 28 - TEST_RUN messages from failing string and char comparison +# BUG: test has no checks at t/helper/test-example-tap.c:125 +not ok 29 - TEST_RUN test with no checks +1..29 diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c index 3c513ce59a..fc50755fae 100644 --- a/t/unit-tests/test-lib.c +++ b/t/unit-tests/test-lib.c @@ -16,6 +16,8 @@ static struct { unsigned running :1; unsigned skip_all :1; unsigned todo :1; + char *desc; + char *location; } ctx = { .lazy_plan = 1, .result = RESULT_NONE, @@ -123,9 +125,45 @@ void test_plan(int count) ctx.lazy_plan = 0; } -int test_done(void) +static void test__run_maybe_end(void) { + if (ctx.running) { + assert(ctx.location); + assert(ctx.desc); + test__run_end(0, ctx.location, "%s", ctx.desc); + FREE_AND_NULL(ctx.location); + FREE_AND_NULL(ctx.desc); + } assert(!ctx.running); + assert(!ctx.location); + assert(!ctx.desc); +} + +int test__run(const char *location, const char *format, ...) +{ + va_list ap; + char *desc; + + test__run_maybe_end(); + + va_start(ap, format); + desc = xstrvfmt(format, ap); + va_end(ap); + + if (test__run_begin()) { + test__run_end(1, location, "%s", desc); + free(desc); + return 0; + } else { + ctx.location = xstrdup(location); + ctx.desc = desc; + return 1; + } +} + +int test_done(void) +{ + test__run_maybe_end(); if (ctx.lazy_plan) test_plan(ctx.count); @@ -169,7 +207,7 @@ void test_skip_all(const char *format, ...) int test__run_begin(void) { - assert(!ctx.running); + test__run_maybe_end(); ctx.count++; ctx.result = RESULT_NONE; diff --git a/t/unit-tests/test-lib.h b/t/unit-tests/test-lib.h index 2de6d715d5..6df40e3b12 100644 --- a/t/unit-tests/test-lib.h +++ b/t/unit-tests/test-lib.h @@ -21,6 +21,13 @@ */ void test_plan(int count); +/* + * Start a test, returns 1 if the test was actually started or 0 if it + * was skipped. The test ends when the next test starts or test_done() + * is called. + */ +#define TEST_RUN(...) test__run(TEST_LOCATION(), __VA_ARGS__) + /* * test_done() must be called at the end of main(). It will print the * plan if plan() was not called at the beginning of the test program @@ -156,6 +163,7 @@ extern union test__tmp test__tmp[2]; int test__run_begin(void); __attribute__((format (printf, 3, 4))) int test__run_end(int, const char *, const char *, ...); +int test__run(const char *location, const char *format, ...); void test__todo_begin(void); int test__todo_end(const char *, const char *, int);
The macro TEST only allows defining a test that consists of a single expression. Add the new macro, TEST_RUN, which provides a way to define unit tests that are made up of one or more statements. A test started with it implicitly ends when the next test is started or test_done() is called. TEST_RUN allows defining self-contained tests en bloc, a bit like test_expect_success does for regular tests. Unlike TEST it does not require defining wrapper functions for test statements. No public method is provided for ending a test explicitly, yet; let's see if we'll ever need one. Signed-off-by: René Scharfe <l.s.r@web.de> --- t/helper/test-example-tap.c | 33 +++++++++++++++++++++++++++++ t/t0080/expect | 35 ++++++++++++++++++++++++++++++- t/unit-tests/test-lib.c | 42 +++++++++++++++++++++++++++++++++++-- t/unit-tests/test-lib.h | 8 +++++++ 4 files changed, 115 insertions(+), 3 deletions(-) -- 2.45.2