Re: my complaints with clar

From: Jeff King <peff@peff.net>
Date: Dec 5, 2025, 18:30 UTC
Message-ID: <20251205183057.GA33447@coredump.intra.peff.net>
In-Reply-To: <aTFsA-jJqcRZJs53@pks.im>

On Thu, Dec 04, 2025 at 12:09:55PM +0100, Patrick Steinhardt wrote:

Show 9 quoted lines

> > Patrick's got a PR open for that at
> > https://github.com/clar-test/clar/pull/117 it seems to have got stuck
> > because of a lack of review.
> 
> Yeah, this is indeed a long-standing issue. As Phillip mentioned I've
> already had the fix pending, but I lost track and just never merged it.
> Phillip now left a review, and I've polished the PR a bit. I'll wait a
> few more days before merging it, and then these issues will be a thing
> of the past :)

Great, thanks.

Show 25 quoted lines

> > ---- 8< ----
> > diff --git a/t/unit-tests/clar/clar/print.h b/t/unit-tests/clar/clar/print.h
> > index 89b66591d75..6a2321b399d 100644
> > --- a/t/unit-tests/clar/clar/print.h
> > +++ b/t/unit-tests/clar/clar/print.h
> > @@ -164,7 +164,7 @@ static void clar_print_tap_ontest(const char
> > *suite_name, const char *test_name,
> >                          printf("      file: '");
> > print_escaped(error->file); printf("'\n");
> >                          printf("      line: %" PRIuMAX "\n",
> > error->line_number);
> >                          printf("      function: '%s'\n", error->function);
> > -                        printf("    ---\n");
> > +                        printf("    ...\n");
> >                  }
> > 
> >                  break;
> > ---- >8 ----
> 
> Indeed, this was a plain bug. I've merged your upstream PR, thanks!
> 
> I'll send a pull request soonish to update our own version of clar.
> 
> Thanks for the feedback! Hope that the pending changes will improve the
> status quo.

It is certainly better for the TAP parser not to choke on the output, but the more fundamental issue remains that the output is never stored or relayed anywhere in our CI runs.

I looked at implementing --verbose-log in our unit-tests clar wrapper, but it's tricky. For one thing, we have to know where to store the test-results (so understanding $TEST_OUTPUT_DIRECTORY and how it may be set). Plus, we would then need to duplicate much of the output, going to both the log and to stdout. And all of the output routines are inside clar. So we'd need hooks there.

The regular test suite uses "tee" to duplicate the output without the script having to worry about it. We could use the same trick here.

And an easy way to solve both issues is to just call it from the test suite, letting it handle --verbose-log itself!

Something like this seems to work:

  #!/bin/sh
  test_description='run clar unit tests'
  . ./test-lib.sh
  exec "$TEST_DIRECTORY/unit-tests/bin/unit-tests" ${immediate:+-i}

We have to "exec" there so that we skip out on the exit handler that checks we called test_done. And we obviously cannot call that, because it outputs extra TAP.

But the exec means we also miss some cleanup, like removing our trash directory.

Probably we'd want a mode in the shell test suite that says "this thing is going to generate TAP, just stay out of its way". We used to have test_external, but it was removed in 5beca49a0b (test-lib: simplify by removing test_external, 2022-07-28). The "modern" alternative is:

  test_expect_success 'run unit-tests' '
	unit-tests/bin/unit-tests
  '

which feels worse (the outer layer of TAP output is all-or-nothing, and the fact that the unit tests produce TAP is irrelevant). I think it roughly accomplishes the same thing, but it might be worth looking again at the reasons for dropping test_external in the first place.

-Peff