| Message ID | 20210926223322.848641-4-isabellabdoamaral@usp.br (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | test_hash.c: refactor into KUnit | expand |
On Mon, 27 Sept 2021 at 00:33, 'Isabella Basso' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Split up test_hash_init so that it calls each test more explicitly > insofar it is possible without rewriting the entire file. This aims at > improving readability. > > Split tests performed on string_or as they don't interfere with those > performed in hash_or. Also separate pr_info calls about skipped tests as > they're not part of the tests themselves, but only warn about > (un)defined arch-specific hash functions. > > Changes since v1: > - As suggested by David Gow: > 1. Rename arch-specific test functions. > 2. Remove spare whitespace changes. > - As suggested by Marco Elver: > 1. Add struct for carrying test variables. Did the patches get mixed up? The struct doesn't appear to be introduced here. > Tested-by: David Gow <davidgow@google.com> > Signed-off-by: Isabella Basso <isabellabdoamaral@usp.br> > --- > lib/test_hash.c | 66 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 54 insertions(+), 12 deletions(-) > > diff --git a/lib/test_hash.c b/lib/test_hash.c > index 08fe63776c4f..db9dd18b4e8b 100644 > --- a/lib/test_hash.c > +++ b/lib/test_hash.c > @@ -153,11 +153,39 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33]) > > #define SIZE 256 /* Run time is cubic in SIZE */ > > -static int __init > -test_hash_init(void) > +static int __init test_string_or(void) > { > char buf[SIZE+1]; > - u32 string_or = 0, hash_or[2][33] = { { 0, } }; > + u32 string_or = 0; > + int i, j; > + > + fill_buf(buf, SIZE, 1); > + > + /* Test every possible non-empty substring in the buffer. */ > + for (j = SIZE; j > 0; --j) { > + buf[j] = '\0'; > + > + for (i = 0; i <= j; i++) { > + u32 h0 = full_name_hash(buf+i, buf+i, j-i); > + > + string_or |= h0; > + } /* i */ > + } /* j */ > + > + /* The OR of all the hash values should cover all the bits */ > + if (~string_or) { > + pr_err("OR of all string hash results = %#x != %#x", > + string_or, -1u); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int __init test_hash_or(void) > +{ > + char buf[SIZE+1]; > + u32 hash_or[2][33] = { { 0, } }; > unsigned tests = 0; > unsigned long long h64 = 0; > int i, j; > @@ -187,7 +215,6 @@ test_hash_init(void) > return -EINVAL; > } > > - string_or |= h0; > h64 = h64 << 32 | h0; /* For use with hash_64 */ > if (!test_int_hash(h64, hash_or)) > return -EINVAL; > @@ -195,12 +222,6 @@ test_hash_init(void) > } /* i */ > } /* j */ > > - /* The OR of all the hash values should cover all the bits */ > - if (~string_or) { > - pr_err("OR of all string hash results = %#x != %#x", > - string_or, -1u); > - return -EINVAL; > - } > if (~hash_or[0][0]) { > pr_err("OR of all __hash_32 results = %#x != %#x", > hash_or[0][0], -1u); > @@ -232,6 +253,13 @@ test_hash_init(void) > } > } > > + pr_notice("%u tests passed.", tests); > + > + return 0; > +} > + > +static void __init notice_skipped_tests(void) > +{ > /* Issue notices about skipped tests. */ > #ifdef HAVE_ARCH__HASH_32 > #if HAVE_ARCH__HASH_32 != 1 > @@ -247,10 +275,24 @@ test_hash_init(void) > #else > pr_info("hash_64() has no arch implementation to test."); > #endif > +} > > - pr_notice("%u tests passed.", tests); > +static int __init > +test_hash_init(void) > +{ > + int ret; > > - return 0; > + ret = test_string_or(); > + if (ret < 0) > + return ret; > + > + ret = test_hash_or(); > + if (ret < 0) > + return ret; > + > + notice_skipped_tests(); > + > + return ret; > } > > static void __exit test_hash_exit(void) > -- > 2.33.0 > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210926223322.848641-4-isabellabdoamaral%40usp.br.
Hi, Marco, On Mon, Sep 27, 2021 at 5:17 AM Marco Elver <elver@google.com> wrote: > > On Mon, 27 Sept 2021 at 00:33, 'Isabella Basso' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > Split up test_hash_init so that it calls each test more explicitly > > insofar it is possible without rewriting the entire file. This aims at > > improving readability. > > > > Split tests performed on string_or as they don't interfere with those > > performed in hash_or. Also separate pr_info calls about skipped tests as > > they're not part of the tests themselves, but only warn about > > (un)defined arch-specific hash functions. > > > > Changes since v1: > > - As suggested by David Gow: > > 1. Rename arch-specific test functions. > > 2. Remove spare whitespace changes. > > - As suggested by Marco Elver: > > 1. Add struct for carrying test variables. > > Did the patches get mixed up? The struct doesn't appear to be introduced here. Yeah, thanks for the heads up! I must have mixed the messages when rebasing. Sorry about that. The struct was actually introduced in patch 2/5. Do you want to have a look at it or should I send the v3 with the correct message before that? Cheers, Isabella Basso
On Mon, 27 Sept 2021 at 14:03, Isabella B do Amaral <isabellabdoamaral@usp.br> wrote: > > Hi, Marco, > > On Mon, Sep 27, 2021 at 5:17 AM Marco Elver <elver@google.com> wrote: > > > > On Mon, 27 Sept 2021 at 00:33, 'Isabella Basso' via KUnit Development > > <kunit-dev@googlegroups.com> wrote: > > > > > > Split up test_hash_init so that it calls each test more explicitly > > > insofar it is possible without rewriting the entire file. This aims at > > > improving readability. > > > > > > Split tests performed on string_or as they don't interfere with those > > > performed in hash_or. Also separate pr_info calls about skipped tests as > > > they're not part of the tests themselves, but only warn about > > > (un)defined arch-specific hash functions. > > > > > > Changes since v1: > > > - As suggested by David Gow: > > > 1. Rename arch-specific test functions. > > > 2. Remove spare whitespace changes. > > > - As suggested by Marco Elver: > > > 1. Add struct for carrying test variables. > > > > Did the patches get mixed up? The struct doesn't appear to be introduced here. > > Yeah, thanks for the heads up! I must have mixed the messages when rebasing. > Sorry about that. The struct was actually introduced in patch 2/5. Do > you want to > have a look at it or should I send the v3 with the correct message before that? For review it's fine as-is, given it's a trivial change, but the final series should have it in the right place.
On Mon, Sep 27, 2021 at 6:33 AM Isabella Basso <isabellabdoamaral@usp.br> wrote: > > Split up test_hash_init so that it calls each test more explicitly > insofar it is possible without rewriting the entire file. This aims at > improving readability. > > Split tests performed on string_or as they don't interfere with those > performed in hash_or. Also separate pr_info calls about skipped tests as > they're not part of the tests themselves, but only warn about > (un)defined arch-specific hash functions. > > Changes since v1: > - As suggested by David Gow: > 1. Rename arch-specific test functions. > 2. Remove spare whitespace changes. > - As suggested by Marco Elver: > 1. Add struct for carrying test variables. Nit: Move the changelog to after the "---" (and the correct patch). > > Tested-by: David Gow <davidgow@google.com> > Signed-off-by: Isabella Basso <isabellabdoamaral@usp.br> > --- This seems good to me, though I admit this is the part I'm probably least knowledgeable about. I'm pretty sure there has to be a more straightforward way to test some of these hash functions, but it's probably better to keep this as-is rather than doing anything too drastic in the middle of the port to KUnit. The biggest downside here is that we now double the number of calls to fill_buffer() and full_name_hash(), so the test is likely to be a bit slower. It still runs fast enough (at least with the default SIZE of 256) that it's not noticeable to me, though, so I don't think it's a problem. Apart from Marco's comment about the changelog in the commit message is fixed, this is: Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > lib/test_hash.c | 66 ++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 54 insertions(+), 12 deletions(-) > > diff --git a/lib/test_hash.c b/lib/test_hash.c > index 08fe63776c4f..db9dd18b4e8b 100644 > --- a/lib/test_hash.c > +++ b/lib/test_hash.c > @@ -153,11 +153,39 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33]) > > #define SIZE 256 /* Run time is cubic in SIZE */ > > -static int __init > -test_hash_init(void) > +static int __init test_string_or(void) > { > char buf[SIZE+1]; > - u32 string_or = 0, hash_or[2][33] = { { 0, } }; > + u32 string_or = 0; > + int i, j; > + > + fill_buf(buf, SIZE, 1); > + > + /* Test every possible non-empty substring in the buffer. */ > + for (j = SIZE; j > 0; --j) { > + buf[j] = '\0'; > + > + for (i = 0; i <= j; i++) { > + u32 h0 = full_name_hash(buf+i, buf+i, j-i); > + > + string_or |= h0; > + } /* i */ > + } /* j */ > + > + /* The OR of all the hash values should cover all the bits */ > + if (~string_or) { > + pr_err("OR of all string hash results = %#x != %#x", > + string_or, -1u); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int __init test_hash_or(void) > +{ > + char buf[SIZE+1]; > + u32 hash_or[2][33] = { { 0, } }; > unsigned tests = 0; > unsigned long long h64 = 0; > int i, j; > @@ -187,7 +215,6 @@ test_hash_init(void) > return -EINVAL; > } > > - string_or |= h0; > h64 = h64 << 32 | h0; /* For use with hash_64 */ > if (!test_int_hash(h64, hash_or)) > return -EINVAL; > @@ -195,12 +222,6 @@ test_hash_init(void) > } /* i */ > } /* j */ > > - /* The OR of all the hash values should cover all the bits */ > - if (~string_or) { > - pr_err("OR of all string hash results = %#x != %#x", > - string_or, -1u); > - return -EINVAL; > - } > if (~hash_or[0][0]) { > pr_err("OR of all __hash_32 results = %#x != %#x", > hash_or[0][0], -1u); > @@ -232,6 +253,13 @@ test_hash_init(void) > } > } > > + pr_notice("%u tests passed.", tests); > + > + return 0; > +} > + > +static void __init notice_skipped_tests(void) > +{ > /* Issue notices about skipped tests. */ > #ifdef HAVE_ARCH__HASH_32 > #if HAVE_ARCH__HASH_32 != 1 > @@ -247,10 +275,24 @@ test_hash_init(void) > #else > pr_info("hash_64() has no arch implementation to test."); > #endif > +} > > - pr_notice("%u tests passed.", tests); > +static int __init > +test_hash_init(void) > +{ > + int ret; > > - return 0; > + ret = test_string_or(); > + if (ret < 0) > + return ret; > + > + ret = test_hash_or(); > + if (ret < 0) > + return ret; > + > + notice_skipped_tests(); > + > + return ret; > } > > static void __exit test_hash_exit(void) > -- > 2.33.0 >
diff --git a/lib/test_hash.c b/lib/test_hash.c index 08fe63776c4f..db9dd18b4e8b 100644 --- a/lib/test_hash.c +++ b/lib/test_hash.c @@ -153,11 +153,39 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33]) #define SIZE 256 /* Run time is cubic in SIZE */ -static int __init -test_hash_init(void) +static int __init test_string_or(void) { char buf[SIZE+1]; - u32 string_or = 0, hash_or[2][33] = { { 0, } }; + u32 string_or = 0; + int i, j; + + fill_buf(buf, SIZE, 1); + + /* Test every possible non-empty substring in the buffer. */ + for (j = SIZE; j > 0; --j) { + buf[j] = '\0'; + + for (i = 0; i <= j; i++) { + u32 h0 = full_name_hash(buf+i, buf+i, j-i); + + string_or |= h0; + } /* i */ + } /* j */ + + /* The OR of all the hash values should cover all the bits */ + if (~string_or) { + pr_err("OR of all string hash results = %#x != %#x", + string_or, -1u); + return -EINVAL; + } + + return 0; +} + +static int __init test_hash_or(void) +{ + char buf[SIZE+1]; + u32 hash_or[2][33] = { { 0, } }; unsigned tests = 0; unsigned long long h64 = 0; int i, j; @@ -187,7 +215,6 @@ test_hash_init(void) return -EINVAL; } - string_or |= h0; h64 = h64 << 32 | h0; /* For use with hash_64 */ if (!test_int_hash(h64, hash_or)) return -EINVAL; @@ -195,12 +222,6 @@ test_hash_init(void) } /* i */ } /* j */ - /* The OR of all the hash values should cover all the bits */ - if (~string_or) { - pr_err("OR of all string hash results = %#x != %#x", - string_or, -1u); - return -EINVAL; - } if (~hash_or[0][0]) { pr_err("OR of all __hash_32 results = %#x != %#x", hash_or[0][0], -1u); @@ -232,6 +253,13 @@ test_hash_init(void) } } + pr_notice("%u tests passed.", tests); + + return 0; +} + +static void __init notice_skipped_tests(void) +{ /* Issue notices about skipped tests. */ #ifdef HAVE_ARCH__HASH_32 #if HAVE_ARCH__HASH_32 != 1 @@ -247,10 +275,24 @@ test_hash_init(void) #else pr_info("hash_64() has no arch implementation to test."); #endif +} - pr_notice("%u tests passed.", tests); +static int __init +test_hash_init(void) +{ + int ret; - return 0; + ret = test_string_or(); + if (ret < 0) + return ret; + + ret = test_hash_or(); + if (ret < 0) + return ret; + + notice_skipped_tests(); + + return ret; } static void __exit test_hash_exit(void)