Re: [PATCH v3 3/3] t/: port helper/test-sha256.c to unit-tests/t-hash.c

From: Patrick Steinhardt <ps@pks.im>
Date: May 24, 2024, 13:30 UTC
Message-ID: <ZlCWeCJq6qxWrJvI@tanuki>
In-Reply-To: <20240523235945.26833-4-shyamthakkar001@gmail.com>

On Fri, May 24, 2024 at 05:29:45AM +0530, Ghanshyam Thakkar wrote:

Show 9 quoted lines

> t/helper/test-sha256 and t/t0015-hash test the hash implementation of
> SHA-256 in Git with basic SHA-256 hash values. Port them to the new
> unit testing framework for better debugging, simplicity and faster
> runtime. The necessary building blocks are already implemented in
> t-hash in the previous commit which ported test-sha1.
> 
> The 'sha256' subcommand of test-tool is still not removed, because it
> is used by pack_trailer() in lib-pack.sh, which is used in many tests
> of the t53** series.

Similar question here, are there replacements we can use for it? I also couldn't see it being used in any test other than t0015 when searing for "test-tool sha256". Maybe I'm looking for the wrong thing?

[snip]

Show 20 quoted lines

> -test_done
> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> index 89dfea9cc1..0f86cd3730 100644
> --- a/t/unit-tests/t-hash.c
> +++ b/t/unit-tests/t-hash.c
> @@ -32,11 +32,24 @@ static void check_hash_data(const void *data, size_t data_length,
>  	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
>  	     "SHA1 (%s) works", #literal)
>  
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL  character. */
> +#define TEST_SHA256_STR(data, expected) \
> +	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
> +	     "SHA256 (%s) works", #data)
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_SHA256_LITERAL(literal, expected) \
> +	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
> +	     "SHA256 (%s) works", #literal)
> +

Same question here regarding the macros and whether we can merge them.

Also, we do have the same test data for both hashes, and if we ever grow another hash it's likely that we'll also want to check for the same inputs there. Would it make sense to have a generic `TAST_HASHES()` macro where you give the input and then both the expected SHA1 and SHA256 to avoid some duplication?

Patrick