Message-ID: <597ab347-0ba4-7c5f-99a6-4ead0b956330@gmail.com>
Date:   Fri, 31 Mar 2023 15:50:37 -0500
From:   Frank Rowand <frowand.list@...il.com>
To:     Rae Moar <rmoar@...gle.com>
Cc:     davidgow@...gle.com, skhan@...uxfoundation.org,
        keescook@...omium.org, Tim.Bird@...y.com,
        brendanhiggins@...gle.com, corbet@....net,
        guillaume.tucker@...labora.com, dlatypov@...gle.com,
        kernelci@...ts.linux.dev, kunit-dev@...glegroups.com,
        linux-kselftest@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines

On 3/29/23 16:14, Rae Moar wrote:
> On Sun, Mar 26, 2023 at 10:12 PM Frank Rowand <frowand.list@...il.com> wrote:
>>
>> On 3/16/23 17:59, Rae Moar wrote:
>>> Change the KTAP v2 spec to allow variable prefixes to KTAP lines,
>>> instead of fixed indentation of two spaces. However, the prefix must be
>>> constant on the same level of testing (besides unknown lines).
>>>
>>> This was proposed by Tim Bird in 2021 and then supported by Frank Rowand
>>> in 2022 (see link below).
>>>
>>> Link: https://lore.kernel.org/all/bc6e9ed7-d98b-c4da-2a59-ee0915c18f10@gmail.com/
>>
>> Another link to the same thread, but expanded to show all replies in one page is:
>>
>>   https://lore.kernel.org/all/bc6e9ed7-d98b-c4da-2a59-ee0915c18f10@gmail.com/T/#u
>>
>> Near the top of that thread I proposed alternative 1 (essentially what Tim
>> originally suggested, and what Rae proposes here) and alternative 2 (with
>> slight variant 2b).  The overall preference seemed to be alternative 1, but
>> if we wanted to provide a method to provide test or system metadata then
>> alternative 2 might provide both a test prefix and metadata.
>>
>> Alternate 1 provides the vast majority of what I need the prefix for, but
>> I think there has been a recent comment that it would be useful to be able
>> to report system metadata (sorry, I haven't found a reference for that yet).
>> In my case, it would be informative to use metadata to report which config
>> options that impact the DT unittests are enabled.
>>
> 
> Hi Frank,
> 
> Thanks for all of your ideas!
> 
> Thinking more on this topic, I do think we will want a specified way
> to report test metadata in KTAP. This can be partly covered with this
> idea for a prefix. However, it might not provide the flexibility or
> comprehensiveness we need. For example, reporting the file for input
> or output might be too verbose for a prefix.
> 

> I thought your idea on config info lines was compelling. However, I am
> not sure using a result line to communicate the metadata is the best
> solution. This would alter the function of a result line. And for
> parsers that count "ok" and "not ok", this might create problems.

Good point.  I agree that using "ok 0 <something>" to define the prefix
string or metadata is a terrible hack.  (This was alternative 2 above.)

> 
> I have an idea that derives from my other KTAP proposal to declare a
> test name with "# Subtest:". The idea is to declare the metadata as
> diagnostic lines in between the version line and the test plan in
> order to separate this information from subtest diagnostic output. We
> could do something similar to below:
> 
> KTAP version 2
> 1..1
>   KTAP version 2
>   # Name: test_1          // Or as proposed: "# Subtest: test_1"
>   # File: /sys/kernel/...
>   # Config: CONFIG_1=y CONFIG_PARAM=2048
>   1..1
>   # subtest_1 passed
>   ok 1 subtest_1
> # test_1 passed
> ok 1 test_1
> 
> This is just an idea. I would love to hear other ideas on the best way
> to report metadata. Alternatively, we could create a new line format
> all together specific to report test metadata.

Let's tag that as "alternative 3".  So far, I like alternative 3 the most.

Alternative 3 has some impact on diagnostic lines.  KTAP v1 allows diagnostic
lines to occur anywhere.  I we leave that unchanged, then I think that any
metadata tag (such as "Name: ", "File: ", "Config: " in the above example)
should be made illegal in other diagnostic lines.  I don't like the idea
of restricting diagnostic line format in that matter, so I would instead
propose instead restricting non-metadata diagnostic lines to not be allowed
between the version line and the test plan line.  I don't think that
restriction would be problematic.

Alternative 3 also provides a clean way of implementing test name.  Also,
changing from subtest name to test name is a good cleanup.  Since the
name could be for the top level test, using "subtest" adds a conceptual
mismatch for the main test name.

> 
>>>
>>> As cited in the original proposal, it is useful in some Fuego tests to
>>> include an identifier in the prefix. This is an example:
>>>
>>>  KTAP version 1
>>>  1..2
>>>  [batch_id 4] KTAP version 1
>>>  [batch_id 4] 1..2
>>>  [batch_id 4] ok 1 cyclictest with 1000 cycles
>>>  [batch_id 4] # problem setting CLOCK_REALTIME
>>>  [batch_id 4] not ok 2 cyclictest with CLOCK_REALTIME
>>>  not ok 1 check realtime
>>>  [batch_id 4] KTAP version 1
>>>  [batch_id 4] 1..1
>>>  [batch_id 4] ok 1 IOZone read/write 4k blocks
>>>  ok 2 check I/O performance
>>>
>>> Here is a link to a version of the KUnit parser that is able to parse
>>> variable length prefixes for KTAP version 2. Note that the prefix must
>>> be constant at the same level of testing.
>>>
>>> Link: https://kunit-review.googlesource.com/c/linux/+/5710
>>>
>>> Signed-off-by: Rae Moar <rmoar@...gle.com>
>>> ---
>>>
>>> This idea has already been proposed but I wanted to potentially
>>> restart the discussion by demonstrating this change can by
>>> implemented in the KUnit parser. Let me know what you think.
>>>
>>> Note: this patch is based on Frank's ktap_spec_version_2 branch.
>>>
>>>  Documentation/dev-tools/ktap.rst | 21 ++++++++++++++++++---
>>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/dev-tools/ktap.rst b/Documentation/dev-tools/ktap.rst
>>> index ff77f4aaa6ef..ac61fdd97096 100644
>>> --- a/Documentation/dev-tools/ktap.rst
>>> +++ b/Documentation/dev-tools/ktap.rst
>>
>> Some additional lines of the Spec to be updated (from my alternate 1 email,
>> I haven't checked the current Spec to see if these are the exact changes
>> needed, but at least capture the intent:
>>
>> The "Version lines" format is changed from:
>>
>>    KTAP version 1
>>
>> to:
>>
>>    [<prefix string>] KTAP version 1
>>
>> The "Plan lines" format is changed from:
>>
>>    "1..N"
>>
>> to:
>>
>>    [<prefix string>] "1..N"
>>
>> The "Test case result lines" format is changed from:
>>
>>    <result> <number> [<description>][ # [<directive>] [<diagnostic data>]]
>>
>> to:
>>
>>    [<prefix string>] <result> <number> [<description>][ # [<directive>] [<diagnostic data>]]
>>
> 
> These are all great additions to the spec. Will add in version 2. I
> suppose we should add this detail to the diagnostic lines syntax as
> well.
> 
>>
>>    <prefix content is a constant string>
>>
>>
>> I wrote (with a bit of imprecision):
>>
>>   Indentation for "Nested tests" follows <prefix string>.  The indentation
>>   does NOT precede <prefix string>.
>>
>> which was meant to imply that the two space indentation would follow the
>> <prefix string>.
>>
>> The patch I am replying to instead replaces the two space indentation
>> entirely with the <prefix string>.  I think this patches' version of
>> indentation is superior to what I suggested.
>>
>>> @@ -192,9 +192,11 @@ starting with another KTAP version line and test plan, and end with the overall
>>>  result. If one of the subtests fail, for example, the parent test should also
>>>  fail.
>>>
>>> -Additionally, all lines in a subtest should be indented. One level of
>>> -indentation is two spaces: "  ". The indentation should begin at the version
>>> -line and should end before the parent test's result line.
>>> +Additionally, all lines in a subtest should be indented. The standard for one
>>> +level of indentation is two spaces: "  ". However, any prefix for indentation
>>> +is allowed as long as the prefix is consistent throughout that level of
>>> +testing. The indentation should begin at the version line and should end
>>> +before the parent test's result line.
>>>
>>>  "Unknown lines" are not considered to be lines in a subtest and thus are
>>>  allowed to be either indented or not indented.
>>
>> I was a little more verbose about "Unknown lines":
>>
>>    "Unknown lines" may optionally be prefixed with the <prefix string>, but
>>    are not required to be prefixed with the <prefix string>.  It is allowed
>>    for some "Unknown lines" to not be prefixed with the <prefix string>, even
>>    if one or more other "Unknown lines" are prefixed with the <prefix string>.
>>
>> I think combining the intent ("not considered to be lines in a subtest") with
>> the extra verbosity would be useful.
>>
> 
> I agree this seems like a useful addition. Will add for version 2.
> 
>>> @@ -229,6 +231,19 @@ An example format with multiple levels of nested testing:
>>>       not ok 1 example_test_1
>>>       ok 2 example_test_2
>>>
>>> +An example of a test with two nested subtests using prefixes:
>>> +
>>> +::
>>> +
>>> +     KTAP version 2
>>> +     1..1
>>> +     [prefix_1] KTAP version 2
>>> +     [prefix_1] 1..2
>>> +     [prefix_1] ok 1 test_1
>>> +     [prefix_1] ok 2 test_2
>>> +     # example passed
>>> +     ok 1 example
>>> +
>>
>> The "[" and "]" are meant to indicate an optional field, so the
>> example would be:
>>
>> +       KTAP version 2
>> +       1..1
>> +       prefix_1 KTAP version 2
>> +       prefix_1 1..2
>> +       prefix_1 ok 1 test_1
>> +       prefix_1 ok 2 test_2
>> +       # example passed
>> +       ok 1 example
>> +
>>
> 
> Thanks, this is better to exclude the square brackets. Will change
> this for version 2.
> 
>> Of course, "[" and "]" are valid characters within the prefix string, so
>> that an example of "[prefix_1]" could be mentioned as a valid example.
>>
>> I would suggest some additional more complex examples:
>>
>> +       prefix_0 KTAP version 2
>> +       prefix_0 1..1
>> +       prefix_0 prefix_1 KTAP version 2
>> +       prefix_0 prefix_1 1..2
>> +       prefix_0 prefix_1 ok 1 test_1
>> +       prefix_0 prefix_1 ok 2 test_2
>> +       # example passed
>> +       prefix_0 ok 1 example
>> +
> 
> Shouldn't the "# example passed" line include the prefix_0?

Yes, I goofed up on that.  The same applies to the following
examples.

> 
>>
>> +       KTAP version 2
>> +       1..2
>> +       prefix_1 KTAP version 2
>> +       prefix_1 1..2
>> +       prefix_1 ok 1 test_a_1
>> +       prefix_1 ok 2 test_a_2
>> +       # example passed
>> +       ok 1 example
>> +       prefix_2 KTAP version 2
>> +       prefix_2 1..2
>> +       prefix_2 ok 1 test_b_1
>> +       prefix_2 ok 2 test_b_2
>> +       # example passed
>> +       ok 2 example
>> +
>>
>> +       KTAP version 2
>> +       1..3
>> +       prefix_1 KTAP version 2
>> +       prefix_1 1..2
>> +       prefix_1 ok 1 test_a_1
>> +       prefix_1 ok 2 test_a_2
>> +       # example passed
>> +       ok 1 example
>> +         KTAP version 2
>> +         1..2
>> +         ok 1 test_b_1
>> +         ok 2 test_b_2
>> +       # example passed
>> +       ok 2 example
>> +       prefix_2 KTAP version 2
>> +       prefix_2 1..2
>> +       prefix_2 ok 1 test_c_1
>> +       prefix_2 ok 2 test_c_2
>> +       # example passed
>> +       ok 3 example
>> +
>>
>>
> 
> Otherwise, these all look very helpful. I will definitely be adding
> these more complex examples in version 2.
> 
> Thanks!
> 
> Rae
> 
>>
>>>
>>>  Major differences between TAP and KTAP
>>>  --------------------------------------
>>>
>>> base-commit: 906f02e42adfbd5ae70d328ee71656ecb602aaf5
>>

Powered by blists - more mailing lists