[PATCH 0/2] t/unit-tests: convert hash tests to use clar

[PATCH 0/2] t/unit-tests: convert hash tests to use clar

[Seyi Kuforiji]

Hello,

This small patch series transitions the existing unit test file t-hash.c
to the Clar testing framework. This change is part of our ongoing effort
to standardize our testing approach and enhance maintainability.

Thanks
Seyi

Mentored-by: Patrick Steinhardt ps@pks.im
Signed-off-by: Seyi Kuforiji kuforiji98@gmail.com

Seyi Kuforiji (2):
  t/unit-tests: match functions signature with trailing code
  t/unit-tests: convert hash to use clar test framework

 Makefile                            |  2 +-
 t/meson.build                       |  2 +-
 t/unit-tests/generate-clar-decls.sh |  2 +-
 t/unit-tests/t-hash.c               | 84 ----------------------------
 t/unit-tests/u-hash.c               | 85 +++++++++++++++++++++++++++++
 5 files changed, 88 insertions(+), 87 deletions(-)
 delete mode 100644 t/unit-tests/t-hash.c
 create mode 100644 t/unit-tests/u-hash.c

-- 
2.34.1

[PATCH 1/2] t/unit-tests: match functions signature with trailing code

[Seyi Kuforiji]

The `generate-clar-decls.sh` script extracts signatures of test
functions from our unit tests, which will later get used by the clar to
automatically wire up tests. The sed command only matches lines that
ended immediately after `void)`, causing it to miss declarations with
additional content such as comments or annotations.

Relax the regular expression by making it match lines with trailing data
after the function signature. This ensures that all valid function
declarations are captured and formatted as `extern` declarations
regardless of their formatting style, improving the robustness of the
script when parsing `$suite` files.

This will be used in subsequent commits to match and capture the
function signature correctly, regardless of any trailing content.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
---
 t/unit-tests/generate-clar-decls.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
index 3b315c64b3..02e45cf0ba 100755
--- a/t/unit-tests/generate-clar-decls.sh
+++ b/t/unit-tests/generate-clar-decls.sh
@@ -14,6 +14,6 @@ do
 	suite_name=$(basename "$suite")
 	suite_name=${suite_name%.c}
 	suite_name=${suite_name#u-}
-	sed -ne "s/^\(void test_${suite_name}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)\)$/extern \1;/p" "$suite" ||
+	sed -ne "s/^\(void test_${suite_name}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)\).*/extern \1;/p" "$suite" ||
 	exit 1
 done >"$OUTPUT"
--
2.34.1

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

[Seyi Kuforiji]

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>
---
 Makefile              |  2 +-
 t/meson.build         |  2 +-
 t/unit-tests/t-hash.c | 84 ------------------------------------------
 t/unit-tests/u-hash.c | 85 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 87 insertions(+), 86 deletions(-)
 delete mode 100644 t/unit-tests/t-hash.c
 create mode 100644 t/unit-tests/u-hash.c

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..d722bc7dff 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,6 +1,7 @@
 clar_test_suites = [
   'unit-tests/u-ctype.c',
   'unit-tests/u-strvec.c',
+  'unit-tests/u-hash.c',
 ]
 
 clar_sources = [
@@ -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/t-hash.c
deleted file mode 100644
index e62647019b..0000000000
--- a/t/unit-tests/t-hash.c
+++ /dev/null
@@ -1,84 +0,0 @@
-#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 (size_t i = 1; i < ARRAY_SIZE(hash_algos); i++) {
-		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) do { \
-		const char *expected_hashes[] = { expected_sha1, expected_sha256 }; \
-		TEST(check_hash_data(data, strlen(data), expected_hashes), \
-		     "SHA1 and SHA256 (%s) works", #data); \
-	} 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); \
-	} while (0)
-
-int cmd_main(int argc UNUSED, const char **argv UNUSED)
-{
-	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");
-	TEST_HASH_STR("a",
-		"86f7e437faa5a7fce15d1ddcb9eaeaea377667b8",
-		"ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
-	TEST_HASH_STR("abc",
-		"a9993e364706816aba3e25717850c26c9cd0d89d",
-		"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
-	TEST_HASH_STR("message digest",
-		"c12252ceda8be8994d5fa0290a47231c1d16aae3",
-		"f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
-	TEST_HASH_STR("abcdefghijklmnopqrstuvwxyz",
-		"32d10c7b8cf96570ca04ce37f2a19d84240d3a89",
-		"71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
-	TEST_HASH_STR(aaaaaaaaaa_100000.buf,
-		"34aa973cd4c4daa4f61eeb2bdbad27316534016f",
-		"cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
-	TEST_HASH_STR(alphabet_100000.buf,
-		"e7da7c55b3484fdf52aebec9cbe7b85a98f02fd4",
-		"e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
-	TEST_HASH_LITERAL("blob 0\0",
-		"e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
-		"473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
-	TEST_HASH_LITERAL("blob 3\0abc",
-		"f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f",
-		"c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
-	TEST_HASH_LITERAL("tree 0\0",
-		"4b825dc642cb6eb9a060e54bf8d69288fbee4904",
-		"6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
-
-	strbuf_release(&aaaaaaaaaa_100000);
-	strbuf_release(&alphabet_100000);
-
-	return test_done();
-}
diff --git a/t/unit-tests/u-hash.c b/t/unit-tests/u-hash.c
new file mode 100644
index 0000000000..c7bac69d03
--- /dev/null
+++ b/t/unit-tests/u-hash.c
@@ -0,0 +1,85 @@
+#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[])
+{
+	cl_assert(data != NULL);
+
+	for (size_t i = 1; i < ARRAY_SIZE(hash_algos); i++) {
+		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);
+
+		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) { \
+	const char *expected_hashes[] = { expected_sha1, expected_sha256 }; \
+	check_hash_data(data, strlen(data), expected_hashes); \
+}
+
+/* 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 }; \
+	check_hash_data(literal, (sizeof(literal) - 1), expected_hashes); \
+}
+
+void test_hash__empty_string(void) 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")
-- 
2.34.1

Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> The `generate-clar-decls.sh` script extracts signatures of test
> functions from our unit tests, which will later get used by the clar to
> automatically wire up tests. The sed command only matches lines that
> ended immediately after `void)`, causing it to miss declarations with
> additional content such as comments or annotations.
>
> Relax the regular expression by making it match lines with trailing data
> after the function signature. This ensures that all valid function
> declarations are captured and formatted as `extern` declarations
> regardless of their formatting style, improving the robustness of the
> script when parsing `$suite` files.
>
> This will be used in subsequent commits to match and capture the
> function signature correctly, regardless of any trailing content.

I am not sure if this is going in the right direction, though.

Especially for things like test suites that are looked at and worked
on only by develoeprs *and* these tools, being uniform and consistent
weighs more than being more flexible.

Let me state it in another way.  How many of the existing test
pieces are picked up by the current pattern, and among them how many
of them would see vast improvements if they are allowed to have
arbitrary garbage after their "I do not take any arguments" function
signature?  Are new tests you are migrating from outside the clar
world lose a lot if they are no longer allowed to have comments
there, or would it be suffice to have the comments before the
functions (which many of our function definition do anyway)?

A quick peek at [PATCH 2/2] tells me that this is not even something
that would make it easier to port the existing tests by allowing
more straight line-by-line copies or something.  The patch splits
many in-line test pieces in the "main" into separate functions, and
it does so in a rather unusual format, e.g.,

  void test_hash__multi_character(void) TEST_HASH_STR("abc",
          "a9993e364706816aba3e25717850c26c9cd0d89d",
          "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad")

where TEST_HASH_STR() expands to the function body that starts with
a "{" and ends with a "}".  It can well be written more like

    void test_hash__multi_character(void)
    {
	TEST_HASH_STR("abc",
        	"a9993e364706816aba3e25717850c26c9cd0d89d",
		"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
    }

and we do not need this step at all if we did so.  Such a construct
would be a lot friendlier to the editors that auto-indent, too.

So, I do not quite see much value in this particular change.

> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>  t/unit-tests/generate-clar-decls.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
> index 3b315c64b3..02e45cf0ba 100755
> --- a/t/unit-tests/generate-clar-decls.sh
> +++ b/t/unit-tests/generate-clar-decls.sh
> @@ -14,6 +14,6 @@ do
>  	suite_name=$(basename "$suite")
>  	suite_name=${suite_name%.c}
>  	suite_name=${suite_name#u-}
> -	sed -ne "s/^\(void test_${suite_name}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)\)$/extern \1;/p" "$suite" ||
> +	sed -ne "s/^\(void test_${suite_name}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)\).*/extern \1;/p" "$suite" ||
>  	exit 1
>  done >"$OUTPUT"
> --
> 2.34.1

Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> A quick peek at [PATCH 2/2] tells me that this is not even something
> that would make it easier to port the existing tests by allowing
> more straight line-by-line copies or something.  The patch splits
> many in-line test pieces in the "main" into separate functions, and
> it does so in a rather unusual format, e.g.,
>
>   void test_hash__multi_character(void) TEST_HASH_STR("abc",
>           "a9993e364706816aba3e25717850c26c9cd0d89d",
>           "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad")
>
> where TEST_HASH_STR() expands to the function body that starts with
> a "{" and ends with a "}".  It can well be written more like
>
>     void test_hash__multi_character(void)
>     {
> 	TEST_HASH_STR("abc",
>         	"a9993e364706816aba3e25717850c26c9cd0d89d",
> 		"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
>     }
>
> and we do not need this step at all if we did so.  Such a construct
> would be a lot friendlier to the editors that auto-indent, too.
>
> So, I do not quite see much value in this particular change.

Having said that, if this were more like that you write a series of

    DEF_HASH_TEST(multi_character, "abc", "a9993e...", "ba7816bf...")

and they expand to

    void test_hash__multi_character(void)
    {
	const char *expected[] = {"a9993e...", "ba7816bf..."};
	check_hash_data("abc", strlen("abc"), expected);
    }

then a preparatory step like this patch _might_ be justifiable.  You
may want to avoid having to write too many boilerplate, and a
special rule to find "DEF_HASH_TEST(name, ...)" and it might make
sense to add support to extract the name of the test function being
defined by the macro automatically.

Not that I think such a sequence of DEF_HASH_TEST(), one per line,
is an improvement at all (it also is unfriendly to editors that
auto-indent the same way as your original version).  I just wanted
to say that a change to the pattern to pick up the function name may
be justifiable if it were so.

Thanks.

Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

[Patrick Steinhardt]

On Tue, Jan 07, 2025 at 10:16:33AM -0800, Junio C Hamano wrote:
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
> 
> > The `generate-clar-decls.sh` script extracts signatures of test
> > functions from our unit tests, which will later get used by the clar to
> > automatically wire up tests. The sed command only matches lines that
> > ended immediately after `void)`, causing it to miss declarations with
> > additional content such as comments or annotations.
> >
> > Relax the regular expression by making it match lines with trailing data
> > after the function signature. This ensures that all valid function
> > declarations are captured and formatted as `extern` declarations
> > regardless of their formatting style, improving the robustness of the
> > script when parsing `$suite` files.
> >
> > This will be used in subsequent commits to match and capture the
> > function signature correctly, regardless of any trailing content.
> 
> I am not sure if this is going in the right direction, though.
> 
> Especially for things like test suites that are looked at and worked
> on only by develoeprs *and* these tools, being uniform and consistent
> weighs more than being more flexible.
> 
> Let me state it in another way.  How many of the existing test
> pieces are picked up by the current pattern, and among them how many
> of them would see vast improvements if they are allowed to have
> arbitrary garbage after their "I do not take any arguments" function
> signature?  Are new tests you are migrating from outside the clar
> world lose a lot if they are no longer allowed to have comments
> there, or would it be suffice to have the comments before the
> functions (which many of our function definition do anyway)?
> 
> A quick peek at [PATCH 2/2] tells me that this is not even something
> that would make it easier to port the existing tests by allowing
> more straight line-by-line copies or something.  The patch splits
> many in-line test pieces in the "main" into separate functions, and
> it does so in a rather unusual format, e.g.,
> 
>   void test_hash__multi_character(void) TEST_HASH_STR("abc",
>           "a9993e364706816aba3e25717850c26c9cd0d89d",
>           "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad")
> 
> where TEST_HASH_STR() expands to the function body that starts with
> a "{" and ends with a "}".  It can well be written more like
> 
>     void test_hash__multi_character(void)
>     {
> 	TEST_HASH_STR("abc",
>         	"a9993e364706816aba3e25717850c26c9cd0d89d",
> 		"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
>     }
> 
> and we do not need this step at all if we did so.  Such a construct
> would be a lot friendlier to the editors that auto-indent, too.
> 
> So, I do not quite see much value in this particular change.

Yeah. This was something I proposed, but I already mentioned to Seyi
that I'm not all that happy with the outcome as it has a couple of
downsides, for example broken syntax highlighting in lots of editors. I
said he can send that version to the mailing list anyway and get some
feedback on it to figure out whether my discomfort with my own idea is
warranted or not. And your comment here basically confirms that my idea
wasn't that great after all :)

So I agree with you, let's scrap the idea and have proper function
bodies instead.

Patrick

Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

[Seyi Chamber]

On Wed, 8 Jan 2025 at 07:14, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Jan 07, 2025 at 10:16:33AM -0800, Junio C Hamano wrote:
> > Seyi Kuforiji <kuforiji98@gmail.com> writes:
> >
> > > The `generate-clar-decls.sh` script extracts signatures of test
> > > functions from our unit tests, which will later get used by the clar to
> > > automatically wire up tests. The sed command only matches lines that
> > > ended immediately after `void)`, causing it to miss declarations with
> > > additional content such as comments or annotations.
> > >
> > > Relax the regular expression by making it match lines with trailing data
> > > after the function signature. This ensures that all valid function
> > > declarations are captured and formatted as `extern` declarations
> > > regardless of their formatting style, improving the robustness of the
> > > script when parsing `$suite` files.
> > >
> > > This will be used in subsequent commits to match and capture the
> > > function signature correctly, regardless of any trailing content.
> >
> > I am not sure if this is going in the right direction, though.
> >
> > Especially for things like test suites that are looked at and worked
> > on only by develoeprs *and* these tools, being uniform and consistent
> > weighs more than being more flexible.
> >
> > Let me state it in another way.  How many of the existing test
> > pieces are picked up by the current pattern, and among them how many
> > of them would see vast improvements if they are allowed to have
> > arbitrary garbage after their "I do not take any arguments" function
> > signature?  Are new tests you are migrating from outside the clar
> > world lose a lot if they are no longer allowed to have comments
> > there, or would it be suffice to have the comments before the
> > functions (which many of our function definition do anyway)?
> >
> > A quick peek at [PATCH 2/2] tells me that this is not even something
> > that would make it easier to port the existing tests by allowing
> > more straight line-by-line copies or something.  The patch splits
> > many in-line test pieces in the "main" into separate functions, and
> > it does so in a rather unusual format, e.g.,
> >
> >   void test_hash__multi_character(void) TEST_HASH_STR("abc",
> >           "a9993e364706816aba3e25717850c26c9cd0d89d",
> >           "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad")
> >
> > where TEST_HASH_STR() expands to the function body that starts with
> > a "{" and ends with a "}".  It can well be written more like
> >
> >     void test_hash__multi_character(void)
> >     {
> >       TEST_HASH_STR("abc",
> >               "a9993e364706816aba3e25717850c26c9cd0d89d",
> >               "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
> >     }
> >
> > and we do not need this step at all if we did so.  Such a construct
> > would be a lot friendlier to the editors that auto-indent, too.
> >
> > So, I do not quite see much value in this particular change.
>
> Yeah. This was something I proposed, but I already mentioned to Seyi
> that I'm not all that happy with the outcome as it has a couple of
> downsides, for example broken syntax highlighting in lots of editors. I
> said he can send that version to the mailing list anyway and get some
> feedback on it to figure out whether my discomfort with my own idea is
> warranted or not. And your comment here basically confirms that my idea
> wasn't that great after all :)
>
> So I agree with you, let's scrap the idea and have proper function
> bodies instead.
>
> Patrick

Thank you for the feedback and insights. I'll update the patch to use
standard function bodies.

Thanks
Seyi

[PATCH v2 0/1] t/unit-tests: convert hash to use clar test framework

[Seyi Kuforiji]

Hello,

This small patch series transitions the existing unit test file t-hash.c
to the Clar testing framework. This change is part of our ongoing effort
to standardize our testing approach and enhance maintainability.

changes in v2:
- Some small fixes were made to the macros and test functions
- Link to v1: https://public-inbox.org/git/20250107091932.126673-1-kuforiji98@gmail.com/T/#t

Thanks
Seyi

Seyi Kuforiji (1):
  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} | 75 +++++++++++++++++++----------
 3 files changed, 52 insertions(+), 27 deletions(-)
 rename t/unit-tests/{t-hash.c => u-hash.c} (79%)

Range-diff against v1:
-:  ---------- > 1:  6fde57893d t/unit-tests: convert hash to use clar test framework
-- 
2.34.1

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

[Seyi Kuforiji]

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>
---
 Makefile                            |  2 +-
 t/meson.build                       |  2 +-
 t/unit-tests/{t-hash.c => u-hash.c} | 75 +++++++++++++++++++----------
 3 files changed, 52 insertions(+), 27 deletions(-)
 rename t/unit-tests/{t-hash.c => u-hash.c} (79%)

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..d722bc7dff 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,6 +1,7 @@
 clar_test_suites = [
   'unit-tests/u-ctype.c',
   'unit-tests/u-strvec.c',
+  'unit-tests/u-hash.c',
 ]
 
 clar_sources = [
@@ -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 79%
rename from t/unit-tests/t-hash.c
rename to t/unit-tests/u-hash.c
index e62647019b..e5e2d2e033 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); \
-	} while (0)
+		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); \
-	} while (0)
+		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();
 }
-- 
2.34.1

Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> So I agree with you, let's scrap the idea and have proper function
> bodies instead.

Yup, sometimes, simple, stupid, and good enough is the way to go.

We could do

-- >8 --

#define T(testname, input, expect1, expect256) \
	void test_hash__ ## testname(void) \
	{ \
		const char *expect[] = { expect1, expect256 }; \
		check_hash_data(input, strlen(input), expect); \
	} extern void test_hash__ ## testname()

T(empty_string, "", "da39...", "e3b0c4...");
T(single_character, "a", "86f7e4...", "ca97811...");

-- 8< --

which may not upset syntax-aware editors too much.

Unless there are more than several dozens of them, I do not think it
is worth it, though ;-)

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

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

> Hello,
>
> This small patch series transitions the existing unit test file t-hash.c
> to the Clar testing framework. This change is part of our ongoing effort
> to standardize our testing approach and enhance maintainability.

Thanks; this is no longer a series but a single patch ;-)

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

[Junio C Hamano]

Seyi Kuforiji <kuforiji98@gmail.com> writes:

>  CLAR_TEST_SUITES += u-ctype
> +CLAR_TEST_SUITES += u-hash
>  CLAR_TEST_SUITES += u-strvec

This is inserted in the middle, presumably because a list without
inherent ordering is by default maintained as a lexicographically
sorted list.

> diff --git a/t/meson.build b/t/meson.build
> index 602ebfe6a2..d722bc7dff 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -1,6 +1,7 @@
>  clar_test_suites = [
>    'unit-tests/u-ctype.c',
>    'unit-tests/u-strvec.c',
> +  'unit-tests/u-hash.c',
>  ]

So, shouldn't we do the same here?

> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/u-hash.c
> similarity index 79%
> rename from t/unit-tests/t-hash.c
> rename to t/unit-tests/u-hash.c
> ...
>  #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); \
> -	} while (0)
> +		check_hash_data(data, strlen(data), expected_hashes); \
> +	} while(0)

Unwanted droppage of SP between "while" and "(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); \
> -	} while (0)
> +		check_hash_data(literal, (sizeof(literal) - 1), expected_hashes); \
> +	} while(0)

Ditto.

> -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");
> +}

OK.

I'll skip the rest as I expect they would be faithful conversions.

Thanks.

Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

[Patrick Steinhardt]

On Wed, Jan 08, 2025 at 07:27:37AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > So I agree with you, let's scrap the idea and have proper function
> > bodies instead.
> 
> Yup, sometimes, simple, stupid, and good enough is the way to go.
> 
> We could do
> 
> -- >8 --
> 
> #define T(testname, input, expect1, expect256) \
> 	void test_hash__ ## testname(void) \
> 	{ \
> 		const char *expect[] = { expect1, expect256 }; \
> 		check_hash_data(input, strlen(input), expect); \
> 	} extern void test_hash__ ## testname()
> 
> T(empty_string, "", "da39...", "e3b0c4...");
> T(single_character, "a", "86f7e4...", "ca97811...");
> 
> -- 8< --
> 
> which may not upset syntax-aware editors too much.
> 
> Unless there are more than several dozens of them, I do not think it
> is worth it, though ;-)

Agreed, I'd go with the simple solution for now, which is to have
function bodies.

Patrick

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

[Seyi Chamber]

On Wed, 8 Jan 2025 at 16:28, Junio C Hamano <gitster@pobox.com> wrote:
>
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
>
> > Hello,
> >
> > This small patch series transitions the existing unit test file t-hash.c
> > to the Clar testing framework. This change is part of our ongoing effort
> > to standardize our testing approach and enhance maintainability.
>
> Thanks; this is no longer a series but a single patch ;-)

You're absolutely right.Thank you for pointing that out :), I'll make
sure to update my phrasing in future submissions.

Thanks
Seyi

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

[Seyi Chamber]

On Wed, 8 Jan 2025 at 16:35, Junio C Hamano <gitster@pobox.com> wrote:
>
> Seyi Kuforiji <kuforiji98@gmail.com> writes:
>
> >  CLAR_TEST_SUITES += u-ctype
> > +CLAR_TEST_SUITES += u-hash
> >  CLAR_TEST_SUITES += u-strvec
>
> This is inserted in the middle, presumably because a list without
> inherent ordering is by default maintained as a lexicographically
> sorted list.
>
> > diff --git a/t/meson.build b/t/meson.build
> > index 602ebfe6a2..d722bc7dff 100644
> > --- a/t/meson.build
> > +++ b/t/meson.build
> > @@ -1,6 +1,7 @@
> >  clar_test_suites = [
> >    'unit-tests/u-ctype.c',
> >    'unit-tests/u-strvec.c',
> > +  'unit-tests/u-hash.c',
> >  ]
>
> So, shouldn't we do the same here?
>

Yes, we should. I'll make the necessary adjustment in the next
iteration. Thanks!

> > diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/u-hash.c
> > similarity index 79%
> > rename from t/unit-tests/t-hash.c
> > rename to t/unit-tests/u-hash.c
> > ...
> >  #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); \
> > -     } while (0)
> > +             check_hash_data(data, strlen(data), expected_hashes); \
> > +     } while(0)
>
> Unwanted droppage of SP between "while" and "(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); \
> > -     } while (0)
> > +             check_hash_data(literal, (sizeof(literal) - 1), expected_hashes); \
> > +     } while(0)
>
> Ditto.
>
> > -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");
> > +}
>
> OK.
>
> I'll skip the rest as I expect they would be faithful conversions.
>
> Thanks.
>

I'll make the required changes, thank you for pointing them out.

Thanks
Seyi

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

[Seyi Kuforiji]

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%)

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();
 }
-- 
2.34.1

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

[Patrick Steinhardt]

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

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

[Junio C Hamano]

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.