diff mbox series

[v3,3/3] t/: port helper/test-sha256.c to unit-tests/t-hash.c

Message ID 20240523235945.26833-4-shyamthakkar001@gmail.com (mailing list archive)
State New
Headers show
Series Port t0015-hash to the unit testing framework | expand

Commit Message

Ghanshyam Thakkar May 23, 2024, 11:59 p.m. UTC 
t/helper/test-sha256 and t/t0015-hash test the hash implementation of
SHA-256 in Git with basic SHA-256 hash values. Port them to the new
unit testing framework for better debugging, simplicity and faster
runtime. The necessary building blocks are already implemented in
t-hash in the previous commit which ported test-sha1.

The 'sha256' subcommand of test-tool is still not removed, because it
is used by pack_trailer() in lib-pack.sh, which is used in many tests
of the t53** series.

Helped-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Co-authored-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t0015-hash.sh       | 34 ----------------------------------
 t/unit-tests/t-hash.c | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 34 deletions(-)
 delete mode 100755 t/t0015-hash.sh

Comments

Patrick Steinhardt May 24, 2024, 1:30 p.m. UTC | #1 
On Fri, May 24, 2024 at 05:29:45AM +0530, Ghanshyam Thakkar wrote:
> t/helper/test-sha256 and t/t0015-hash test the hash implementation of
> SHA-256 in Git with basic SHA-256 hash values. Port them to the new
> unit testing framework for better debugging, simplicity and faster
> runtime. The necessary building blocks are already implemented in
> t-hash in the previous commit which ported test-sha1.
> 
> The 'sha256' subcommand of test-tool is still not removed, because it
> is used by pack_trailer() in lib-pack.sh, which is used in many tests
> of the t53** series.

Similar question here, are there replacements we can use for it? I also
couldn't see it being used in any test other than t0015 when searing for
"test-tool sha256". Maybe I'm looking for the wrong thing?

[snip]
> -test_done
> diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> index 89dfea9cc1..0f86cd3730 100644
> --- a/t/unit-tests/t-hash.c
> +++ b/t/unit-tests/t-hash.c
> @@ -32,11 +32,24 @@ static void check_hash_data(const void *data, size_t data_length,
>  	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
>  	     "SHA1 (%s) works", #literal)
>  
> +
> +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL  character. */
> +#define TEST_SHA256_STR(data, expected) \
> +	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
> +	     "SHA256 (%s) works", #data)
> +
> +/* Only works with a literal string, useful when it contains a NUL character. */
> +#define TEST_SHA256_LITERAL(literal, expected) \
> +	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
> +	     "SHA256 (%s) works", #literal)
> +

Same question here regarding the macros and whether we can merge them.

Also, we do have the same test data for both hashes, and if we ever grow
another hash it's likely that we'll also want to check for the same
inputs there. Would it make sense to have a generic `TAST_HASHES()`
macro where you give the input and then both the expected SHA1 and
SHA256 to avoid some duplication?

Patrick
Ghanshyam Thakkar May 25, 2024, 1:15 a.m. UTC | #2 
On Fri, 24 May 2024, Patrick Steinhardt <ps@pks.im> wrote:
> On Fri, May 24, 2024 at 05:29:45AM +0530, Ghanshyam Thakkar wrote:
> > t/helper/test-sha256 and t/t0015-hash test the hash implementation of
> > SHA-256 in Git with basic SHA-256 hash values. Port them to the new
> > unit testing framework for better debugging, simplicity and faster
> > runtime. The necessary building blocks are already implemented in
> > t-hash in the previous commit which ported test-sha1.
> > 
> > The 'sha256' subcommand of test-tool is still not removed, because it
> > is used by pack_trailer() in lib-pack.sh, which is used in many tests
> > of the t53** series.
> 
> Similar question here, are there replacements we can use for it? I also
> couldn't see it being used in any test other than t0015 when searing for
> "test-tool sha256". Maybe I'm looking for the wrong thing?

It is used indirectly and not explicitly like 'test-tool sha256'. e.g.
in t/lib-pack.sh when GIT_TEST_DEFUALT_HASH=sha256, ...

  # Compute and append pack trailer to "$1"
  pack_trailer () {
	  test-tool $(test_oid algo) -b <"$1" >trailer.tmp &&
	  cat trailer.tmp >>"$1" &&
	  rm -f trailer.tmp
  }

...it will use 'test-tool sha256' on the first line of pack_trailer().
And the pack_trailer() is used in t7450, t5308, t5309, t5321.

And I will consult with Christian and Kaartic on the replacements but as
Christian and Junio said, doing it in another series would be a good
idea.

> [snip]
> > -test_done
> > diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
> > index 89dfea9cc1..0f86cd3730 100644
> > --- a/t/unit-tests/t-hash.c
> > +++ b/t/unit-tests/t-hash.c
> > @@ -32,11 +32,24 @@ static void check_hash_data(const void *data, size_t data_length,
> >  	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
> >  	     "SHA1 (%s) works", #literal)
> >  
> > +
> > +/* Works with a NUL terminated string. Doesn't work if it should contain a NUL  character. */
> > +#define TEST_SHA256_STR(data, expected) \
> > +	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
> > +	     "SHA256 (%s) works", #data)
> > +
> > +/* Only works with a literal string, useful when it contains a NUL character. */
> > +#define TEST_SHA256_LITERAL(literal, expected) \
> > +	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
> > +	     "SHA256 (%s) works", #literal)
> > +
> 
> Same question here regarding the macros and whether we can merge them.
> 
> Also, we do have the same test data for both hashes, and if we ever grow
> another hash it's likely that we'll also want to check for the same
> inputs there. Would it make sense to have a generic `TAST_HASHES()`
> macro where you give the input and then both the expected SHA1 and
> SHA256 to avoid some duplication?

Yeah, that can be done as the inputs are same for both hashes minus one
extra for sha256 (though I think it can be easily obtained). It looks
like a good idea to me for v4.

Thank you for the review!
diff mbox series

Patch

diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
deleted file mode 100755
index 2264e702d5..0000000000
--- a/t/t0015-hash.sh
+++ /dev/null
@@ -1,34 +0,0 @@ 
-#!/bin/sh
-
-test_description='test basic hash implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'test basic SHA-256 hash values' '
-	test-tool sha256 </dev/null >actual &&
-	grep e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 actual &&
-	printf "a" | test-tool sha256 >actual &&
-	grep ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb actual &&
-	printf "abc" | test-tool sha256 >actual &&
-	grep ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad actual &&
-	printf "message digest" | test-tool sha256 >actual &&
-	grep f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650 actual &&
-	printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha256 >actual &&
-	grep 71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73 actual &&
-	# Try to exercise the chunking code by turning autoflush on.
-	perl -e "$| = 1; print q{aaaaaaaaaa} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
-	perl -e "$| = 1; print q{abcdefghijklmnopqrstuvwxyz} for 1..100000;" |
-		test-tool sha256 >actual &&
-	grep e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35 actual &&
-	printf "blob 0\0" | test-tool sha256 >actual &&
-	grep 473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813 actual &&
-	printf "blob 3\0abc" | test-tool sha256 >actual &&
-	grep c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6 actual &&
-	printf "tree 0\0" | test-tool sha256 >actual &&
-	grep 6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321 actual
-'
-
-test_done
diff --git a/t/unit-tests/t-hash.c b/t/unit-tests/t-hash.c
index 89dfea9cc1..0f86cd3730 100644
--- a/t/unit-tests/t-hash.c
+++ b/t/unit-tests/t-hash.c
@@ -32,11 +32,24 @@  static void check_hash_data(const void *data, size_t data_length,
 	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA1), \
 	     "SHA1 (%s) works", #literal)
 
+
+/* Works with a NUL terminated string. Doesn't work if it should contain a NUL  character. */
+#define TEST_SHA256_STR(data, expected) \
+	TEST(check_hash_data(data, strlen(data), expected, GIT_HASH_SHA256), \
+	     "SHA256 (%s) works", #data)
+
+/* Only works with a literal string, useful when it contains a NUL character. */
+#define TEST_SHA256_LITERAL(literal, expected) \
+	TEST(check_hash_data(literal, (sizeof(literal) - 1), expected, GIT_HASH_SHA256), \
+	     "SHA256 (%s) works", #literal)
+
 int cmd_main(int argc, const char **argv)
 {
 	struct strbuf aaaaaaaaaa_100000 = STRBUF_INIT;
+	struct strbuf alphabet_100000 = STRBUF_INIT;
 
 	strbuf_addstrings(&aaaaaaaaaa_100000, "aaaaaaaaaa", 100000);
+	strbuf_addstrings(&alphabet_100000, "abcdefghijklmnopqrstuvwxyz", 100000);
 
 	TEST_SHA1_STR("", "da39a3ee5e6b4b0d3255bfef95601890afd80709");
 	TEST_SHA1_STR("a", "86f7e437faa5a7fce15d1ddcb9eaeaea377667b8");
@@ -48,7 +61,19 @@  int cmd_main(int argc, const char **argv)
 	TEST_SHA1_LITERAL("blob 3\0abc", "f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f");
 	TEST_SHA1_LITERAL("tree 0\0", "4b825dc642cb6eb9a060e54bf8d69288fbee4904");
 
+	TEST_SHA256_STR("", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
+	TEST_SHA256_STR("a", "ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb");
+	TEST_SHA256_STR("abc", "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
+	TEST_SHA256_STR("message digest", "f7846f55cf23e14eebeab5b4e1550cad5b509e3348fbc4efa3a1413d393cb650");
+	TEST_SHA256_STR("abcdefghijklmnopqrstuvwxyz", "71c480df93d6ae2f1efad1447c66c9525e316218cf51fc8d9ed832f2daf18b73");
+	TEST_SHA256_STR(aaaaaaaaaa_100000.buf, "cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0");
+	TEST_SHA256_STR(alphabet_100000.buf, "e406ba321ca712ad35a698bf0af8d61fc4dc40eca6bdcea4697962724ccbde35");
+	TEST_SHA256_LITERAL("blob 0\0", "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813");
+	TEST_SHA256_LITERAL("blob 3\0abc", "c1cf6e465077930e88dc5136641d402f72a229ddd996f627d60e9639eaba35a6");
+	TEST_SHA256_LITERAL("tree 0\0", "6ef19b41225c5369f1c104d45d8d85efa9b057b53b14b4b9b939dd74decc5321");
+
 	strbuf_release(&aaaaaaaaaa_100000);
+	strbuf_release(&alphabet_100000);
 
 	return test_done();
 }