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

[Kyle Lippincott <spectral@google.com>]

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.