git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 3/4] t/unit-tests: convert strbuf test to use clar test framework

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Feb 2, 2025, 14:38 UTC
Message-ID
<ab05e979-2a5f-4532-bb0c-e8c59665647c@gmail.com>
In-Reply-To
<20250131221420.38161-4-kuforiji98@gmail.com>
Hi Seyi
On 31/01/2025 22:14, Seyi Kuforiji wrote:
> Adapt strbuf test script to clar framework by using clar assertions
> where necessary.

This patch looks correct but it looses the nice messages we get from check_char() and check_uint(a, </<=/>/>=, b). I think it would be worth adding equivalent assertions to clar to avoid that.

Show 9 quoted lines
>   /* wrapper that supplies tests with an empty, initialized strbuf */
> @@ -9,8 +9,8 @@ static void setup(void (*f)(struct strbuf*, const void*),
>   
>   	f(&buf, data);
>   	strbuf_release(&buf);
> -	check_uint(buf.len, ==, 0);
> -	check_uint(buf.alloc, ==, 0);
> +	cl_assert_equal_i(buf.len, 0);
> +	cl_assert_equal_i(buf.alloc, 0);

It is a shame that we do not have an assertion for checking two unsigned integers are equal. In this case it probably does not matter but large unsigned values will be printed as negative numbers which could be rather confusing for someone trying to debug a test failure.

Show 11 quoted lines
> -static int assert_sane_strbuf(struct strbuf *buf)
> +static void assert_sane_strbuf(struct strbuf *buf)
>   {
>   	/* Initialized strbufs should always have a non-NULL buffer */
> -	if (!check(!!buf->buf))
> -		return 0;
> +	cl_assert(buf->buf != NULL);
>   	/* Buffers should always be NUL-terminated */
> -	if (!check_char(buf->buf[buf->len], ==, '\0'))
> -		return 0;
> +	cl_assert(buf->buf[buf->len] == '\0');

It is unfortunate that this looses the helpful diagnostic message that shows the values of the two chars being compared that is printed by check_char() when the check fails. I think it would be worth porting check_char() arcoss to clar. When we decided to move to the new test framework we planned to add new assertions to clar to match our existing framework.

Show 13 quoted lines
>   	/*
> -	 * Freshly-initialized strbufs may not have a dynamically allocated
> -	 * buffer
> -	 */
> -	if (buf->len == 0 && buf->alloc == 0)
> -		return 1;
> -	/* alloc must be at least one byte larger than len */
> -	return check_uint(buf->len, <, buf->alloc);
> +         * In case the buffer contains anything, `alloc` must alloc must
> +         * be at least one byte larger than `len`.
> +         */
> +	if (buf->len)
> +            cl_assert(buf->len < buf->alloc);

This is another case where we loose a helpful diagnostic message because clar lacks cl_assert_lt_u(a, b) to check that unsigned integer a is less than unsigned integer b. I think it would be worth adding the assertions that are missing show that we have the equivalent of

	check_int(a, <, b)
	check_int(a, <=, b)
	check_int(a, >, b)
	check_int(a, >=, b)

and their unsigned companions so we do not regress the diagnostic output when a test fails.

Looking at the comment that is deleted above I wonder if we should add
	else
		cl_assert_equal_i(buf->alloc, 0);
To check that alloc is zero when len is zero.
Best Wishes
Phillip
Show 114 quoted lines
>   }
>   
> -static void t_static_init(void)
> +void test_strbuf__static_init(void)
>   {
>   	struct strbuf buf = STRBUF_INIT;
>   
> -	check_uint(buf.len, ==, 0);
> -	check_uint(buf.alloc, ==, 0);
> -	check_char(buf.buf[0], ==, '\0');
> +	cl_assert_equal_i(buf.len, 0);
> +	cl_assert_equal_i(buf.alloc, 0);
> +	cl_assert(buf.buf[0] == '\0');
>   }
>   
> -static void t_dynamic_init(void)
> +void test_strbuf__dynamic_init(void)
>   {
>   	struct strbuf buf;
>   
>   	strbuf_init(&buf, 1024);
> -	check(assert_sane_strbuf(&buf));
> -	check_uint(buf.len, ==, 0);
> -	check_uint(buf.alloc, >=, 1024);
> -	check_char(buf.buf[0], ==, '\0');
> +	assert_sane_strbuf(&buf);
> +	cl_assert_equal_i(buf.len, 0);
> +	cl_assert(buf.alloc >= 1024);
> +	cl_assert(buf.buf[0] == '\0');
>   	strbuf_release(&buf);
>   }
>   
> @@ -73,16 +69,12 @@ static void t_addch(struct strbuf *buf, const void *data)
>   	size_t orig_alloc = buf->alloc;
>   	size_t orig_len = buf->len;
>   
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> +	assert_sane_strbuf(buf);
>   	strbuf_addch(buf, ch);
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> -	if (!(check_uint(buf->len, ==, orig_len + 1) &&
> -	      check_uint(buf->alloc, >=, orig_alloc)))
> -		return; /* avoid de-referencing buf->buf */
> -	check_char(buf->buf[buf->len - 1], ==, ch);
> -	check_char(buf->buf[buf->len], ==, '\0');
> +	assert_sane_strbuf(buf);
> +	cl_assert_equal_i(buf->len, orig_len + 1);
> +	cl_assert(buf->alloc >= orig_alloc);
> +	cl_assert(buf->buf[buf->len] == '\0');
>   }
>   
>   static void t_addstr(struct strbuf *buf, const void *data)
> @@ -92,31 +84,36 @@ static void t_addstr(struct strbuf *buf, const void *data)
>   	size_t orig_alloc = buf->alloc;
>   	size_t orig_len = buf->len;
>   
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> +	assert_sane_strbuf(buf);
>   	strbuf_addstr(buf, text);
> -	if (!check(assert_sane_strbuf(buf)))
> -		return;
> -	if (!(check_uint(buf->len, ==, orig_len + len) &&
> -	      check_uint(buf->alloc, >=, orig_alloc) &&
> -	      check_uint(buf->alloc, >, orig_len + len) &&
> -	      check_char(buf->buf[orig_len + len], ==, '\0')))
> -	    return;
> -	check_str(buf->buf + orig_len, text);
> +	assert_sane_strbuf(buf);
> +	cl_assert_equal_i(buf->len, orig_len + len);
> +	cl_assert(buf->alloc >= orig_alloc);
> +	cl_assert(buf->buf[buf->len] == '\0');
> +	cl_assert_equal_s(buf->buf + orig_len, text);
>   }
>   
> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +void test_strbuf__add_single_char(void)
>   {
> -	if (!TEST(t_static_init(), "static initialization works"))
> -		test_skip_all("STRBUF_INIT is broken");
> -	TEST(t_dynamic_init(), "dynamic initialization works");
> -	TEST(setup(t_addch, "a"), "strbuf_addch adds char");
> -	TEST(setup(t_addch, ""), "strbuf_addch adds NUL char");
> -	TEST(setup_populated(t_addch, "initial value", "a"),
> -	     "strbuf_addch appends to initial value");
> -	TEST(setup(t_addstr, "hello there"), "strbuf_addstr adds string");
> -	TEST(setup_populated(t_addstr, "initial value", "hello there"),
> -	     "strbuf_addstr appends string to initial value");
> -
> -	return test_done();
> +	setup(t_addch, "a");
> +}
> +
> +void test_strbuf__add_empty_char(void)
> +{
> +	setup(t_addch, "");
> +}
> +
> +void test_strbuf__add_append_char(void)
> +{
> +	setup_populated(t_addch, "initial value", "a");
> +}
> +
> +void test_strbuf__add_single_str(void)
> +{
> +	setup(t_addstr, "hello there");
> +}
> +
> +void test_strbuf__add_append_str(void)
> +{
> +	setup_populated(t_addstr, "initial value", "hello there");
>   }
Previous: Seyi KuforijiNext: Seyi Kuforiji
Message 19 of 21 in “t/unit-tests: convert unit-tests to use clar”
  1. 0/4 t/unit-tests: convert unit-tests to use clarSeyi Kuforiji, Jan 30, 2025
  2. 1/4 t/unit-tests: convert hashmap test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  3. Patrick SteinhardtJan 31, 2025
  4. 2/4 t/unit-tests: adapt example decorate test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  5. Patrick SteinhardtJan 31, 2025
  6. 3/4 t/unit-tests: convert strbuf test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  7. Patrick SteinhardtJan 31, 2025
  8. 4/4 t/unit-tests: convert strcmp-offset test to clar frameworkSeyi Kuforiji, Jan 30, 2025
  9. Patrick SteinhardtJan 31, 2025
  10. Patrick SteinhardtJan 31, 2025
  11. 0/4 t/unit-tests: convert unit-tests to use clarSeyi Kuforiji, Jan 31, 2025
  12. 1/4 t/unit-tests: convert hashmap test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  13. Junio C HamanoJan 31, 2025
  14. phillip.wood123@gmail.comFeb 2, 2025
  15. Patrick SteinhardtFeb 3, 2025
  16. phillip.wood123@gmail.comFeb 3, 2025
  17. 2/4 t/unit-tests: adapt example decorate test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  18. 3/4 t/unit-tests: convert strbuf test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  19. Phillip WoodFeb 2, 2025
  20. 4/4 t/unit-tests: convert strcmp-offset test to use clar test frameworkSeyi Kuforiji, Jan 31, 2025
  21. Junio C HamanoJan 31, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.