From: Jeff King <peff@peff•net>
To: git@vger•kernel.org
Cc: Junio C Hamano <gitster@pobox•com>,
Phillip Wood <phillip.wood123@gmail•com>,
Patrick Steinhardt <ps@pks•im>, Taylor Blau <me@ttaylorr•com>
Subject: my complaints with clar
Date: Sun, 30 Nov 2025 08:46:25 -0500 [thread overview]
Message-ID: <20251130134625.GA199421@coredump.intra.peff.net> (raw)
In-Reply-To: <20251130131537.GB199335@coredump.intra.peff.net>
> --- /dev/null
> +++ b/t/unit-tests/u-parse-int.c
This was my first time writing unit tests with clar, so I thought I'd
document a few rough edges I found. It's possible I'm holding it wrong
in some areas.
> +static void check_int(const char *buf, size_t len,
> + size_t expect_ep_ofs, int expect_errno,
> + int expect_result)
> +{
> + const char *ep;
> + int result;
> + bool ok = parse_int_from_buf(buf, len, &ep, &result);
> +
> + if (expect_errno) {
> + cl_assert(!ok);
> + cl_assert_equal_i(expect_errno, errno);
> + return;
> + }
> +
> + cl_assert(ok);
> + cl_assert_equal_i(expect_result, result);
> + cl_assert_equal_i(expect_ep_ofs, ep - buf);
> +}
The error messages I got on failure from this function were not super
informative. Naively, if you do something like:
check_int_full("0", 0);
check_int_full("11", 11);
check_int_full("-23", -23);
check_int_full("+23", 23);
and it fails, you'll get not much beyond "expected ok, but it's not
true" with a line number in the helper, but no idea which input failed.
So OK, we have cl_invoke for that, and:
cl_invoke(check_int_full("0", 0));
cl_invoke(check_int_full("11", 11));
cl_invoke(check_int_full("-23", -23));
cl_invoke(check_int_full("+23", 23));
gives you the line number in the caller. Better, but there's a lot of
cross-referencing the line numbers (plus sprinkling cl_invoke everywhere
is ugly).
What I really would have liked is some notion of "context". If the
helper could have done:
cl_context("input: %.*s", (int)len, buf);
or similar, and failed assertions print that context, then that would
have made the failing part of the test easy to see, even without using
cl_invoke() at all.
Alternatively, I kind of wonder if cl_invoke() could just stringify the
entire macro argument and shove that into the context. That helps for:
cl_invoke(check_int_full("11", 11));
though not if parameters are opaque in that line, like:
cl_invoke(check_int_full(str, expect));
But I think boilerplate-saving helpers tend to be written more like the
first way.
In a more general sense, what I'd really have loved is an automatic
backtrace, but I suspect getting a readable one is impossible. Even if
we knew the called function and the parameters, a generic backtracer
cannot know the meaning of "buf" and "len" enough to show what was in
the buffer.
And of course the more obvious way to avoid that is to break this:
> +void test_parse_int__basic(void)
> +{
> + cl_invoke(check_int_full("0", 0));
> + cl_invoke(check_int_full("11", 11));
> + cl_invoke(check_int_full("-23", -23));
> + cl_invoke(check_int_full("+23", 23));
> + cl_invoke(check_int_str(" 31337 ", 7, 0, 31337));
> +
> + cl_invoke(check_int_err(" garbage", EINVAL));
> + cl_invoke(check_int_err("", EINVAL));
> + cl_invoke(check_int_err("-", EINVAL));
> +
> + cl_invoke(check_int("123", 2, 2, 0, 12));
> +}
into a series of nine separate tests, each of which gets a name. But
each of those tests is at least five lines of boilerplate, which sucks
(plus you have to come up with syntactically valid C names for them).
> + /*
> + * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro
> + * casts to int under the hood, corrupting the values.
> + */
> + clar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC,
> + CLAR_CURRENT_LINE,
> + "expect_result != result", 1,
> + "%"PRIuMAX, expect_result, result);
> +}
This was an exciting bug to track down. If you use i_fmt() here, you get
some neat undefined behavior. It worked for gcc, but failed with clang
(but only with -O2!).
Obviously this was me using it wrong, and the "i" in the macro should
have been a hint. But this invocation is kind of ugly, with the explicit
mentions of internal CLAR variables. clar__assert_equal() understands
PRIuMAX as a comparator, but there doesn't appear to be any macro to use
it nicely.
Should there be a generic cl_assert_equal() that fills in the first
few parameters but is otherwise type-agnostic?
It also looked error-prone to me that if you pass in an unknown format
specifier, clar__assert_equal() will assume you want integers. So a
typo, or using an unknown-but-equivalent specifier will give you weird
undefined behavior bugs. These are just tests, so we can perhaps a bit
more loose, but these kinds of things can be hard to track down
(especially if they trigger only in certain compiler combos via CI,
which is what happened to me).
And that brings me to my final complaint. ;)
When the unit-tests fail in CI, you get very little useful feedback,
because the output is eaten by "prove", and the unit tests don't
understand --verbose-log at all. And then to make it more exciting, the
output that clar produces actually chokes prove. For example, if I do
this:
diff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c
index a1601bb16b..da706d5840 100644
--- a/t/unit-tests/u-parse-int.c
+++ b/t/unit-tests/u-parse-int.c
@@ -38,7 +38,7 @@ static void check_int_err(const char *buf, int err)
void test_parse_int__basic(void)
{
cl_invoke(check_int_full("0", 0));
- cl_invoke(check_int_full("11", 11));
+ cl_invoke(check_int_full("11", 10));
cl_invoke(check_int_full("-23", -23));
cl_invoke(check_int_full("+23", 23));
then running t/unit-tests/bin/unit-tests produces:
# start of suite 10: parse_int
not ok 59 - parse_int::basic
---
reason: |
expect_result != result
10 != 11
at:
file: 't/unit-tests/u-parse-int.c'
line: 41
function: 'test_parse_int__basic'
---
OK, but "prove t/unit-tests/bin/unit-tests" gives me:
t/unit-tests/bin/unit-tests .. Failed 1/59 subtests
Test Summary Report
-------------------
t/unit-tests/bin/unit-tests (Wstat: (none) Tests: 59 Failed: 1)
Failed test: 59
Parse errors: Badly formed hash line: '---' at /usr/share/perl/5.40/TAP/Parser/YAMLish/Reader.pm line 244.
Yuck. It actually does have what I need (that test 59 was the failure),
so the extra parse error is mostly a red herring (though it does prevent
us finding any further failures). I think in TAP that arbitrary output
is supposed to be prefixed with a "#". In test-lib.sh, we solve this by
only allowing "--verbose-log", not regular "-v", under a TAP harness.
I kind of wonder if we should have t0011-unit-tests.sh that simply runs
unit-tests and filters the output into stdout and stderr. But maybe it's
too ugly. I think --verbose-log works because we know in the test code
when we are outputting TAP on stdout, and everything else goes to the
log. But because it's all generated by the unit-tests bin, we'd end up
having to parse its output ourselves and redirect some to stdout and
some to the log. It might be less work to implement --verbose-log in
our clar harness.
Anyway. Those are all of my complaints. For now. ;) I don't know if I'll
work on any of them or not, and maybe people more familiar with clar can
offer suggestions. But I thought it worth documenting the experience.
-Peff
next prev parent reply other threads:[~2025-11-30 13:46 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-12 7:55 [PATCH 0/9] asan bonanza Jeff King
2025-11-12 7:56 ` [PATCH 1/9] compat/mmap: mark unused argument in git_munmap() Jeff King
2025-11-12 8:01 ` [PATCH 2/9] pack-bitmap: handle name-hash lookups in incremental bitmaps Jeff King
2025-11-12 11:25 ` Patrick Steinhardt
2025-11-13 2:55 ` Taylor Blau
2025-11-18 8:59 ` Jeff King
2025-11-12 8:02 ` [PATCH 3/9] Makefile: turn on NO_MMAP when building with ASan Jeff King
2025-11-12 8:17 ` Collin Funk
2025-11-12 10:31 ` Jeff King
2025-11-12 20:06 ` Collin Funk
2025-11-12 11:26 ` Patrick Steinhardt
2025-11-13 3:12 ` Taylor Blau
2025-11-13 6:34 ` Patrick Steinhardt
2025-11-18 8:49 ` Jeff King
2025-11-13 16:30 ` Junio C Hamano
2025-11-14 7:00 ` Patrick Steinhardt
2025-11-15 2:13 ` Jeff King
2025-11-12 8:05 ` [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer Jeff King
2025-11-12 11:26 ` Patrick Steinhardt
2025-11-13 3:09 ` Taylor Blau
2025-11-18 8:40 ` Jeff King
2025-11-18 8:38 ` Jeff King
2025-11-12 8:06 ` [PATCH 5/9] fsck: assert newline presence in fsck_ident() Jeff King
2025-11-12 8:06 ` [PATCH 6/9] fsck: avoid strcspn() " Jeff King
2025-11-12 8:06 ` [PATCH 7/9] fsck: remove redundant date timestamp check Jeff King
2025-11-12 8:10 ` [PATCH 8/9] fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated Jeff King
2025-11-12 11:25 ` Patrick Steinhardt
2025-11-12 19:36 ` Junio C Hamano
2025-11-15 2:12 ` Jeff King
2025-11-12 8:10 ` [PATCH 9/9] t: enable ASan's strict_string_checks option Jeff King
2025-11-13 3:17 ` [PATCH 0/9] asan bonanza Taylor Blau
2025-11-18 9:11 ` [PATCH v2 " Jeff King
2025-11-18 9:11 ` [PATCH v2 1/9] compat/mmap: mark unused argument in git_munmap() Jeff King
2025-11-18 9:12 ` [PATCH v2 2/9] pack-bitmap: handle name-hash lookups in incremental bitmaps Jeff King
2025-11-18 9:12 ` [PATCH v2 3/9] Makefile: turn on NO_MMAP when building with ASan Jeff King
2025-11-18 9:12 ` [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer Jeff King
2025-11-18 14:30 ` Phillip Wood
2025-11-23 6:19 ` Junio C Hamano
2025-11-23 15:51 ` Phillip Wood
2025-11-23 18:06 ` Junio C Hamano
2025-11-24 22:30 ` Jeff King
2025-11-24 23:09 ` Junio C Hamano
2025-11-26 15:09 ` Jeff King
2025-11-26 17:22 ` Junio C Hamano
2025-11-30 13:13 ` [PATCH 0/4] more robust functions for parsing int from buf Jeff King
2025-11-30 13:14 ` [PATCH 1/4] parse: prefer bool to int for boolean returns Jeff King
2025-12-04 11:23 ` Patrick Steinhardt
2025-11-30 13:15 ` [PATCH 2/4] parse: add functions for parsing from non-string buffers Jeff King
2025-11-30 13:46 ` Jeff King [this message]
2025-12-01 14:16 ` my complaints with clar Phillip Wood
2025-12-04 11:09 ` Patrick Steinhardt
2025-12-05 18:30 ` Jeff King
2025-12-04 11:23 ` [PATCH 2/4] parse: add functions for parsing from non-string buffers Patrick Steinhardt
2025-12-05 16:11 ` Phillip Wood
2026-01-20 20:54 ` Junio C Hamano
2026-01-21 5:27 ` Jeff King
2025-11-30 13:15 ` [PATCH 3/4] cache-tree: use parse_int_from_buf() Jeff King
2025-11-30 13:16 ` [PATCH 4/4] fsck: use parse_unsigned_from_buf() for parsing timestamp Jeff King
2025-11-18 9:12 ` [PATCH v2 5/9] fsck: assert newline presence in fsck_ident() Jeff King
2025-11-18 9:12 ` [PATCH v2 6/9] fsck: avoid strcspn() " Jeff King
2025-11-18 9:12 ` [PATCH v2 7/9] fsck: remove redundant date timestamp check Jeff King
2025-11-18 9:12 ` [PATCH v2 8/9] fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated Jeff King
2025-11-18 9:12 ` [PATCH v2 9/9] t: enable ASan's strict_string_checks option Jeff King
2025-11-23 5:49 ` [PATCH v2 0/9] asan bonanza Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251130134625.GA199421@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=phillip.wood123@gmail$(echo .)com \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox