Hi Patrick
On 20/08/2024 15:02, Patrick Steinhardt wrote:
Import the clar unit testing framework at commit 1516124 (Merge pull
request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework
will be wired up in subsequent commits.
I would be good to explain why we're doing this so we have a record in
the project history. A summary of the advantages and disadvantages of
using clar vs our current unit test framework would be helpful as well.
> diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
> [...]
> +static void
> +clar_parse_args(int argc, char **argv)
> +{
> [...]
+ case 'q':
+ _clar.report_errors_only = 1;
+ break;
I think this option is incompatible with TAP output as when TAP output
is selected the first error is printed as part of clar_print_ontest()
and clar_print_errors() is a no-op. We should error out if it is given
with '-t'
> +void clar__fail(
+ const char *file,
+ const char *function,
+ size_t line,
+ const char *error_msg,
+ const char *description,
+ int should_abort)
+{
+ struct clar_error *error = calloc(1, sizeof(struct clar_error));
clar seems to take an inconsistent approach to memory allocation errors.
Here "error" is de-referenced without checking if it is NULL and yet in
other places there are checks for NULL after calling calloc() or strdup().
+void clar__assert_equal(
+ const char *file,
+ const char *function,
+ size_t line,
+ const char *err,
+ int should_abort,
+ const char *fmt,
+ ...)
+{
+ va_list args;
+ char buf[4096];
+ int is_equal = 1;
+
+ va_start(args, fmt);
+
+ if (!strcmp("%s", fmt)) {
+ const char *s1 = va_arg(args, const char *);
+ const char *s2 = va_arg(args, const char *);
+ is_equal = (!s1 || !s2) ? (s1 == s2) : !strcmp(s1, s2);
+
+ if (!is_equal) {
+ if (s1 && s2) {
+ int pos;
+ for (pos = 0; s1[pos] == s2[pos] && s1[pos] && s2[pos]; ++pos)
+ /* find differing byte offset */;
+ p_snprintf(buf, sizeof(buf), "'%s' != '%s' (at byte %d)",
+ s1, s2, pos);
s1 and s2 are passed to snprintf without checking if they are NULL. As
'snprintf(buf, sizeof(buf), "%s", NULL)' is undefined we should check
for NULL here. The use of variadic arguments means there is no type
checking so if one is checking the contents of an strbuf and writes
cl_assert_equal_s(buf, "expect");
instead of
cl_assert_equal_s(buf->buf, "expect");
the compiler does not warn you. This is a regression compared to
check_str(). We can address this by having cl_assert_equal_s() wrap a
function that takes two strings. Another regression is that if the
string contains control characters they are printed verbatim.
diff --git a/t/unit-tests/clar/clar.h b/t/unit-tests/clar/clar.h
> [...]
+#define cl_assert_equal_i(i1,i2) clar__assert_equal(__FILE__,__func__,__LINE__,#i1 " != " #i2, 1, "%d", (int)(i1), (int)(i2))
The (int) casts here mean that we'll silently truncate longer arguments
and suppress any compiler warnings about comparing incompatible types.
> diff --git a/t/unit-tests/clar/clar/print.h
b/t/unit-tests/clar/clar/print.h
> [...]
+static void clar_print_tap_ontest(const char *suite_name, const char *test_name, int test_number, enum cl_test_status status)
+{
+ const struct clar_error *error = _clar.last_report->errors;
+
+ (void)test_name;
+ (void)test_number;
+
+ switch(status) {
+ case CL_TEST_OK:
+ printf("ok %d - %s::%s\n", test_number, suite_name, test_name);
+ break;
+ case CL_TEST_FAILURE:
+ printf("not ok %d - %s::%s\n", test_number, suite_name, test_name);
+
+ printf(" ---\n");
+ printf(" reason: |\n");
+ printf(" %s\n", error->error_msg);
+
+ if (error->description)
+ printf(" %s\n", error->description);
+
+ printf(" at:\n");
+ printf(" file: '"); print_escaped(error->file); printf("'\n");
+ printf(" line: %" PRIuZ "\n", error->line_number);
+ printf(" function: '%s'\n", error->function);
+ printf(" ---\n");
If a test calls cl_warning() only the message from the first call to
that function is printed and any message from later calls to
cl_warning() or clar__fail() is suppressed when TAP output is selected.
Best Wishes
Phillip
