On Tue, Jul 30, 2024 at 7:08 AM René Scharfe <l.s.r@xxxxxx> wrote:
>
> The macro TEST only allows defining a test that consists of a single
> expression. Add a new macro, if_test, which provides a way to define
> unit tests that are made up of one or more statements.
>
> if_test allows defining self-contained tests en bloc, a bit like
> test_expect_success does for regular tests. It acts like a conditional;
> the test body is executed if test_skip_all() had not been called before.
>
> Signed-off-by: René Scharfe <l.s.r@xxxxxx>
> ---
> .clang-format | 5 +++++
> t/helper/test-example-tap.c | 33 +++++++++++++++++++++++++++++++++
> t/t0080-unit-test-output.sh | 35 ++++++++++++++++++++++++++++++++++-
> t/unit-tests/test-lib.c | 29 +++++++++++++++++++++++++++++
> t/unit-tests/test-lib.h | 20 ++++++++++++++++++++
> 5 files changed, 121 insertions(+), 1 deletion(-)
>
> diff --git a/.clang-format b/.clang-format
> index 6408251577..4c6d317508 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -169,6 +169,11 @@ ForEachMacros:
> - 'strmap_for_each_entry'
> - 'strset_for_each_entry'
>
> +# A list of macros that should be interpreted as conditionals instead of as
> +# function calls.
> +IfMacros:
> + - 'if_test'
> +
> # The maximum number of consecutive empty lines to keep.
> MaxEmptyLinesToKeep: 1
>
> diff --git a/t/helper/test-example-tap.c b/t/helper/test-example-tap.c
> index 79c12b01cd..914af88e0a 100644
> --- a/t/helper/test-example-tap.c
> +++ b/t/helper/test-example-tap.c
> @@ -94,5 +94,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 ("if_test passing test")
> + check_int(1, ==, 1);
> + if_test ("if_test failing test")
> + check_int(1, ==, 2);
> + if_test ("if_test passing TEST_TODO()")
> + TEST_TODO(check(0));
> + if_test ("if_test failing TEST_TODO()")
> + TEST_TODO(check(1));
> + if_test ("if_test test_skip()") {
> + check(0);
> + test_skip("missing prerequisite");
> + check(1);
> + }
> + if_test ("if_test test_skip() inside TEST_TODO()")
> + TEST_TODO((test_skip("missing prerequisite"), 1));
> + if_test ("if_test TEST_TODO() after failing check") {
> + check(0);
> + TEST_TODO(check(0));
> + }
> + if_test ("if_test failing check after TEST_TODO()") {
> + check(1);
> + TEST_TODO(check(0));
> + check(0);
> + }
> + if_test ("if_test messages from failing string and char comparison") {
> + check_str("\thello\\", "there\"\n");
> + check_str("NULL", NULL);
> + check_char('a', ==, '\n');
> + check_char('\\', ==, '\'');
> + }
> + if_test ("if_test test with no checks")
> + ; /* nothing */
> +
> return test_done();
> }
> diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
> index fe221f3bdb..3c369c88e2 100755
> --- a/t/t0080-unit-test-output.sh
> +++ b/t/t0080-unit-test-output.sh
> @@ -50,7 +50,40 @@ test_expect_success 'TAP output from unit tests' - <<\EOT
> # BUG: test has no checks at t/helper/test-example-tap.c:94
> not ok 18 - test with no checks
> ok 19 - test with no checks returns 0
> - 1..19
> + ok 20 - if_test passing test
> + # check "1 == 2" failed at t/helper/test-example-tap.c:100
> + # left: 1
> + # right: 2
> + not ok 21 - if_test failing test
> + not ok 22 - if_test passing TEST_TODO() # TODO
> + # todo check 'check(1)' succeeded at t/helper/test-example-tap.c:104
> + not ok 23 - if_test failing TEST_TODO()
> + # check "0" failed at t/helper/test-example-tap.c:106
> + # skipping test - missing prerequisite
> + # skipping check '1' at t/helper/test-example-tap.c:108
> + ok 24 - if_test test_skip() # SKIP
> + # skipping test - missing prerequisite
> + ok 25 - if_test test_skip() inside TEST_TODO() # SKIP
> + # check "0" failed at t/helper/test-example-tap.c:113
> + not ok 26 - if_test TEST_TODO() after failing check
> + # check "0" failed at t/helper/test-example-tap.c:119
> + not ok 27 - if_test failing check after TEST_TODO()
> + # check "!strcmp("\thello\\\\", "there\"\n")" failed at t/helper/test-example-tap.c:122
> + # left: "\011hello\\\\"
> + # right: "there\"\012"
> + # check "!strcmp("NULL", NULL)" failed at t/helper/test-example-tap.c:123
> + # left: "NULL"
> + # right: NULL
> + # check "'a' == '\n'" failed at t/helper/test-example-tap.c:124
> + # left: 'a'
> + # right: '\012'
> + # check "'\\\\' == '\\''" failed at t/helper/test-example-tap.c:125
> + # left: '\\\\'
> + # right: '\\''
> + not ok 28 - if_test messages from failing string and char comparison
> + # BUG: test has no checks at t/helper/test-example-tap.c:127
> + not ok 29 - if_test test with no checks
> + 1..29
> EOF
>
> ! test-tool example-tap >actual &&
> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
> index 989dc758e6..fa1f95965c 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 location[100];
> + char description[100];
> } ctx = {
> .lazy_plan = 1,
> .result = RESULT_NONE,
> @@ -125,6 +127,8 @@ void test_plan(int count)
>
> int test_done(void)
> {
> + if (ctx.running && ctx.location[0] && ctx.description[0])
> + test__run_end(1, ctx.location, "%s", ctx.description);
> assert(!ctx.running);
>
> if (ctx.lazy_plan)
> @@ -167,13 +171,38 @@ void test_skip_all(const char *format, ...)
> va_end(ap);
> }
>
> +void test__run_describe(const char *location, const char *format, ...)
> +{
> + va_list ap;
> + int len;
> +
> + assert(ctx.running);
> + assert(!ctx.location[0]);
> + assert(!ctx.description[0]);
> +
> + xsnprintf(ctx.location, sizeof(ctx.location), "%s",
> + make_relative(location));
> +
> + va_start(ap, format);
> + len = vsnprintf(ctx.description, sizeof(ctx.description), format, ap);
> + va_end(ap);
> + if (len < 0)
> + die("unable to format message: %s", format);
> + if (len >= sizeof(ctx.description))
> + BUG("ctx.description too small to format %s", format);
> +}
> +
> int test__run_begin(void)
> {
> + if (ctx.running && ctx.location[0] && ctx.description[0])
> + test__run_end(1, ctx.location, "%s", ctx.description);
> assert(!ctx.running);
>
> ctx.count++;
> ctx.result = RESULT_NONE;
> ctx.running = 1;
> + ctx.location[0] = '\0';
> + ctx.description[0] = '\0';
>
> return ctx.skip_all;
> }
> diff --git a/t/unit-tests/test-lib.h b/t/unit-tests/test-lib.h
> index 2de6d715d5..f15dceb29e 100644
> --- a/t/unit-tests/test-lib.h
> +++ b/t/unit-tests/test-lib.h
> @@ -14,6 +14,23 @@
> test__run_end(test__run_begin() ? 0 : (t, 1), \
> TEST_LOCATION(), __VA_ARGS__)
>
> +/*
> + * Run a test unless test_skip_all() has been called. Acts like a
> + * conditional; the test body is expected as a statement or block after
> + * the closing parenthesis. The description for each test should be
> + * unique. E.g.:
> + *
> + * if_test ("something else %d %d", arg1, arg2) {
> + * prepare();
> + * test_something_else(arg1, arg2);
> + * cleanup();
> + * }
> + */
> +#define if_test(...) \
> + if (test__run_begin() ? \
> + (test__run_end(0, TEST_LOCATION(), __VA_ARGS__), 0) : \
> + (test__run_describe(TEST_LOCATION(), __VA_ARGS__), 1))
> +
I personally prefer this implementation much more than the `for_test`
implementation, thanks. I think the extra functionality/usability from
having `test__run_end` automatically called is also very useful.
I'm still not wild about the name, and I'm still not convinced of the
readability improvements. I still want to read `if_test(...)` as
having a condition in the parenthesis. Tests "skipping everything"
seems like an internal implementation detail that there's disagreement
on whether to expose to the test author/reader or not; maybe we should
redesign if we don't migrate to clar or some other test framework. Do
we get significant benefits from a test function continuing to execute
after the "skip everything" flag has been set?
Specifically, the only current user of `test_skip_all` is t-strbuf.c.
Maybe we do this in t-strbuf.c, and remove the "skip everything"
logic? Below, `test_skipping_remainder` does most of the stuff that
`test_skip_all` does, except:
- automatically calls test_done()
- "poison"s some state so that we can't call `test__run_begin`
anymore, if test author forgets to `return` from this
int cmd_main(int argc, const char **argv)
{
if (!TEST(t_static_init(), "static initialization works"))
- test_skip_all("STRBUF_INIT is broken");
+ return test_skipping_remainder("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();
}
With that out of the way, I think that `test__run_begin()` always
returns true, and calling `test__run_end()` isn't necessary, so this
would be able to become:
#define if_test(...) \
if (test__run_describe(TEST_LOCATION(), __VA_ARGS__), 1) /*
automatically calls test__run_begin() */
At that point, the `if` is just used to allow this syntax:
if_test("example if_test") {
check_int(1, ==, 1);
check_int(1, !=, 2);
}
But it's equivalent, since we don't have any conditional that we're
actually checking, to:
{
if_test("example if_test"); /* note the semicolon; leaving it off
doesn't matter, as long as compiler doesn't complain about empty if
block */
check_int(1, ==, 1);
check_int(1, !=, 2);
}
Or even doing that without the {}. At that point, we can remove the
`if` aspect of the name, and get something like this, where you can
use {} blocks if needed or desired:
#define multiline_test(...) test__run_describe(TEST_LOCATION(), __VA_ARGS__)
multiline_test("equality"); /* semicolon required */
check_int(1, ==, 1);
check_int(2, ==, 2);
multiline_test("inequality");
check_int(1, !=, 2);
check_int(2, !=, 1);
> /*
> * Print a test plan, should be called before any tests. If the number
> * of tests is not known in advance test_done() will automatically
> @@ -153,6 +170,9 @@ union test__tmp {
>
> extern union test__tmp test__tmp[2];
>
> +__attribute__((format (printf, 2, 3)))
> +void test__run_describe(const char *, const char *, ...);
> +
> int test__run_begin(void);
> __attribute__((format (printf, 3, 4)))
> int test__run_end(int, const char *, const char *, ...);
> --
> 2.46.0
