On 12/10/2020 22.46, Brendan Higgins wrote: > On Fri, Aug 21, 2020 at 03:28:49PM +0300, Andy Shevchenko wrote: >> On Fri, Aug 21, 2020 at 01:37:10PM +0200, Petr Mladek wrote: >>> On Mon 2020-08-17 09:06:32, Rasmus Villemoes wrote: >>>> On 17/08/2020 06.30, Arpitha Raghunandan wrote: >>>>> Converts test lib/test_printf.c to KUnit. >>>>> More information about KUnit can be found at >>>>> https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html. >>>>> KUnit provides a common framework for unit tests in the kernel. >>>> >>>> So I can continue to build a kernel with some appropriate CONFIG set to >>>> y, boot it under virt-me, run dmesg and see if I broke printf? That's >>>> what I do now, and I don't want to have to start using some enterprisy >>>> framework. >>> >>> I had the same concern. I have tried it. > > Sorry you feel that way. Do you have any suggestions on how we can make > it seem less enterprisy? Seems like there are people here who are not a > fan of the output format, so of which we can fix here, some of which is > part of KTAP[1]. I'm fine with machine-readable TAP, but I most defintely also want human-readable, which means all the excessive and pointless lines need to go away. >> Which raises an obvious question: did the people who convert this test this >> themselves? Looks like a janitor work in the area without understanding the >> area good enough. > > Looks to me like Arpitha ran it, but you are right, we don't have a lot > of familiarity with this area; we were treating it as "janitor work" as > you say. > > Our intention was just to take some existing tests and as non-invasively > as possible, get them to report using a common format, and maybe even > get some of the tests to follow a common pattern. > >> Probably I will NAK all those patches from now on, until it will be good commit >> messages and cover of risen aspects, including reference to before and after >> outcome for passed and failed test cases. > > Fair enough, hopefully we can address these issues in the next revision. > > One issue though, with the "before and after outcome" you are > referencing; are you referring to the issue that Petr pointed out in how > they are inconsistent: > > + original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0' > + kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127' > > (I think Rasmus addressed this.) Or are your referring to something > else? Yeah, that change is fine and expected, can we stop bringing that up. It's all the explicit "memcmp() == 0 failed" gunk at least I am concerned with. If you can get rid of that (basically, stop stringifying the code, that's completely irrelevant) and just get the messages from the test itself that explains what went wrong. I'm fine with interspersing that with a few TAP-readable lines. But things like Expected memcmp(test_buffer, expect, written) == 0, but memcmp(test_buffer, expect, written) == 1 0 == 0 are utterly useless. We're not _testing_ memcmp, we're _using_ it to know if vsprintf() did as we expected. So just mechanically changing "memcmp() == 0" into "SOME_MACRO(memcmp(), 0)" is never going to work, at least when SOME_MACRO does the stringify and ends up producing the above. But if you can end up producing [ 56.795433] # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76 vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127' that's fine; that's basically just prepending an EXPECTATION FAILED line to the existing output. So doing it properly would probably be either - change the existing pr_warn()s to use some KUNIT macro that generates whatever extra info is needed by TAP, in addition to the current human-readable message, and/or - just add a few lines of TAP-suitable FAIL/PASS lines here and there but let me repeat that the control flow (early returns) in do_test() cannot be modified. Rasmus
