Am 23.07.24 um 08:36 schrieb Patrick Steinhardt: > 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? My goal is to be able to define a test without repeating its description even partly, like test_expect_success allows for shell-based tests. > 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. So the name of a test is generated automatically based on the function name and there is no way to add a description beyond that. This would achieve my goal above, but prevent developers from using space and punctuation in test descriptions. Fair enough. > There is of course some magic involved with how we generate the file. It requires magic function names and generates code using a different language, while for_test is a just single new control flow keyword, like the dozen or so we already have. So the magic employed by the libgit2 system is both broader and deeper. > 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. With for_test all the developer has to do is write a test with a description, no extra infrastructure beyond the existing unit test framework needed. > I'd be happy to implement something along these lines if folks think > that this is a good direction to go into. FWIW I don't see the appeal. It uses more code and more dependencies to almost allow what for_test permits. René