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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 28, 2024 at 02:18:02PM +0100, Phillip Wood wrote:
> Hi Patrick
> 
> On 20/08/2024 15:02, Patrick Steinhardt wrote:
> > Convert the ctype tests to use the new clar unit testing framework.
> > Introduce a new function `cl_failf()`
> 
> This is a nice addition which somewhat mitigates the lack of an equivalent
> to test_msg() that adds addition context messages to test failures.
> 
> > that allows us to print a
> > formatted error message, which we can use to point out which of the
> > characters was classified incorrectly. This results in output like this
> > on failure:
> > 
> >      # start of suite 1: ctype
> >      ok 1 - ctype::isspace
> >      not ok 2 - ctype::isdigit
> >          ---
> >          reason: |
> >            Test failed.
> 
> "Test failed." is not the reason for the test failure

The clar expects two strings, one "general" description and the details.
I agree that it's a bit shoddy to have this as part of the reason, it
really should be a separate "field". But it allows us to distinguish
e.g. between test failures and tests which emitted a warning, only.

> >            0x61 is classified incorrectly
> >          at:
> >            file: 't/unit-tests/ctype.c'
> >            line: 38
> >            function: 'test_ctype__isdigit'
> >          ---
> 
> This is rather verbose compared to the current framework

I guess this is a matter of taste. I actually prefer the more verbose
style.

> # check "isdigit(i) == !!memchr("123456789", i, len)" failed at
> t/unit-tests/t-ctype.c:36
> #    left: 1
> #   right: 0
> #       i: 0x30
> not ok 2 - isdigit works
> 
> The current tests also shows which characters are expected to return true
> and distinguishes between the two possible failure modes which are (a)
> misclassification and (b) returning a non-zero integer other than '1' as
> "true". The new test output does not allow the person running the test to
> distinguish between these two failure modes.

This one is true though. Will amend.

> > diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h
> > index 66ec2387cc6..4c461defe16 100644
> > --- a/t/unit-tests/unit-test.h
> > +++ b/t/unit-tests/unit-test.h
> > @@ -1,3 +1,10 @@
> >   #include "git-compat-util.h"
> >   #include "clar/clar.h"
> >   #include "clar-decls.h"
> > +#include "strbuf.h"
> > +
> > +#define cl_failf(fmt, ...) do { \
> > +	char *desc = xstrfmt(fmt, __VA_ARGS__); \
> 
> In our current framework we avoid relying on the strbuf api and functions
> like xstrfmt() that use it as we want to be able to test strbuf.c with the
> framework. We'd be better with a macro wrapping a new function that uses a
> stack allocated buffer and p_snprintf() like clar__assert_equal(). That
> would allow us to upstream this change as well.

I don't think it's all that important to avoid a low-level API like
`xstrfmt()`. But the second argument makes sense to me indeed.

> > +	clar__fail(__FILE__, __func__, __LINE__, "Test failed.", desc, 1); \
> > +	free(desc); \
> 
> This is leaked on failure due to the use of longjmp.

As discussed elsewhere I don't think this leak matters all that much.
But it's getting fixed with your proposal, so that's that.

Patrick