diff mbox series

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

Message ID 20250109140952.5267-1-kuforiji98@gmail.com (mailing list archive)
State New
Headers show
Series [v3] t/unit-tests: convert hash to use clar test framework | expand

Commit Message

Seyi Kuforiji Jan. 9, 2025, 2:09 p.m. UTC 
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>
---
Changes relative to v2:

  - A couple of fixes to code formatting to match our standards 

Thanks
Seyi
---
Range-diff against v2:
-:  ---------- > 1:  fcc2a376a5 t/unit-tests: convert hash to use clar test framework

 Makefile                            |  2 +-
 t/meson.build                       |  2 +-
 t/unit-tests/{t-hash.c => u-hash.c} | 71 +++++++++++++++++++----------
 3 files changed, 50 insertions(+), 25 deletions(-)
 rename t/unit-tests/{t-hash.c => u-hash.c} (80%)

Comments

Patrick Steinhardt Jan. 9, 2025, 3:10 p.m. UTC | #1 
On Thu, Jan 09, 2025 at 03:09:52PM +0100, Seyi Kuforiji wrote:
> Range-diff against v2:
> -:  ---------- > 1:  fcc2a376a5 t/unit-tests: convert hash to use clar test framework

Hm. The range-diff is a bit funny -- I would have expected it to
recognize that it's the same patch as not a lot of things have changed
compared to v2. Did you maybe compare to v1 by accident?

Anyway, this is of course no reason to send a revised version. The
changes themselves look good to me, thanks!

Partick
Junio C Hamano Jan. 9, 2025, 4:14 p.m. UTC | #2 
Seyi Kuforiji <kuforiji98@gmail.com> writes:

> 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., ...

>  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 ...

>  #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.

> -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.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 97e8385b66..d3011e30f7 100644
--- a/Makefile
+++ b/Makefile
@@ -1338,6 +1338,7 @@  THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/%
 THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
 
 CLAR_TEST_SUITES += u-ctype
+CLAR_TEST_SUITES += u-hash
 CLAR_TEST_SUITES += u-strvec
 CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
 CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES))
@@ -1345,7 +1346,6 @@  CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
 CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/unit-test.o
 
 UNIT_TEST_PROGRAMS += t-example-decorate
-UNIT_TEST_PROGRAMS += t-hash
 UNIT_TEST_PROGRAMS += t-hashmap
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-oid-array
diff --git a/t/meson.build b/t/meson.build
index 602ebfe6a2..7b35eadbc8 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,5 +1,6 @@ 
 clar_test_suites = [
   'unit-tests/u-ctype.c',
+  'unit-tests/u-hash.c',
   'unit-tests/u-strvec.c',
 ]
 
@@ -41,7 +42,6 @@  test('unit-tests', clar_unit_tests)
 
 unit_test_programs = [
   'unit-tests/t-example-decorate.c',
-  'unit-tests/t-hash.c',
   'unit-tests/t-hashmap.c',
   'unit-tests/t-mem-pool.c',
   'unit-tests/t-oid-array.c',
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/u-hash.c
similarity index 80%
rename from t/unit-tests/t-hash.c
rename to t/unit-tests/u-hash.c
index e62647019b..a0320efe4b 100644
--- a/t/unit-tests/t-hash.c
+++ b/t/unit-tests/u-hash.c
@@ -1,14 +1,11 @@ 
-#include "test-lib.h"
+#include "unit-test.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;
-	}
+	cl_assert(data != NULL);
 
 	for (size_t i = 1; i < ARRAY_SIZE(hash_algos); i++) {
 		git_hash_ctx ctx;
@@ -19,66 +16,94 @@  static void check_hash_data(const void *data, size_t data_length,
 		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);
+		cl_assert_equal_s(hash_to_hex_algop(hash,algop), expected_hashes[i - 1]);
 	}
 }
 
 /* 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) 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); \
 	} while (0)
 
 /* Only works with a literal string, useful when it contains a NUL character. */
 #define TEST_HASH_LITERAL(literal, expected_sha1, expected_sha256) do { \
 		const char *expected_hashes[] = { expected_sha1, expected_sha256 }; \
-		TEST(check_hash_data(literal, (sizeof(literal) - 1), expected_hashes), \
-		     "SHA1 and SHA256 (%s) works", #literal); \
+		check_hash_data(literal, (sizeof(literal) - 1), expected_hashes); \
 	} while (0)
 
-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");
+}
+
+void test_hash__single_character(void)
+{
 	TEST_HASH_STR("a",
 		"86f7e437faa5a7fce15d1ddcb9eaeaea377667b8",
 		"ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
+}
+
+void test_hash__multi_character(void)
+{
 	TEST_HASH_STR("abc",
 		"a9993e364706816aba3e25717850c26c9cd0d89d",
 		"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
+}
+
+void test_hash__message_digest(void)
+{
 	TEST_HASH_STR("message digest",
 		"c12252ceda8be8994d5fa0290a47231c1d16aae3",
 		"f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
+}
+
+void test_hash__alphabet(void)
+{
 	TEST_HASH_STR("abcdefghijklmnopqrstuvwxyz",
 		"32d10c7b8cf96570ca04ce37f2a19d84240d3a89",
 		"71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
+}
+
+void test_hash__aaaaaaaaaa_100000(void)
+{
+	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
 	TEST_HASH_STR(aaaaaaaaaa_100000.buf,
 		"34aa973cd4c4daa4f61eeb2bdbad27316534016f",
 		"cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
+	strbuf_release(&aaaaaaaaaa_100000);
+}
+
+void test_hash__alphabet_100000(void)
+{
+	struct strbuf alphabet_100000 = STRBUF_INIT;
+	strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
 	TEST_HASH_STR(alphabet_100000.buf,
 		"e7da7c55b3484fdf52aebec9cbe7b85a98f02fd4",
 		"e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
+	strbuf_release(&alphabet_100000);
+}
+
+void test_hash__zero_blob_literal(void)
+{
 	TEST_HASH_LITERAL("blob 0\0",
 		"e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
 		"473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
+}
+
+void test_hash__three_blob_literal(void)
+{
 	TEST_HASH_LITERAL("blob 3\0abc",
 		"f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f",
 		"c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
+}
+
+void test_hash__zero_tree_literal(void)
+{
 	TEST_HASH_LITERAL("tree 0\0",
 		"4b825dc642cb6eb9a060e54bf8d69288fbee4904",
 		"6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
-
-	strbuf_release(&aaaaaaaaaa_100000);
-	strbuf_release(&alphabet_100000);
-
-	return test_done();
 }