On Mon, Jul 22, 2024 at 12:36:57PM -0700, Junio C Hamano wrote: > Kyle Lippincott <spectral@xxxxxxxxxx> writes: > >> +#define for_test(...) \ > >> + for (int for_test_running_ = test__run_begin() ? \ > >> + (test__run_end(0, TEST_LOCATION(), __VA_ARGS__), 0) : 1;\ > >> + for_test_running_; \ > >> + test__run_end(1, TEST_LOCATION(), __VA_ARGS__), \ > >> + for_test_running_ = 0) > > > > IMHO: this is borderline "too much magic" for my tastes. I think > > having multiple test functions is generally easier to understand, and > > the overhead isn't really relevant. It's not _as_ compact in the > > source file, and requires that we have both the TEST statement and the > > function (and forgetting the TEST statement means that we won't invoke > > the function). If that is the main issue we're facing here, I wonder > > if there's some other way of resolving that (such as unused function > > detection via some compiler flags; even if it's not detected on all > > platforms, getting at least one of the CI platforms should be > > sufficient to prevent the issue [but we should target as many as > > possible, so it's caught earlier than "I tried to send it to the > > list"]) > > Interesting. > > > If others agree that this is a good simplification for the people > > reading the test code (and hopefully for the test author), I'm fine > > with this going in (with a different name). I'm not trying to veto the > > concept. > > OK. But what you suggested in the previous paragraph has merit. > Are there other things that could be improved in the existing unit > test helpers, that would help those who do not use this new for_test() > thing? Let's see how the patches to improve them would look like. Honestly I'm also not too sure how I feel about these new macros, and I'm somewhat in the same boat that it starts to feels "magicky". Taking a step back: what is it that is bugging folks about the current way of writing one function per test? The boilerplate cannot really be it, as tests should be self-contained anyway and thus we should have the same boilerplate regardless of whether we use separate functions or the new macros. I also doubt that it's the function declaration itself, as that isn't all that involved. So I guess what people are annoyed with is that every function needs to be defined, but then also wired up via `TEST_RUN()`, right? And that I totally get, as it is quite annoying. So... can we solve this in a different way? I quite liked the system that we had in libgit2: every function must conform to a specific name `test_foo_bar`. We then have a Python script that reads all test files, extracts all files that have the `test_` prefix, and writes those into an array. Optionally, the `test_foo` test suite can also have a setup and teardown function that gets called for every test, namely `test_foo_setup()` and `test_foo_teardown()`. Altogether, the output would look somewhat like this: ```test.c static test_foo_setup(void) { ... setup global state ... } static test_foo_teardown(void) { ... tear down global state ... } static test_foo_one(void) { ... first test ... } static test_foo_two(void) { ... second test ... } ``` ```generated.c static const struct test_func _test_foo_functions[] = { { name: "foo::one", test: test_foo_one, setup: test_foo_setup, teardown: test_foo_teardown, }, { name: "foo::two", test: test_foo_two, setup: test_foo_setup, teardown: test_foo_teardown, }, }; int main(int argc, const char *argv[]) { ... boilerplate to execute all functions ... } ``` The setup and teardown functions are optional -- if not defined for a specific test unit, then we simply won't invoke them. There is of course some magic involved with how we generate the file. But I think that would be quite manageable, and ultimately all that the developer would need to care about is writing a `test_foo_something()` function. Everything else would be handled by our infra. I'd be happy to implement something along these lines if folks think that this is a good direction to go into. Patrick
Attachment:
signature.asc
Description: PGP signature