threads / patch / 62755

patch, 2 partst/unit-tests: match functions signature with trailing code

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

## tl;dr

18 messages between Jan 7, 2025 and Jan 9, 2025. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Seyi Kuforiji· Jan 7, 2025, 09:19 UTC · lore

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

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

Seyi Kuforiji· Jan 7, 2025, 09:19 UTC · re: Seyi Kuforiji · lore

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

Show changes to t/unit-tests/generate-clar-decls.sh +1 −2

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

Junio C Hamano· Jan 7, 2025, 18:16 UTC · re: Seyi Kuforiji · lore

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

Seyi Kuforiji <kuforiji98@gmail.com> writes:

Show 14 quoted lines

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

Show 20 quoted lines

> 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

Junio C Hamano· Jan 7, 2025, 18:41 UTC · re: Junio C Hamano · lore

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

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

Show 24 quoted lines

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

Patrick Steinhardt· Jan 8, 2025, 06:14 UTC · re: Junio C Hamano · lore

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

On Tue, Jan 07, 2025 at 10:16:33AM -0800, Junio C Hamano wrote:

Show 56 quoted lines

> 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

Seyi Chamber· Jan 8, 2025, 08:31 UTC · re: Patrick Steinhardt · lore

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

On Wed, 8 Jan 2025 at 07:14, Patrick Steinhardt <ps@pks.im> wrote:

Show 71 quoted lines

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

Junio C Hamano· Jan 8, 2025, 15:27 UTC · re: Patrick Steinhardt · lore

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

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

Patrick Steinhardt· Jan 8, 2025, 16:15 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 08, 2025 at 07:27:37AM -0800, Junio C Hamano wrote:

Show 27 quoted lines

> 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

Seyi Kuforiji· Jan 7, 2025, 09:19 UTC · re: Seyi Kuforiji · lore

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

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

Show changes to 4 files +87 −86

Makefile, t/meson.build, t/unit-tests/t-hash.c, 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

Seyi Kuforiji· Jan 8, 2025, 12:03 UTC · re: Seyi Kuforiji · lore

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

Hello,

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

Seyi Kuforiji· Jan 8, 2025, 12:03 UTC · re: Seyi Kuforiji · lore

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

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

Show changes to 4 files +52 −27

Makefile, t/meson.build, t/unit-tests/t-hash.c, 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/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

Junio C Hamano· Jan 8, 2025, 15:35 UTC · re: Seyi Kuforiji · lore

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

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.

Show 10 quoted lines

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

Show 12 quoted lines

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

Show 8 quoted lines

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

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

Seyi Chamber· Jan 9, 2025, 07:30 UTC · re: Junio C Hamano · lore

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

On Wed, 8 Jan 2025 at 16:35, Junio C Hamano <gitster@pobox.com> wrote:

Show 24 quoted lines

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

Show 53 quoted lines

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

Junio C Hamano· Jan 8, 2025, 15:28 UTC · re: Seyi Kuforiji · lore

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

Seyi Kuforiji <kuforiji98@gmail.com> writes:

Show 5 quoted lines

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

Seyi Chamber· Jan 9, 2025, 07:21 UTC · re: Junio C Hamano · lore

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

On Wed, 8 Jan 2025 at 16:28, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

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

Seyi Kuforiji· Jan 9, 2025, 14:09 UTC · re: Seyi Kuforiji · lore

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

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

Show changes to 4 files +50 −25

Makefile, t/meson.build, t/unit-tests/t-hash.c, 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..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

Patrick Steinhardt· Jan 9, 2025, 15:10 UTC · re: Seyi Kuforiji · lore

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

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, 16:14 UTC · re: Seyi Kuforiji · lore

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

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.