| Message ID | da902de9-288e-4783-8b4b-a2a968d1e1f8@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add and use if_test to simplify tests | expand |
On Tue, Jul 30, 2024 at 7:08 AM René Scharfe <l.s.r@web.de> 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@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); > /* > * 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
Am 01.08.24 um 00:04 schrieb Kyle Lippincott: > > 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? The idea is that the unit test framework may allow to skip individual tests in the future; see https://lore.kernel.org/git/c2de7f4e-e5dc-4324-8238-1d06795a2107@gmail.com/ René
On Thu, Aug 1, 2024 at 12:32 AM René Scharfe <l.s.r@web.de> wrote: > > Am 01.08.24 um 00:04 schrieb Kyle Lippincott: > > > > 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? > > The idea is that the unit test framework may allow to skip individual > tests in the future; see > https://lore.kernel.org/git/c2de7f4e-e5dc-4324-8238-1d06795a2107@gmail.com/ I see. This functionality isn't currently available, but we may want it in the future. If that future ever happens, then we may be able to avoid some cleanup if we keep the setup the way it is, where `test__run_start` returns whether or not the test actually started. This future `--run` flag for the unit tests would be based on either (a) position in the file, like the integration tests, or (b) some substring of the description. Since the tests don't have a name, there's no ability for `--run` to operate based on that. Ok. I still don't like the name, but I might be in the minority on this. Yes, there's a control structure in use to make this work with the syntax you desire, but unlike with `for_test`, there's many fewer edge cases added because of it: `break` and `continue` won't do anything with `if_test`. I suppose that we would still need to be concerned about the possibility of someone writing this: if (condition) if_test("...") check_int(1, ==, 1); else ... I'm also still not personally sold on the value here. Single-expression tests can be written with `TEST(check_int(1, ==, 1), "equality")`, and having a single large function with all multi-statement tests in it is harder to follow, in my opinion, than the alternative where the tests are in a separate function. I can only really think of the case where the tests have inter-dependencies (ex: test 2 sets some state that test 5 relies on) for wanting them all in the same function, because then that dependency becomes a bit more obvious. However, getting that inter-test dependency to _not_ happen actually seems like a worthwhile goal (except for cases like strbuf, where if strbuf is inherently broken, we need to check that first and bail out). There's increased risk that someone will write a test like this (purely hypothetical example of some state being carried from one test to another; yes, you can accomplish the same terribleness with global variables, but more people have immediate negative reactions about globals): int cmd_main(int argc UNUSED, const char **argv UNUSED) { char tmp_path[PATH_MAX]; if_test("alpha") { /* perform basic functionality checks, like strbuf does */ } if_test("beta") { /* or some other "expensive" initialization we don't want to do if everything is broken */ sprintf("/tmp/tmp.%d", rand()); do_something(tmp_path, ...); } if_test("gamma") { /* * if beta didn't run, or if tests get reordered such that gamma executes before beta, * things are going to go very poorly here */ do_something_else(tmp_path, ...); } return tests_done(); } That said, I'm still not opposed enough to the idea to veto (plus I don't have anywhere near that kind of power, and shouldn't :)). I just don't think I'd ever use it.
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)) + /* * 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 *, ...);
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(-) -- 2.46.0