On Tue, 10 Nov 2020 at 17:32, Arpitha Raghunandan 98.arpi@gmail.com wrote:
On 10/11/20 4:05 pm, Marco Elver wrote:
On Tue, 10 Nov 2020 at 08:21, David Gow davidgow@google.com 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@elver.google.com [2] https://lkml.kernel.org/r/20201106123433.GA3563235@elver.google.com
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:
- 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.
- 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. ;-)
- 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_decodingok 1 - ext4_inode_test
I will send another patch with this change. Thanks!
Yes that looks right, but see the comment above ^
Thanks, -- Marco