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

> On Wed, Jul 24, 2024 at 7:53 AM René Scharfe <l.s.r@web.de> wrote:
>>
> 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).
>