linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines
@ 2023-03-16 22:59 Rae Moar
  2023-03-27  2:12 ` Frank Rowand
  0 siblings, 1 reply; 4+ messages in thread
From: Rae Moar @ 2023-03-16 22:59 UTC (permalink / raw)
  To: frowand.list, davidgow, skhan, keescook, Tim.Bird, brendanhiggins
  Cc: corbet, guillaume.tucker, dlatypov, kernelci, kunit-dev,
	linux-kselftest, linux-doc, linux-kernel, Rae Moar

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/

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@google.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
@@ -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.
@@ -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
+
 
 Major differences between TAP and KTAP
 --------------------------------------

base-commit: 906f02e42adfbd5ae70d328ee71656ecb602aaf5
-- 
2.40.0.rc1.284.g88254d51c5-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines
  2023-03-16 22:59 [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines Rae Moar
@ 2023-03-27  2:12 ` Frank Rowand
  2023-03-29 21:14   ` Rae Moar
  0 siblings, 1 reply; 4+ messages in thread
From: Frank Rowand @ 2023-03-27  2:12 UTC (permalink / raw)
  To: Rae Moar, davidgow, skhan, keescook, Tim.Bird, brendanhiggins
  Cc: corbet, guillaume.tucker, dlatypov, kernelci, kunit-dev,
	linux-kselftest, linux-doc, linux-kernel

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.

> 
> 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@google.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>]]


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

> @@ -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
+

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
+

+	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
+



>  
>  Major differences between TAP and KTAP
>  --------------------------------------
> 
> base-commit: 906f02e42adfbd5ae70d328ee71656ecb602aaf5


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines
  2023-03-27  2:12 ` Frank Rowand
@ 2023-03-29 21:14   ` Rae Moar
  2023-03-31 20:50     ` Frank Rowand
  0 siblings, 1 reply; 4+ messages in thread
From: Rae Moar @ 2023-03-29 21:14 UTC (permalink / raw)
  To: Frank Rowand
  Cc: davidgow, skhan, keescook, Tim.Bird, brendanhiggins, corbet,
	guillaume.tucker, dlatypov, kernelci, kunit-dev, linux-kselftest,
	linux-doc, linux-kernel

On Sun, Mar 26, 2023 at 10:12 PM Frank Rowand <frowand.list@gmail.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.

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.

> >
> > 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@google.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?

>
> +       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
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines
  2023-03-29 21:14   ` Rae Moar
@ 2023-03-31 20:50     ` Frank Rowand
  0 siblings, 0 replies; 4+ messages in thread
From: Frank Rowand @ 2023-03-31 20:50 UTC (permalink / raw)
  To: Rae Moar
  Cc: davidgow, skhan, keescook, Tim.Bird, brendanhiggins, corbet,
	guillaume.tucker, dlatypov, kernelci, kunit-dev, linux-kselftest,
	linux-doc, linux-kernel

On 3/29/23 16:14, Rae Moar wrote:
> On Sun, Mar 26, 2023 at 10:12 PM Frank Rowand <frowand.list@gmail.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@google.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
>>


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-03-31 20:50 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 22:59 [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines Rae Moar
2023-03-27  2:12 ` Frank Rowand
2023-03-29 21:14   ` Rae Moar
2023-03-31 20:50     ` Frank Rowand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).