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
