On 24/07/2024 20:24, Kyle Lippincott wrote:
On Wed, Jul 24, 2024 at 7:53 AM René Scharfe <l.s.r@xxxxxx> wrote:
Macros like this require additional cognitive overhead to understand;
I can't read `for_test ("for_test passing test") check_int(1, ==, 1);`
and understand what's going on without going and reading the comment
above `for_test`. Unlike `TEST(<func>, "<description>")`, I can't even
_guess_ what it's doing. This macro is project-specific, and this
knowledge has to be attained by everyone wishing to even read this
code.
As well as the obscure name and oddness of using "continue" to exit a
test early I think having two ways of defining tests also adds to the
cognitive overhead of writing tests as one now has to decide which to use.
As you say below it is not clear to me that the changes in the last
three patches that start using for_test() in our existing tests are an
improvement.
This will be the last comment from me for a while as I'm about to go off
the list for a couple of weeks
Best Wishes
Phillip
I personally consider the patches that use this to be regressions in
my ability to read and understand the tests. As an example, one of the
diff hunks in the strbuf patch (patch #6 in the series) does this:
- TEST(setup_populated(t_addch, "initial value", "a"),
- "strbuf_addch appends to initial value");
+ for_test ("strbuf_addch appends to initial value") {
+ struct strbuf sb = STRBUF_INIT;
+ t_addstr(&sb, "initial value");
+ t_addch(&sb, 'a');
+ t_release(&sb);
+ }
But the main simplification in that patch (not using
`setup_populated`) can be achieved without `for_test`:
+ static void t_addch_appends(void)
+ {
+ struct strbuf sb = STRBUF_INIT;
+ t_addstr(&sb, "initial value");
+ t_addch(&sb, 'a');
+ t_release(&sb);
+ }
- TEST(setup_populated(t_addch, "initial value", "a"),
- "strbuf_addch appends to initial value");
+ TEST(t_addch_appends(), "strbuf_addch appends to initial value");
It seems to me that all `for_test` is getting us there is an inlining
of the test logic, which seems like it's optimizing for vertical space
in the file. Maybe it's because I'm coming from a C++ environment at
$JOB that's using Google's gunit and gmock frameworks, where every
test is in its own function and we usually don't even write the main
function ourselves, but I have a preference for the separate
functions.
Maybe I'm in the minority, so only consider this at somewhere like a
-0.5 on this series (fine with deferring to a reviewer who is
significantly in favor of it, but if there aren't any, I'd lean
towards not landing this).
