From: Phillip Wood <phillip.wood123@gmail•com>
To: Jeff King <peff@peff•net>, git@vger•kernel.org
Cc: Junio C Hamano <gitster@pobox•com>,
Patrick Steinhardt <ps@pks•im>, Taylor Blau <me@ttaylorr•com>
Subject: Re: my complaints with clar
Date: Mon, 1 Dec 2025 14:16:13 +0000 [thread overview]
Message-ID: <bd0a8a76-fccb-4b6c-abb7-b53dd890e9e0@gmail.com> (raw)
In-Reply-To: <20251130134625.GA199421@coredump.intra.peff.net>
Hi Peff
On 30/11/2025 13:46, Jeff King wrote:
>
> 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).
The README for the old unit-testing framework recommended wrapping
helper functions in a macro to pass the file and line numbers from the
calling site. Perhaps we should do the same with clar
#define check_int_full(input, expect) cl_invoke(check_int_full(input,
expect))
> 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.
If you're writing a helper function you might want to use cl_failf()
instead of cl_assert_* to provide more context but it's a pain that you
can't just use the builtin assertions. I've not used them but there are
assertions named cl_assert_*_ which I think let you add some context.
One of the features of the conversion of our unit tests from the old
framework to clar has been a degradation of the diagnostic messages when
a test fails.
>> +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).
Yes that's a pain. One of the nice things about the old framework was
the the TEST() macro just took an expression and created a test case out
of it which worked well for tests like this and meant you could have
table driven tests where each entry in the table was a separate test case.
>> + /*
>> + * 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?
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.
> # 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 "#".
TAP also allows you to embed YAML and unfortunately that's what clar
tries to do but that last " ---" line should be " ...". With the
diff below (which I'm afraid thunderbird will probably mangle) prove
parses the output correctly but still does not print the error message.
I'll update clar's self tests and open a PR later this week.
---- 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 ----
> 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.
I don't have a strong opinion on this but now that we don't run the unit
tests in parallel because clar links them all into a single executable
there is less reason to use prove.
Thanks
Phillip
next prev parent reply other threads:[~2025-12-01 14:16 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 ` my complaints with clar Jeff King
2025-12-01 14:16 ` Phillip Wood [this message]
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=bd0a8a76-fccb-4b6c-abb7-b53dd890e9e0@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--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