Re: [PATCH v4 3/6] unit-tests: add if_test

From: Kyle Lippincott <spectral@google.com>
Date: Jul 31, 2024, 22:04 UTC
Message-ID: <CAO_smVh_2FEbTAqJDMZAKMsfTKrB=NH36G6jNmMTaEOtRaP2Pg@mail.gmail.com>
In-Reply-To: <da902de9-288e-4783-8b4b-a2a968d1e1f8@web.de>

On Tue, Jul 30, 2024 at 7:08 AM René Scharfe <l.s.r@web.de> wrote:

Show 209 quoted lines

>
> 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@web.de>
> ---
>  .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);

Show 15 quoted lines

>  /*
>   * 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