Re: [PATCH v4 2/2] t/: migrate helper/test-{sha1, sha256} to unit-tests/t-hash

From: Patrick Steinhardt <ps@pks.im>
Date: May 29, 2024, 06:26 UTC
Message-ID: <ZlbKkMfmWFw59aO8@tanuki>
In-Reply-To: <20240526084345.24138-3-shyamthakkar001@gmail.com>

On Sun, May 26, 2024 at 02:13:45PM +0530, Ghanshyam Thakkar wrote:

Show 20 quoted lines

> -test_done
> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> new file mode 100644
> index 0000000000..bf3e0e9e94
> --- /dev/null
> +++ b/t/unit-tests/t-hash.c
> @@ -0,0 +1,86 @@
> +#include "test-lib.h"
> +#include "hex.h"
> +#include "strbuf.h"
> +
> +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;
> +	}
> +
> +	for (int i = 1; i < ARRAY_SIZE(hash_algos); i++) {

s/int/size_t/

Show 16 quoted lines

> +		git_hash_ctx ctx;
> +		unsigned char hash[GIT_MAX_HEXSZ];
> +		const struct git_hash_algo *algop = &hash_algos[i];
> +
> +		algop->init_fn(&ctx);
> +		algop->update_fn(&ctx, data, data_length);
> +		algop->final_fn(hash, &ctx);
> +
> +		if (!check_str(hash_to_hex_algop(hash, algop), expected_hashes[i - 1]))
> +			test_msg("result does not match with the expected for %s\n", hash_algos[i].name);
> +	}
> +}
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL character. */
> +#define TEST_HASH_STR(data, expected_sha1, expected_sha256) \
> +	{ \

These macros should like start with `do {`. The reason why we do this is that the compiler will complain if there is no semicolon after the macro.

Show 24 quoted lines

> +		const char *expected_hashes[] = { expected_sha1, expected_sha256 }; \
> +		TEST(check_hash_data(data, strlen(data), expected_hashes), \
> +		     "SHA1 and SHA256 (%s) works", #data); \
> +	}
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_HASH_LITERAL(literal, expected_sha1, expected_sha256) \
> +	{ \
> +		const char *expected_hashes[] = { expected_sha1, expected_sha256 }; \
> +		TEST(check_hash_data(literal, (sizeof(literal) - 1), expected_hashes), \
> +		     "SHA1 and SHA256 (%s) works", #literal); \
> +	}
> +
> +int cmd_main(int argc, const char **argv)
> +{
> +	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");

I think these might've been a bit easier to read if they formatted like this:

	TEST_HASH_STR("",
	    "da39a3ee5e6b4b0d3255bfef95601890afd80709",
	    "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");

	TEST_HASH_STR("a",
	    "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8",
	    "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");

It makes it a bit easier to separate the test data from the gibberish that those hashes are.

Other than that these patches look good to me.

Patrick