[v3] t-strvec: improve check_strvec() output

Commit Message

René Scharfe July 14, 2024, 10:17 a.m. UTC

The macro check_strvec calls the function check_strvec_loc(), which
performs the actual checks.  They report the line number inside that
function on error, which is not very helpful.  Half of them trigger
an assertion with a custom message that reports the line of the
check_strvec call, which is more useful, but a bit awkward.

Improve the output by getting rid of check_strvec_loc() and performing
all checks within check_strvec, as they then report the line number of
the call site, aiding in finding the broken test.  Determine the number
of items and check it up front to avoid having to do them both in the
loop and at the end.

Sanity check the expected items to make sure there are any and that the
last one is NULL, as the compiler no longer does that for us with the
removal of the function attribute LAST_ARG_MUST_BE_NULL.

Use only the actual strvec name passed to the macro, the internal
"expect" array name and an index "i" in the output, for clarity.  While
"expect" does not exist at the call site, it's reasonably easy to infer
that its referring to the NULL-terminated list of expected strings,
converted to an array.  Align the "i" with previous lines in the output.

Here's the output with less items than expected in the strvec before:

 # check "vec->nr > nr" failed at t/unit-tests/t-strvec.c:19
 #    left: 1
 #   right: 1
 # check "strvec index 1" failed at t/unit-tests/t-strvec.c:71

... and with the patch we get the more concise and still comprehensive:

 # check "(&vec)->nr == ARRAY_SIZE(expect) - 1" failed at t/unit-tests/t-strvec.c:53
 #    left: 1
 #   right: 2

With too many items in the strvec we got before:

 # check "vec->nr == nr" failed at t/unit-tests/t-strvec.c:34
 #    left: 1
 #   right: 0
 # check "vec->v[nr] == NULL" failed at t/unit-tests/t-strvec.c:36
 #    left: 0x6000019a40b0
 #   right: 0x0

... and with the patch we get the more concise and informative output
that includes the caller's line number:

 # check "(&vec)->nr == ARRAY_SIZE(expect) - 1" failed at t/unit-tests/t-strvec.c:53
 #    left: 1
 #   right: 0

A broken alloc value was reported like this:

 # check "vec->alloc > nr" failed at t/unit-tests/t-strvec.c:20
 #    left: 0
 #   right: 0
 # check "strvec index 0" failed at t/unit-tests/t-strvec.c:74

 ... and with the patch:

 # check "(&vec)->nr <= (&vec)->alloc" failed at t/unit-tests/t-strvec.c:56
 #    left: 2
 #   right: 0

Note that .alloc == .nr is only valid if the strvec is empty, but
check_strvec doesn't detect this error with or without this patch.
Leave that for later.

An unexpected string value was reported like this:

 # check "!strcmp(vec->v[nr], str)" failed at t/unit-tests/t-strvec.c:21
 #    left: "foo"
 #   right: "bar"
 # check "strvec index 0" failed at t/unit-tests/t-strvec.c:71

... and with the patch we get:

 # check "!strcmp((&vec)->v[i], expect[i])" failed at t/unit-tests/t-strvec.c:53
 #    left: "foo"
 #   right: "bar"
 #       i: 0

If the strvec is not NULL terminated, we got:

 # check "vec->v[nr] == NULL" failed at t/unit-tests/t-strvec.c:36
 #    left: 0x10444e9e4
 #   right: 0x0

... and with the patch we get the line number of the caller:

 # check "!strcmp((&vec)->v[i], expect[i])" failed at t/unit-tests/t-strvec.c:53
 #    left: "bar"
 #   right: NULL
 #       i: 1

check_strvec calls without a trailing NULL were detected at compile time
before:

 t/unit-tests/t-strvec.c:71:2: error: missing sentinel in function call [-Werror,-Wsentinel]

... and with the patch it's only found at runtime:

 # check "expect[ARRAY_SIZE(expect) - 1] == NULL" failed at t/unit-tests/t-strvec.c:53
 #    left: 0x100e5a663
 #   right: 0x0

We can let check_strvec add the terminating NULL for us and remove it
from callers, making it impossible to forget.  Leave that conversion for
a future patch, though, since this reimplementation is already intrusive
enough.

Reported-by: Phillip Wood <[e-mail]>
Helped-by: Jeff King <[e-mail]>
Signed-off-by: René Scharfe <[e-mail]>
---
Changes since v2:
- Replace check_strvec_loc() with macro code.
- Rewrite commit message and subject accordingly.

 t/unit-tests/t-strvec.c | 46 +++++++++++++----------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

--
2.45.2

Comments

René Scharfe July 14, 2024, 5:06 p.m. UTC | #1

Oh, v2 is already on next.  Will send the equivalent of v3 as an update
on top of next as a replacement.

René

Jeff King July 16, 2024, 4:37 a.m. UTC | #2

On Sun, Jul 14, 2024 at 12:17:53PM +0200, René Scharfe wrote:

> +	do { \
> +		const char *expect[] = { __VA_ARGS__ }; \
> +		if (check_uint(ARRAY_SIZE(expect), >, 0) && \
> +		    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \
> +		    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \
> +		    check_uint((vec)->nr, <=, (vec)->alloc)) { \
> +			for (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \
> +				if (!check_str((vec)->v[i], expect[i])) { \
> +					test_msg("      i: %"PRIuMAX, i); \
> +					break; \
> +				} \
> +			} \
> +		} \
> +	} while (0)

The linux32 CI job seems to complain about this, since the concrete type
of "i" (a size_t) is "unsigned int" there, but PRIuMAX is %llu.
Presumably you just need to cast to uintmax_t.

-Peff

Junio C Hamano July 16, 2024, 3:20 p.m. UTC | #3

Jeff King <[e-mail]> writes:

> On Sun, Jul 14, 2024 at 12:17:53PM +0200, René Scharfe wrote:
>
>> +	do { \
>> +		const char *expect[] = { __VA_ARGS__ }; \
>> +		if (check_uint(ARRAY_SIZE(expect), >, 0) && \
>> +		    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \
>> +		    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \
>> +		    check_uint((vec)->nr, <=, (vec)->alloc)) { \
>> +			for (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \
>> +				if (!check_str((vec)->v[i], expect[i])) { \
>> +					test_msg("      i: %"PRIuMAX, i); \
>> +					break; \
>> +				} \
>> +			} \
>> +		} \
>> +	} while (0)
>
> The linux32 CI job seems to complain about this, since the concrete type
> of "i" (a size_t) is "unsigned int" there, but PRIuMAX is %llu.
> Presumably you just need to cast to uintmax_t.

Yeah, I noticed it, too, but unfortunately it was after pushing it
out and seeing it break there X-<.

Patch

diff --git a/t/unit-tests/t-strvec.c b/t/unit-tests/t-strvec.c
index d4615ab06d..fdb28ba220 100644
--- a/t/unit-tests/t-strvec.c
+++ b/t/unit-tests/t-strvec.c
@@ -3,38 +3,20 @@ 
 #include "strvec.h"

 #define check_strvec(vec, ...) \
-	check_strvec_loc(TEST_LOCATION(), vec, __VA_ARGS__)
-LAST_ARG_MUST_BE_NULL
-static void check_strvec_loc(const char *loc, struct strvec *vec, ...)
-{
-	va_list ap;
-	size_t nr = 0;
-
-	va_start(ap, vec);
-	while (1) {
-		const char *str = va_arg(ap, const char *);
-		if (!str)
-			break;
-
-		if (!check_uint(vec->nr, >, nr) ||
-		    !check_uint(vec->alloc, >, nr) ||
-		    !check_str(vec->v[nr], str)) {
-			struct strbuf msg = STRBUF_INIT;
-			strbuf_addf(&msg, "strvec index %"PRIuMAX, (uintmax_t) nr);
-			test_assert(loc, msg.buf, 0);
-			strbuf_release(&msg);
-			va_end(ap);
-			return;
-		}
-
-		nr++;
-	}
-	va_end(ap);
-
-	check_uint(vec->nr, ==, nr);
-	check_uint(vec->alloc, >=, nr);
-	check_pointer_eq(vec->v[nr], NULL);
-}
+	do { \
+		const char *expect[] = { __VA_ARGS__ }; \
+		if (check_uint(ARRAY_SIZE(expect), >, 0) && \
+		    check_pointer_eq(expect[ARRAY_SIZE(expect) - 1], NULL) && \
+		    check_uint((vec)->nr, ==, ARRAY_SIZE(expect) - 1) && \
+		    check_uint((vec)->nr, <=, (vec)->alloc)) { \
+			for (size_t i = 0; i < ARRAY_SIZE(expect); i++) { \
+				if (!check_str((vec)->v[i], expect[i])) { \
+					test_msg("      i: %"PRIuMAX, i); \
+					break; \
+				} \
+			} \
+		} \
+	} while (0)

 static void t_static_init(void)
 {