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

["Ghanshyam Thakkar" <shyamthakkar001@gmail.com>]

René Scharfe <l.s.r@web.de> wrote:
> 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?

Ah, apologies. I should clarify that I meant in general terms about the
future uses of TEST_RUN() and not about this particular patch. But I see
it being less of a problem now that I think about it more. And for the
record, I see no problems in this patch. But on a side note, with what
Phillip was suggesting to remove having TEST_RUN() inside if(), it
would definitely make verifying state independence more harder.

<snip>
> > 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.

Yeah, but I didn't mind that divide (and I don't mind bringing them
together either). :)

Thanks.