On Thu, Aug 1, 2024 at 12:32 AM René Scharfe <l.s.r@xxxxxx> 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@xxxxxxxxx/ 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.
