Re: [PATCH v3] t/unit-tests: convert hash to use clar test framework

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2025, 16:14 UTC
Message-ID: <xmqqmsg0j762.fsf@gitster.g>
In-Reply-To: <20250109140952.5267-1-kuforiji98@gmail.com>

Seyi Kuforiji <kuforiji98@gmail.com> writes:

Show 8 quoted lines

> Adapt the hash test functions to clar framework by using clar
> assertions where necessary. Following the consensus to convert
> the unit-tests scripts found in the t/unit-tests folder to clar driven by
> Patrick Steinhardt. Test functions are structured as a standalone to
> test individual hash string and literal case.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>

The change to the test program was a very pleasant read. It was trivially obvious that the new version faithfully rewrites the original. E.g., ...

Show 8 quoted lines

>  static void check_hash_data(const void *data, size_t data_length,
>  			    const char *expected_hashes[])
>  {
> -	if (!check(data != NULL)) {
> -		test_msg("BUG: NULL data pointer provided");
> -		return;
> -	}
> +	cl_assert(data != NULL);

... instead of using check() and giving message with test_msg(), the clar framework gives cl_assert() for us to use.

And ...

Show 5 quoted lines

>  #define TEST_HASH_STR(data, expected_sha1, expected_sha256) do { \
>  		const char *expected_hashes[] = { expected_sha1, expected_sha256 }; \
> -		TEST(check_hash_data(data, strlen(data), expected_hashes), \
> -		     "SHA1 and SHA256 (%s) works", #data); \
> +		check_hash_data(data, strlen(data), expected_hashes); \

... instead of TEST() macro with the title string, we call the underlying test function. The loss of the message does not hurt us, as both the test suite name and name of each test are shown by the clar framework.

Show 13 quoted lines

> -int cmd_main(int argc UNUSED, const char **argv UNUSED)
> +void test_hash__empty_string(void)
>  {
> -	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
> -	struct strbuf alphabet_100000 = STRBUF_INIT;
> -
> -	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
> -	strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
> -
>  	TEST_HASH_STR("",
>  		"da39a3ee5e6b4b0d3255bfef95601890afd80709",
>  		"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
> +}

And the fact that the body of each test function are unchanged from the original helps to build confidence in the faithfulness of the conversion.

The strbuf allocation is lost from here, and clean-up is lost from the end of the file, and they are done in the function that needs the strbuf, which also contributes to the clarity of the new version.

Nicely done.  Will queue.

Thanks.