# [PATCH 1/2] t/unit-tests: match functions signature with trailing code 18 messages from 2025-01-07 to 2025-01-09. Participants: Seyi Kuforiji, Junio C Hamano, Patrick Steinhardt, Seyi Chamber. Thread: https://gitlist.dev/t/62755 ## Seyi Kuforiji, 2025-01-07 09:19 Subject: [PATCH 0/2] t/unit-tests: convert hash tests to use clar Message-ID: <20250107091932.126673-1-kuforiji98@gmail.com> URL: https://gitlist.dev/e/20250107091932.126673-1-kuforiji98%40gmail.com ``` 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, 2025-01-07 09:19 Subject: [PATCH 1/2] t/unit-tests: match functions signature with trailing code Message-ID: <20250107091932.126673-2-kuforiji98@gmail.com> URL: https://gitlist.dev/e/20250107091932.126673-2-kuforiji98%40gmail.com In-Reply-To: <20250107091932.126673-1-kuforiji98@gmail.com> ``` 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 Signed-off-by: Seyi Kuforiji --- 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 ``` ## Seyi Kuforiji, 2025-01-07 09:19 Subject: [PATCH 2/2] t/unit-tests: convert hash to use clar test framework Message-ID: <20250107091932.126673-3-kuforiji98@gmail.com> URL: https://gitlist.dev/e/20250107091932.126673-3-kuforiji98%40gmail.com In-Reply-To: <20250107091932.126673-1-kuforiji98@gmail.com> ``` 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 Signed-off-by: Seyi Kuforiji --- 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 ``` ## Junio C Hamano, 2025-01-07 18:16 Subject: Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code Message-ID: URL: https://gitlist.dev/e/xmqq7c76trpa.fsf%40gitster.g In-Reply-To: <20250107091932.126673-2-kuforiji98@gmail.com> ``` Seyi Kuforiji 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 > Signed-off-by: Seyi Kuforiji > --- > 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, 2025-01-07 18:41 Subject: Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code Message-ID: URL: https://gitlist.dev/e/xmqqv7uqsc03.fsf%40gitster.g In-Reply-To: ``` Junio C Hamano 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. ``` ## Patrick Steinhardt, 2025-01-08 06:14 Subject: Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code Message-ID: URL: https://gitlist.dev/e/Z34XvPjhY15MFHrT%40pks.im In-Reply-To: ``` On Tue, Jan 07, 2025 at 10:16:33AM -0800, Junio C Hamano wrote: > Seyi Kuforiji 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, 2025-01-08 08:31 Subject: Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code Message-ID: URL: https://gitlist.dev/e/CAGedMtcNpYBRsr9b8-ftbs6JFdp58wNGyYssiF1s0MEmziDtxw%40mail.gmail.com In-Reply-To: ``` On Wed, 8 Jan 2025 at 07:14, Patrick Steinhardt wrote: > > On Tue, Jan 07, 2025 at 10:16:33AM -0800, Junio C Hamano wrote: > > Seyi Kuforiji 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 ``` ## Seyi Kuforiji, 2025-01-08 12:03 Subject: [PATCH v2 0/1] t/unit-tests: convert hash to use clar test framework Message-ID: <20250108120339.225596-1-kuforiji98@gmail.com> URL: https://gitlist.dev/e/20250108120339.225596-1-kuforiji98%40gmail.com In-Reply-To: <20250107091932.126673-1-kuforiji98@gmail.com> ``` 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 ``` ## Seyi Kuforiji, 2025-01-08 12:03 Subject: [PATCH v2 1/1] t/unit-tests: convert hash to use clar test framework Message-ID: <20250108120339.225596-2-kuforiji98@gmail.com> URL: https://gitlist.dev/e/20250108120339.225596-2-kuforiji98%40gmail.com In-Reply-To: <20250108120339.225596-1-kuforiji98@gmail.com> ``` 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 Signed-off-by: Seyi Kuforiji --- 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 ``` ## Junio C Hamano, 2025-01-08 15:27 Subject: Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code Message-ID: URL: https://gitlist.dev/e/xmqqcygxpbpy.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt 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 ;-) ``` ## Junio C Hamano, 2025-01-08 15:28 Subject: Re: [PATCH v2 0/1] t/unit-tests: convert hash to use clar test framework Message-ID: URL: https://gitlist.dev/e/xmqq8qrlpbnx.fsf%40gitster.g In-Reply-To: <20250108120339.225596-1-kuforiji98@gmail.com> ``` Seyi Kuforiji 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 ;-) ``` ## Junio C Hamano, 2025-01-08 15:35 Subject: Re: [PATCH v2 1/1] t/unit-tests: convert hash to use clar test framework Message-ID: URL: https://gitlist.dev/e/xmqqv7upnwsp.fsf%40gitster.g In-Reply-To: <20250108120339.225596-2-kuforiji98@gmail.com> ``` Seyi Kuforiji 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. ``` ## Patrick Steinhardt, 2025-01-08 16:15 Subject: Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code Message-ID: URL: https://gitlist.dev/e/Z36kqlcKl20AkJvW%40pks.im In-Reply-To: ``` On Wed, Jan 08, 2025 at 07:27:37AM -0800, Junio C Hamano wrote: > Patrick Steinhardt 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 Chamber, 2025-01-09 07:21 Subject: Re: [PATCH v2 0/1] t/unit-tests: convert hash to use clar test framework Message-ID: URL: https://gitlist.dev/e/CAGedMtdJgM%2BvRryKLz5BKuQSBreHOc%2Bq%3DrexciPB2WZ4aT3%3DYQ%40mail.gmail.com In-Reply-To: ``` On Wed, 8 Jan 2025 at 16:28, Junio C Hamano wrote: > > Seyi Kuforiji 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 Chamber, 2025-01-09 07:30 Subject: Re: [PATCH v2 1/1] t/unit-tests: convert hash to use clar test framework Message-ID: URL: https://gitlist.dev/e/CAGedMtd8GxYHj_N9XRwFb11jyRxk4qFASoKHw8iU2KWMPfucoA%40mail.gmail.com In-Reply-To: ``` On Wed, 8 Jan 2025 at 16:35, Junio C Hamano wrote: > > Seyi Kuforiji 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 ``` ## Seyi Kuforiji, 2025-01-09 14:09 Subject: [PATCH v3] t/unit-tests: convert hash to use clar test framework Message-ID: <20250109140952.5267-1-kuforiji98@gmail.com> URL: https://gitlist.dev/e/20250109140952.5267-1-kuforiji98%40gmail.com In-Reply-To: <20250108120339.225596-1-kuforiji98@gmail.com> ``` 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 Signed-off-by: Seyi Kuforiji --- 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 ``` ## Patrick Steinhardt, 2025-01-09 15:10 Subject: Re: [PATCH v3] t/unit-tests: convert hash to use clar test framework Message-ID: URL: https://gitlist.dev/e/Z3_m5uNhXjdFyGJK%40pks.im In-Reply-To: <20250109140952.5267-1-kuforiji98@gmail.com> ``` 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, 2025-01-09 16:14 Subject: Re: [PATCH v3] t/unit-tests: convert hash to use clar test framework Message-ID: URL: https://gitlist.dev/e/xmqqmsg0j762.fsf%40gitster.g In-Reply-To: <20250109140952.5267-1-kuforiji98@gmail.com> ``` Seyi Kuforiji 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 > Signed-off-by: Seyi Kuforiji 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. ```