Re: [PATCH v7 11/14] t/unit-tests: implement test driver

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 04, 2024 at 02:35:20PM +0100, Phillip Wood wrote:
> Hi Patrick
> 
> On 03/09/2024 10:15, Patrick Steinhardt wrote:
> > The test driver in "unit-test.c" is responsible for setting up our unit
> > tests and eventually running them. As such, it is also responsible for
> > parsing the command line arguments.
> > 
> > The clar unit testing framework provides function `clar_test()` that
> > parses command line arguments and then executes the tests for us. In
> > theory that would already be sufficient. We have the special requirement
> > to always generate TAP-formatted output though, so we'd have to always
> > pass the "-t" argument to clar. Furthermore, some of the options exposed
> > by clar are ineffective when "-t" is used, but they would still be shown
> > when the user passes the "-h" parameter to have the clar show its usage.
> > 
> > Implement our own option handling instead of using the one provided by
> > clar, which gives us greater flexibility in how exactly we set things
> > up.
> 
> That makes sense
> 
> > We would ideally not use any "normal" code of ours for this such that
> > the unit testing framework doesn't depend on it working correctly. But
> > it is somewhat dubious whether we really want to reimplement all of the
> > option parsing. So for now, let's be pragmatic and reuse it until we
> > find a good reason in the future why we'd really want to avoid it.
> 
> I think that's fine for now. Using parse_options() gives a much nicer user
> experience than clar_test() as it supports long options and has more
> flexible support for option arguments. I'd expect the code that implements
> "struct string_list" and "struct strvec" to be pretty stable so its probably
> safe to rely on those.
> 
> Given there's only a couple of options it wouldn't be too bad to implement
> the parsing ourselves if we have to in the future. We might need to do that
> to support the libification work as I suspect we wont want to link tests for
> other libraries against libgit.a.
> 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >   t/unit-tests/unit-test.c | 43 ++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 41 insertions(+), 2 deletions(-)
> > 
> > diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
> > index 3d12cde6dae..96fa64de71d 100644
> > --- a/t/unit-tests/unit-test.c
> > +++ b/t/unit-tests/unit-test.c
> > @@ -1,6 +1,45 @@
> >   #include "unit-test.h"
> > +#include "parse-options.h"
> > +#include "string-list.h"
> > +#include "strvec.h"
> > -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> > +static const char * const unit_test_usage[] = {
> > +	N_("unit-test [<options>]"),
> > +	NULL,
> > +};
> > +
> > +int cmd_main(int argc, const char **argv)
> >   {
> > -	return 0;
> > +	struct string_list run_args = STRING_LIST_INIT_NODUP;
> > +	struct string_list exclude_args = STRING_LIST_INIT_NODUP;
> > +	int immediate = 0;
> > +	struct option options[] = {
> > +		OPT_BOOL('i', "--immediate", &immediate,
> > +			 N_("immediately exit upon the first failed test")),
> 
> This is unused. If we want to to behave like the "--immediate" option of our
> integration tests that's  hard to implement by wrapping clar_test() which
> requires "-i<suite>". The simplest thing would be to just drop it for now.
> Otherwise as the most likely use for "-i" is manually testing some tests in
> a suite we could require "--run" with "-i". Then we would have one or more
> suite names which we can append to "-i" when passing it to clar_test().
> Alternatively we could include "clar.suite" and wade through all the test
> suite names ourselves to construct a suitable list of "-i" options to pass
> to clar_test() but that would probably mean we have to parse the excludes as
> well which makes it all a bit of a faff.

Oh, that's a plain oversight on my side. It is easy to wire up given
that the clar already supports it via "-Q". Also made me notice that I
wrote "--immediate" instead of "immediate".

> > +		OPT_STRING_LIST('r', "run", &run_args, N_("name"),
> > +				N_("run only test suite or individual test <name>")),
> 
> It's nice that this option name now matches our integration tests. It would
> be helpful to show the syntax for "name" (I think it expects
> <suite>[::<test>]) but I failed to come up with a concise description to add
> to the help here.

Isn't `<suite[::test]>` concise enough? I certainly like it.

Patrick