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

From: Josh Steadmon <steadmon@google.com>
Date: Jul 8, 2024, 18:11 UTC
Message-ID: <tswyfparvchgi7qxrjxbx4eb7cohypzekjqzbnkbffsesaiazs@vtewtz7o6twi>
In-Reply-To: <D2F8BS39IFC4.WJ9TNK3IDFBV@gmail.com>

On 2024.07.02 22:59, Ghanshyam Thakkar wrote:

Show 53 quoted lines

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

It's a matter of taste rather than strict principles, I suppose, but I think that test cases should focus on being readable rather than avoiding duplication. Production code can follow a Don't Repeat Yourself philosophy, but when a test breaks, it's better to be able to look at only the failing test and quickly see what's wrong, rather than having to work out which functions call which callbacks and what gets verified where. Also, since we don't have tests for our tests, it's important that the tests are easy for people to manually verify them.

I also agree with René's point about the test cases matching real-world API usage.

I do agree that having separate functions improves isolation, but hopefully we can encourage test authors to define local variables in their TEST_RUN blocks.

Show 18 quoted lines

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