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. > # 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
