On 10/11/20 10:11 pm, Marco Elver wrote: > On Tue, 10 Nov 2020 at 17:32, Arpitha Raghunandan <98.arpi@xxxxxxxxx> wrote: >> >> On 10/11/20 4:05 pm, Marco Elver wrote: >>> On Tue, 10 Nov 2020 at 08:21, David Gow <davidgow@xxxxxxxxxx> wrote: >>> [...] >>>>>> >>>>>> The previous attempt [1] at something similar failed because it seems >>>>>> we'd need to teach kunit-tool new tricks [2], too. >>>>>> [1] https://lkml.kernel.org/r/20201105195503.GA2399621@xxxxxxxxxxxxxxxx >>>>>> [2] https://lkml.kernel.org/r/20201106123433.GA3563235@xxxxxxxxxxxxxxxx >>>>>> >>>>>> So if we go with a different format, we might need a patch before this >>>>>> one to make kunit-tool compatible with that type of diagnostic. >>>>>> >>>>>> Currently I think we have the following proposals for a format: >>>>>> >>>>>> 1. The current "# [test_case->name]: param-[index] [ok|not ok]" -- >>>>>> this works well, because no changes to kunit-tool are required, and it >>>>>> also picks up the diagnostic context for the case and displays that on >>>>>> test failure. >>>>>> >>>>>> 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]". >>>>>> As-is, this needs a patch for kunit-tool as well. I just checked, and >>>>>> if we change it to "# [ok|not ok] - [test_case->name]: param-[index]" >>>>>> (note the space after ':') it works without changing kunit-tool. ;-) >>>>>> >>>>>> 3. Something like "# [ok|not ok] param-[index] - [test_case->name]", >>>>>> which I had played with earlier but kunit-tool is definitely not yet >>>>>> happy with. >>>>>> >>>>>> So my current preference is (2) with the extra space (no change to >>>>>> kunit-tool required). WDYT? >>>>>> >>>> >>>> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care >>>> what comes after the comment character except if it's an explicit >>>> subtest declaration or a crash. I'll try to put a patch together to >>>> fix it, but I'd rather not delay this just for that. >>>> >>>> In any thought about this a bit more, It turns out that the proposed >>>> KTAP spec[1] discourages the use of ':', except as part of a subtest >>>> declaration, or perhaps an as-yet-unspecified fully-qualified test >>>> name. The latter is what I was going for, but if it's actively >>>> breaking kunit_tool, we might want to hold off on it. >>>> >>>> If we were to try to treat these as subtests in accordance with that >>>> spec, the way we'd want to use one of these options: >>>> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the >>>> test case name, but otherwise treats things exactly the same way we >>>> treat existing subtests. >>>> >>>> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the >>>> "subtest", just gives repeated results with the same name. >>>> >>>> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]" >>>> -- This is equivalent to my suggestion with a separator of ":", or 2 >>>> above with a separator of ": ". The in-progress spec doesn't yet >>>> specify how these fully-qualified names would work, other than that >>>> they'd use a colon somewhere, and if we comment it out, ": " is >>>> required. >>>> >>>>> >>>>> Which format do we finally go with? >>>>> >>>> >>>> I'm actually going to make another wild suggestion for this, which is >>>> a combination of (1) and (A): >>>> "# [test_case->name]: [ok|not ok] [index] - param-[index]" >>>> >>>> This gives us a KTAP-compliant result line, except prepended with "# >>>> [test_case->name]: ", which makes it formally a diagnostic line, >>>> rather than an actual subtest. Putting the test name at the start >>>> matches what kunit_tool is expecting at the moment. If we then want to >>>> turn it into a proper subtest, we can just get rid of that prefix (and >>>> add the appropriate counts elsewhere). >>>> >>>> Does that sound good? >>> >>> Sounds reasonable to me! The repetition of [index] seems unnecessary >>> for now, but I think if we at some point have a way to get a string >>> representation of a param, we can substitute param-[index] with a >>> string that represents the param. >>> >> >> So, with this the inode-test.c will have the following output, right? >> >> TAP version 14 >> 1..7 >> # Subtest: ext4_inode_test >> 1..1 >> # inode_test_xtimestamp_decoding: ok 0 - param-0 > > So the params should certainly be 0-indexed, but I think TAP starts > counting at 1, so maybe this should be: > > # inode_test_xtimestamp_decoding: ok 1 - param-0 > > and so on... ? > > Right now it doesn't matter, but it will if we make these subsubtests > as David suggested. > Okay yes, I will make the first index (following [ok|not ok]) start with 1 and let the params remain 0-indexed. Thanks! >> # inode_test_xtimestamp_decoding: ok 1 - param-1 >> # inode_test_xtimestamp_decoding: ok 2 - param-2 >> # inode_test_xtimestamp_decoding: ok 3 - param-3 >> # inode_test_xtimestamp_decoding: ok 4 - param-4 >> # inode_test_xtimestamp_decoding: ok 5 - param-5 >> # inode_test_xtimestamp_decoding: ok 6 - param-6 >> # inode_test_xtimestamp_decoding: ok 7 - param-7 >> # inode_test_xtimestamp_decoding: ok 8 - param-8 >> # inode_test_xtimestamp_decoding: ok 9 - param-9 >> # inode_test_xtimestamp_decoding: ok 10 - param-10 >> # inode_test_xtimestamp_decoding: ok 11 - param-11 >> # inode_test_xtimestamp_decoding: ok 12 - param-12 >> # inode_test_xtimestamp_decoding: ok 13 - param-13 >> # inode_test_xtimestamp_decoding: ok 14 - param-14 >> # inode_test_xtimestamp_decoding: ok 15 - param-15 >> ok 1 - inode_test_xtimestamp_decoding >> ok 1 - ext4_inode_test >> >> I will send another patch with this change. >> Thanks! > > Yes that looks right, but see the comment above ^ > > Thanks, > -- Marco >
