Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5626cd99-f44a-97db-334e-99f1d62112a1@gmail.com>
Date:   Sun, 26 Mar 2023 21:12:42 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [KTAP V2 PATCH] ktap_v2: allow prefix to KTAP lines
Content-Language: en-US
To:     Rae Moar <rmoar@google.com>, davidgow@google.com,
        skhan@linuxfoundation.org, keescook@chromium.org,
        Tim.Bird@sony.com, brendanhiggins@google.com
Cc:     corbet@lwn.net, guillaume.tucker@collabora.com,
        dlatypov@google.com, kernelci@lists.linux.dev,
        kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230316225926.494921-1-rmoar@google.com>
From:   Frank Rowand <frowand.list@gmail.com>
In-Reply-To: <20230316225926.494921-1-rmoar@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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