+Shuah Khan On Sat, Oct 2, 2021 at 12:30 AM David Gow <davidgow@xxxxxxxxxx> wrote: > > On Mon, Sep 27, 2021 at 6:33 AM Isabella Basso <isabellabdoamaral@xxxxxx> wrote: > > > > We refactored the lib/test_hash.c file into KUnit as part of the student > > group LKCAMP [1] introductory hackathon for kernel development. > > > > This test was pointed to our group by Daniel Latypov [2], so its full > > conversion into a pure KUnit test was our goal in this patch series, but > > we ran into many problems relating to it not being split as unit tests, > > which complicated matters a bit, as the reasoning behind the original > > tests is quite cryptic for those unfamiliar with hash implementations. > > > > Some interesting developments we'd like to highlight are: > > > > - In patch 1/5 we noticed that there was an unused define directive that > > could be removed. > > - In patch 4/5 we noticed how stringhash and hash tests are all under > > the lib/test_hash.c file, which might cause some confusion, and we > > also broke those kernel config entries up. > > > > Overall KUnit developments have been made in the other patches in this > > series: > > > > In patches 2/5, 3/5 and 5/5 we refactored the lib/test_hash.c > > file so as to make it more compatible with the KUnit style, whilst > > preserving the original idea of the maintainer who designed it (i.e. > > George Spelvin), which might be undesirable for unit tests, but we > > assume it is enough for a first patch. > > > > This is our first patch series so we hope our contributions are > > interesting and also hope to get some useful criticism from the > > community. :) > > > > Changes since V1: > > - Fixed compilation on parisc and m68k. > > - Fixed whitespace mistakes. > > - Renamed a few functions. > > - Refactored globals into struct for test function params, thus removing > > a patch. > > - Reworded some commit messages. > > > > [1] - https://lkcamp.dev/ > > [2] - https://lore.kernel.org/linux-kselftest/CAGS_qxojszgM19u=3HLwFgKX5bm5KhywvsSunuBAt5RtR+GyxQ@xxxxxxxxxxxxxx/ > > > > Thanks: I've gone through this new revision, and it still works fine, > and my prior comments have been addressed. The commit messages in > particular are much clearer, thank you! I've reviewed the various > patches and left a few comments here and there, but there's nothing > too drastic. > > I'm pretty happy with this from the KUnit side, but it would be ideal > if someone with more knowledge of the hash functions looked over it. > Unfortunately, George's email is bouncing, and no-one else has made > any particularly major changes to this. > > My only remaining comment on the tests themselves is that it'd be nice > to have them split up further into more, smaller tests. Given that > it's a port of an existing test, though, I understand the desire not > to change things too drastically. > > We also need to work out how this is going to go upstream: does it go > through the kunit branch (via Shuah's kselftest repo), or directly to > Linus (who's handled most of the other recent-ish changes here. > Brendan, any thoughts? I think Shuah should take them in 5.16. Shuah, let me know if you are OK taking these in 5.16 and I will update the patch tracker. > Cheers, > -- David > > > > > Isabella Basso (5): > > hash.h: remove unused define directive > > test_hash.c: split test_int_hash into arch-specific functions > > test_hash.c: split test_hash_init > > lib/Kconfig.debug: properly split hash test kernel entries > > test_hash.c: refactor into kunit > > > > include/linux/hash.h | 5 +- > > lib/Kconfig.debug | 28 ++++- > > lib/Makefile | 3 +- > > lib/test_hash.c | 247 +++++++++++++++++-------------------- > > tools/include/linux/hash.h | 5 +- > > 5 files changed, 139 insertions(+), 149 deletions(-) > > > > -- > > 2.33.0 > >
