Re: [PATCH 6/6] t-strbuf: use TEST_RUN

["René Scharfe" <l.s.r@web.de>]

Am 02.07.24 um 19:29 schrieb Ghanshyam Thakkar:
> Josh Steadmon <steadmon@google.com> wrote:
>>> -	TEST(setup(t_addch, "a"), "strbuf_addch adds char");
>>> -	TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
>>> -	TEST(setup_populated(t_addch, "initial value", "a"),
>>> -	     "strbuf_addch appends to initial value");
>>> -	TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
>>> -	TEST(setup_populated(t_addstr, "initial value", "hello there"),
>>> -	     "strbuf_addstr appends string to initial value");
>>> +
>>> +	if (TEST_RUN("strbuf_addch adds char")) {
>>> +		struct strbuf sb = STRBUF_INIT;
>>> +		t_addch(&sb, 'a');
>>> +		t_release(&sb);
>>> +	}
>>> +
>>> +	if (TEST_RUN("strbuf_addch adds NUL char")) {
>>> +		struct strbuf sb = STRBUF_INIT;
>>> +		t_addch(&sb, '\0');
>>> +		t_release(&sb);
>>> +	}
>>> +
>>> +	if (TEST_RUN("strbuf_addch appends to initial value")) {
>>> +		struct strbuf sb = STRBUF_INIT;
>>> +		t_addstr(&sb, "initial value");
>>> +		t_addch(&sb, 'a');
>>> +		t_release(&sb);
>>> +	}
>>> +
>>> +	if (TEST_RUN("strbuf_addstr adds string")) {
>>> +		struct strbuf sb = STRBUF_INIT;
>>> +		t_addstr(&sb, "hello there");
>>> +		t_release(&sb);
>>> +	}
>>> +
>>> +	if (TEST_RUN("strbuf_addstr appends string to initial value")) {
>>> +		struct strbuf sb = STRBUF_INIT;
>>> +		t_addstr(&sb, "initial value");
>>> +		t_addstr(&sb, "hello there");
>>> +		t_release(&sb);
>>> +	}
>>>
>>>  	return test_done();
>>>  }
>>> --
>>> 2.45.2
>>
>> I think this commit in particular shows how TEST_RUN() is more
>> convenient than TEST(). (Although, arguably we shouldn't have allowed
>> the setup() + callback situation to start with.)
>
> Could you expand a bit on why the setup() + callback thing shouldn't be
> allowed? I think it is a nice way of avoiding boilerplate and having
> independent state. And, I see the true potential of TEST_RUN() in
> testcases defined through macros rather than replacing functions. I
> actually think that the previous version with the functions was not
> particularly bad, and I agree with Phillip that the previous version's
> main() provided nice overview of the tests and it was easier to
> verify the independence between each testcase.

Each test uses its own strbuf and the t_ functions don't use global or
static variables, so how does the doubt about their independence creep
in?

With tests specified as above I now notice that we never check the
resulting string.  t_addch and t_addstr check the tail matches their
argument, but strbuf_addch and strbuf_addstr could overwrite the old
content and we wouldn't notice.  Perhaps it's not worth checking, but
now that tests look more like test_expect_success and all steps are
visible I somehow miss the equivalent of test_cmp; didn't see that
in the original version.

> Perhaps, the code snippets inside the functions are small enough to
> perceive TEST_RUN() as more convenient than TEST() in this test, but,
> for future reference, I definitely don't think TEST_RUN() should be
> looked at as a replacement for TEST(), and more like 'when we have to
> use macro magic which requires us to use internal test__run_*
> functions, using TEST_RUN() is more convenient. Patch [3/6] is a good> example of that.

Sure, not all tests will be improved by using TEST_RUN.  If TEST fits
better, use it.

> But, I also don't mind if patches 4, 5, or 6 get
> merged as I don't see any difference between using TEST_RUN() or
> TEST() in those patches, besides moving everything inside main().

The difference is that in the original version test description and
definition are separated, only linked by a function name.  The new
version brings them together and does away with function name.  A small
change, for sure, just to get rid of the artificial divide and the need
for that link.

René