Re: [PATCH v3 3/7] unit-tests: add for_test

[Kyle Lippincott <spectral@google.com>]

On Wed, Jul 24, 2024 at 7:53 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, for_test, which provides a way to define
> unit tests that are made up of one or more statements.

Perhaps obvious to you and others, but I'm at a loss; can you
elaborate on why this is a significant problem? Why is having the
tests in a separate function a significant downside, and what are the
benefits of this new macro that counteract the "magic" here? I'm not
seeing it.

Macros like this require additional cognitive overhead to understand;
I can't read `for_test ("for_test passing test") check_int(1, ==, 1);`
and understand what's going on without going and reading the comment
above `for_test`. Unlike `TEST(<func>, "<description>")`, I can't even
_guess_ what it's doing. This macro is project-specific, and this
knowledge has to be attained by everyone wishing to even read this
code.

I personally consider the patches that use this to be regressions in
my ability to read and understand the tests. As an example, one of the
diff hunks in the strbuf patch (patch #6 in the series) does this:
-       TEST(setup_populated(t_addch, "initial value", "a"),
-            "strbuf_addch appends to initial value");
+       for_test ("strbuf_addch appends to initial value") {
+               struct strbuf sb = STRBUF_INIT;
+               t_addstr(&sb, "initial value");
+               t_addch(&sb, 'a');
+               t_release(&sb);
+       }

But the main simplification in that patch (not using
`setup_populated`) can be achieved without `for_test`:

+ static void t_addch_appends(void)
+ {
+     struct strbuf sb = STRBUF_INIT;
+     t_addstr(&sb, "initial value");
+     t_addch(&sb, 'a');
+     t_release(&sb);
+ }

-       TEST(setup_populated(t_addch, "initial value", "a"),
-            "strbuf_addch appends to initial value");
+       TEST(t_addch_appends(), "strbuf_addch appends to initial value");

It seems to me that all `for_test` is getting us there is an inlining
of the test logic, which seems like it's optimizing for vertical space
in the file. Maybe it's because I'm coming from a C++ environment at
$JOB that's using Google's gunit and gmock frameworks, where every
test is in its own function and we usually don't even write the main
function ourselves, but I have a preference for the separate
functions.

Maybe I'm in the minority, so only consider this at somewhere like a
-0.5 on this series (fine with deferring to a reviewer who is
significantly in favor of it, but if there aren't any, I'd lean
towards not landing this).