On Mon, Jul 22, 2024 at 12:37 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Kyle Lippincott <spectral@xxxxxxxxxx> writes: > > > I can see based on this description where the name came from, but > > without this context, it's not clear when reading a test what it > > actually does. The name comes from an implementation detail, and is > > not describing what it _does_, just _how_ it does it. > > > > Maybe a name like `small_test` or `quick_test` or something like that > > would better express the intended usage? > > Names that explicitly have C keyword for control structures, e.g. > "if_somecondition_test()", "while_foo_test()" or "for_test()" is > vastly preferrable than these, in order to make it clear that we are > introducing a macro that defines control structure. Perhaps expression_test or something, then? It's testing an expression (I think blocks are a type of expression? I never remember which is 'larger': expressions or statements, and that might only be a C++ thing, C might not have the same terminology). I was going to suggest a lint check that checks to ensure that we don't have a semicolon immediately after the description, or a lint check that enforced that even single-statement tests are wrapped in {} (inverting the style guide requirements), but realistically neither are actually needed: `test__run_begin` already asserts that a test isn't currently running. `check_int` and friends already assert that a test _is_ running. So this is already defended against: for_test ("test description"); check_int(1, ==, 1); /* `assert`s: not in the middle of a test */ What we don't have is defense against a completely empty test, or a test without any actual conditions: for_test ("test one"); for_test ("test two") { printf("this test doesn't actually _test_ anything\n"); } Adding that is doable, and improves the first case: the `for_test ("test description");` line fails because the test didn't do anything, not the `check_int` line. > > >> + for_test ("for_test passing test") > >> + check_int(1, ==, 1); > > > > I'm concerned that users will write this like: > > + for_test ("for_test passing test"); > > + check_int(1, ==, 1); > > And that is exactly why we want the macro name to include C keyword > for control structures. > > > And the issue won't be caught. > > You are right. Making an empty body somehow catchable by the > compiler would be a vast improvement. > > >> +#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. > > Thanks. >