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

[Phillip Wood <phillip.wood123@gmail.com>]

On 24/07/2024 20:24, Kyle Lippincott wrote:
> 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.

As well as the obscure name and oddness of using "continue" to exit a 
test early I think having two ways of defining tests also adds to the 
cognitive overhead of writing tests as one now has to decide which to use.

As you say below it is not clear to me that the changes in the last 
three patches that start using for_test() in our existing tests are an 
improvement.

This will be the last comment from me for a while as I'm about to go off 
the list for a couple of weeks

Best Wishes

Phillip

> 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).
>