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 # 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! > Note that once we want to make it a real subtest, we'd need to run the > generator twice, once to get the number of params and then to run the > tests. If we require that param generators are deterministic in the > number of params generated, this is not a problem. > > Thanks, > -- Marco >
