On Fri, 13 Nov 2020 at 23:37, David Gow <davidgow@xxxxxxxxxx> wrote: > > On Fri, Nov 13, 2020 at 6:31 PM Marco Elver <elver@xxxxxxxxxx> wrote: > > > > On Fri, Nov 13, 2020 at 01:17PM +0800, David Gow wrote: > > > On Thu, Nov 12, 2020 at 8:37 PM Marco Elver <elver@xxxxxxxxxx> wrote: > > [...] > > > > > (It also might be a little tricky with the current implementation to > > > > > produce the test plan, as the parameters come from a generator, and I > > > > > don't think there's a way of getting the number of parameters ahead of > > > > > time. That's a problem with the sub-subtest model, too, though at > > > > > least there it's a little more isolated from other tests.) > > > > > > > > The whole point of generators, as I envisage it, is to also provide the > > > > ability for varying parameters dependent on e.g. environment, > > > > configuration, number of CPUs, etc. The current array-based generator is > > > > the simplest possible use-case. > > > > > > > > However, we *can* require generators generate a deterministic number of > > > > parameters when called multiple times on the same system. > > > > > > I think this is a reasonable compromise, though it's not actually > > > essential. As I understand the TAP spec, the test plan is actually > > > optional (and/or can be at the end of the sequence of tests), though > > > kunit_tool currently only supports having it at the beginning (which > > > is strongly preferred by the spec anyway). I think we could get away > > > with having it at the bottom of the subtest results though, which > > > would save having to run the generator twice, when subtest support is > > > added to kunit_tool. > > > > I can't find this in the TAP spec, where should I look? Perhaps we > > shouldn't venture too far off the beaten path, given we might not be the > > only ones that want to parse this output. > > > > It's in the "Test Lines and the Plan" section: > "The plan is optional but if there is a plan before the test points it > must be the first non-diagnostic line output by the test file. In > certain instances a test file may not know how many test points it > will ultimately be running. In this case the plan can be the last > non-diagnostic line in the output. The plan cannot appear in the > middle of the output, nor can it appear more than once." Ah, that's fine then. > My only concern with running through the generator multiple times to > get the count is that it might be slow and/or more difficult if > someone uses a more complicated generator. I can't think of anything > specific yet, though, so we can always do it for now and change it > later if a problematic case occurs. I'm all for simplicity, so if nobody objects, let's just get rid of the number of parameters and avoid running it twice. > > > > To that end, I propose a v7 (below) that takes care of getting number of > > > > parameters (and also displays descriptions for each parameter where > > > > available). > > > > > > > > Now it is up to you how you want to turn the output from diagnostic > > > > lines into something TAP compliant, because now we have the number of > > > > parameters and can turn it into a subsubtest. But I think kunit-tool > > > > doesn't understand subsubtests yet, so I suggest we take these patches, > > > > and then somebody can prepare kunit-tool. > > > > > > > > > > This sounds good to me. The only thing I'm not sure about is the > > > format of the parameter description: thus far test names be valid C > > > identifier names, due to the fact they're named after the test > > > function. I don't think there's a fundamental reason parameters (and > > > hence, potentially, subsubtests) need to follow that convention as > > > well, but it does look a bit odd. Equally, the square brackets around > > > the description shouldn't be necessary according to the TAP spec, but > > > do seem to make things a little more readable, particuarly with the > > > names in the ext4 inode test. I'm not too worried about either of > > > those, though: I'm sure it'll look fine once I've got used to it. > > > > The parameter description doesn't need to be a C identifier. At least > > that's what I could immediately glean from TAP v13 spec (I'm looking > > here: https://testanything.org/tap-version-13-specification.html and see > > e.g. "ok 1 - Input file opened" ...). > > > > Yeah: it looked a bit weird for everything else to be an identifier > (given that KUnit does require it for tests), but these parameter > descriptions not to be. It's not a problem, though, so let's go ahead > with it. > > > [...] > > > > > In any case, I'm happy to leave the final decision here to Arpitha and > > > > > Marco, so long as we don't actually violate the TAP/KTAP spec and > > > > > kunit_tool is able to read at least the top-level result. My > > > > > preference is still to go either with the "# [test_case->name]: > > > > > [ok|not ok] [index] - param-[index]", or to get rid of the > > > > > per-parameter results entirely for now (or just print out a diagnostic > > > > > message on failure). In any case, it's a decision we can revisit once > > > > > we have support for named parameters, better tooling, or a better idea > > > > > of how people are actually using this. > > > > > > > > Right, so I think we'll be in a better place if we implement: 1) > > > > parameter to description conversion support, 2) counting parameters. So > > > > I decided to see what it looks like, and it wasn't too bad. I just don't > > > > know how you want to fix kunit-tool to make these non-diagnostic lines > > > > and not complain, but as I said, it'd be good to not block these > > > > patches. > > > > > > Yup, I tried this v7, and it looks good to me. The kunit_tool work > > > will probably be a touch more involved, so I definitely don't want to > > > hold up supporting this on that. > > > > > > My only thoughts on the v7 patch are: > > > - I don't think we actually need the parameter count yet (or perhaps > > > ever if we go with subtests as planned), so I be okay with getting rid > > > of that. > > > > As noted above, perhaps we should keep it for compatibility with other > > parsers and CI systems we don't have much control over. It'd be a shame > > if 99% of KUnit output can be parsed by some partially compliant parser, > > yet this would break it. > > KUnit has only started providing the test plans in some cases pretty > recently, and the spec does make it optional, so I'm not particularly > worried about this breaking parsers. I'm not too worried about it > causing problems to have it either, though, so if you'd rather keep > it, that's fine by me as well. > > > > - It'd be a possibility to get rid of the square brackets from the > > > output, and if we still want them, make them part of the test itself: > > > if this were TAP formatted, those brackets would be part of the > > > subsubtest name. > > > > I don't mind. It's just that we can't prescribe a format, and as > > seen below the descriptions include characters -<>=,. which can be > > confusing. But perhaps you're right, so let's remove them. > > > > But as noted, TAP doesn't seem to care. So let's remove them. > > > > Yeah: I have a slight preference for removing them, as TAP parsers > would otherwise include them in the parameter name, which looks a > little weird. > Of course, the point is moot until we actually fix kunit_tool and make > these subtests, so there's no fundamental reason we couldn't leave > them in for now, and remove them then if you thought it was > significantly more readable. (Personally, I'd still err on the side of > removing them to avoid any unnecessary churn.) Sounds good. Arpitha: Do you want to send v7, but with the following modifications from what I proposed? Assuming nobody objects. 1. Remove the num_params counter and don't print the number of params anymore, nor do validation that generators are deterministic. 2. Remove the []. [ I'm happy to send as well, just let me know what you prefer. ] Thanks, -- Marco
