Re: [PATCH v6 11/13] t/unit-tests: convert strvec tests to use clar

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 28, 2024 at 02:17:05PM +0100, Phillip Wood wrote:
> Hi Patrick
> 
> On 20/08/2024 15:02, Patrick Steinhardt wrote:
> > Convert the strvec tests to use the new clar unit testing framework.
> > This is a first test balloon that demonstrates how the testing infra for
> > clar-based tests looks like.
> > 
> > The tests are part of the "t/unit-tests/bin/unit-tests" binary. When
> > running that binary, it generates TAP output:
> 
> It would be interesting to see a comparison between the current framework
> and clar of the output from a failing test - the TAP output for passing
> tests is pretty much the same regardless of the framework used.

Will do.

> >      # ./t/unit-tests/bin/unit-tests
> >      TAP version 13
> >      # start of suite 1: strvec
> >      ok 1 - strvec::init
> > [...] The binary also supports some parameters that allow us to run only
> > a
> > subset of unit tests or alter the output:
> > 
> >      $ ./t/unit-tests/bin/unit-tests -h
> >      Usage: ./t/unit-tests/bin/unit-tests [options]
> > 
> >      Options:
> >        -sname        Run only the suite with `name` (can go to individual test name)
> >        -iname        Include the suite with `name`
> >        -xname        Exclude the suite with `name`
> >        -v            Increase verbosity (show suite names)
> 
> The output above seems to include the suite name - are we running the tests
> with '-v' from our Makefile?

The `-v` switch is actually doing nothing when generating TAP output.

> >        -q            Only report tests that had an error
> 
> This option is incompatible with TAP output. As we force TAP output we
> should find a way to stop displaying this help.
> 
> >        -Q            Quit as soon as a test fails
> >        -t            Display results in tap format
> 
> We force TAP output by adding '-t' to argv in main() so this line is not
> very helpful

True indeed. This is the default argument parsing and output from clar,
so it's nothing that we can change. That being said, I guess the best
way to address this is to use our own option parsing here instead of
using whatever clar provides, and then we can also print our own usage.

Will amend accordingly.

> >        -l            Print suite names
> >        -r[filename]  Write summary file (to the optional filename)
> 
> > diff --git a/t/unit-tests/strvec.c b/t/unit-tests/strvec.c
> > [..]
> > +#define check_strvec(vec, ...) \
> > +	do { \
> > +		const char *expect[] = { __VA_ARGS__ }; \
> > +		cl_assert(ARRAY_SIZE(expect) > 0); \
> 
> As there are a lot occurrences of ARRAY_SIZE(expect) it is probably worth
> adding
> 
> 	size_t expect_len = ARRAY_SIZE(expect);
> 
> above.

Can do.

> > +		cl_assert_equal_p(expect[ARRAY_SIZE(expect) - 1], NULL); \
> > +		cl_assert_equal_i((vec)->nr, ARRAY_SIZE(expect) - 1); \
> > +		cl_assert((vec)->nr <= (vec)->alloc); \
> 
> The conversion here loses the values of nr and alloc which is a shame as
> they would be useful when debugging a test failure.

This is something I'd address in the future, once we have macros that
can do relative comparisons.

> > +		for (size_t i = 0; i < ARRAY_SIZE(expect); i++) \
> > +			cl_assert_equal_s((vec)->v[i], expect[i]); \
> 
> The original test also printed the array index of the failing check. As the
> elements of the test vectors all seem to be unique that is less of a worry
> than if we had tests with repeating elements.
> 
> > +	} while (0)
> > +
> > +void test_strvec__init(void)
> > +{
> > +	struct strvec vec = STRVEC_INIT;
> 
> If we're rewriting the tests perhaps we can take the opportunity to add a
> blank line to each one after the variable declarations in accordance with
> our coding guidelines.

Can do.

> It might be a good opportunity to show the set-up and tear-down facilities
> in clar as well instead of repeating the initialization in each test.

I don't think it's a good fit here, as setup and teardown would hit the
system under test. I rather think they should be used in cases where you
e.g. always have to setup a repository for your tests.

Patrick