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

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

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.

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. 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().

Thanks.