Am 02.07.24 um 19:29 schrieb Ghanshyam Thakkar:
> Josh Steadmon <steadmon@xxxxxxxxxx> 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é
