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;
Subject: Re: [PATCH 1/1] Documentation: dev-tools: clarify KTAP specification
 wording
To:     David Gow <davidgow@google.com>
Cc:     Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Kees Cook <keescook@chromium.org>, Rae Moar <rmoar@google.com>,
        "Bird, Tim" <Tim.Bird@sony.com>,
        Brendan Higgins <brendanhiggins@google.com>,
        Rae Moar <rmr167@gmail.com>,
        Guillaume Tucker <guillaume.tucker@collabora.com>,
        Daniel Latypov <dlatypov@google.com>, kernelci@groups.io,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20220204203248.2981902-1-frowand.list@gmail.com>
 <CABVgOS=JUxV6PRUZvTQhisSP+p34+K9Z6yT7HkXu6qeqtak1tw@mail.gmail.com>
From:   Frank Rowand <frowand.list@gmail.com>
Message-ID: <f4317040-df10-02cb-90bb-59f993de1e41@gmail.com>
Date:   Fri, 4 Feb 2022 18:18:47 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <CABVgOS=JUxV6PRUZvTQhisSP+p34+K9Z6yT7HkXu6qeqtak1tw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/4/22 5:13 PM, David Gow wrote:
> On Sat, Feb 5, 2022 at 4:32 AM <frowand.list@gmail.com> wrote:
>>
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Clarify some confusing phrasing.
> 
> Thanks for this! A few comments below:
> 
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> One item that may result in bikeshedding is that I added the spec
>> version to the title line.
> 
> This is fine by me.
> 
>>
>>  Documentation/dev-tools/ktap.rst | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/dev-tools/ktap.rst b/Documentation/dev-tools/ktap.rst
>> index 878530cb9c27..3b7a26816930 100644
>> --- a/Documentation/dev-tools/ktap.rst
>> +++ b/Documentation/dev-tools/ktap.rst
>> @@ -1,8 +1,8 @@
>>  .. SPDX-License-Identifier: GPL-2.0
>>
>> -========================================
>> -The Kernel Test Anything Protocol (KTAP)
>> -========================================
>> +===================================================
>> +The Kernel Test Anything Protocol (KTAP), version 1
>> +===================================================
>>
>>  TAP, or the Test Anything Protocol is a format for specifying test results used
>>  by a number of projects. It's website and specification are found at this `link
>> @@ -186,7 +186,7 @@ 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 result lines in a subtest should be indented. One level of
>> +Additionally, all lines in a subtest should be indented. One level of
> 
> The original reason for this is to accommodate "unknown" lines which
> were not generated by the test itself (e.g, a KASAN report or BUG or
> something). These are awkward, as sometimes they're a useful thing to
> have as part of the test result, and sometimes they're unrelated spam.
> (Additionally, I think kselftest will indent these, as it indents the
> full results in a separate pass afterwards, but KUnit won't, as the
> level of nesting is done during printing.)
> 
> Personally, I'd rather leave this as is, or perhaps call out "unknown"
> lines explicitly, e.g:
> Additionally, all lines in a subtest (except for 'unknown' lines)
> should be indented...

Only listing result lines as being indented is not consistent with
the "Example KTAP output" section.  The example shows:

   Version line           - indented
   Plan line              - indented
   Test case result lines - indented
   Diagnostic lines       - indented
   Unknown lines          - not shown in the example

So there seem to be at least 4 types of lines that are indented for a
nested test.

The TAP standard (I'll use version 14 for my examples) does not allow
unknown lines (TAP 14 calls them "Anything else").  It says "is
incorrect", and "When the `pragma +strict` is enabled, incorrect test
lines SHOULD result in the test set being a failure, ...".  TAP 14
calls for the opposite behavior if `pragma -strict` is set.

TAP 14 goes on to say "`Test::Harness` silently ignores incorrect lines,
but will become more stringent in the futures.

It seems to me that KTAP "Unknown lines" are fundamentally different
than TAP 14 "Anything else" lines.  Tests that generate KTAP output
may print their results to the system console (or log), in which
case kernel messages (or for the system log the messages may even
come from non-kernel sources) either directly triggered by a test or
from a task that is totally unrelated to the test may exist in the KTAP
data stream.  So I would agree that "Unknown lines" are not indented.
Even if the "Unknown line" is directly triggered by the test.

But I think the KTAP specification should say that "Diagnostic lines"
are emitted by the test (or the test harness), and thus must be
indented when related to a nested test.

And as you suggest, "Unknown lines" should be explicitly called out
as not being part of "lines in a subtest", thus do not need to be
indented.

Does that sound good?

> 
> Thoughts?
> 
>>  indentation is two spaces: "  ". The indentation should begin at the version
>>  line and should end before the parent test's result line.
>>
>> @@ -225,8 +225,8 @@ Major differences between TAP and KTAP
>>  --------------------------------------
>>
>>  Note the major differences between the TAP and KTAP specification:
>> -- yaml and json are not recommended in diagnostic messages
>> -- TODO directive not recognized
>> +- yaml and json are not recommended in KTAP diagnostic messages
>> +- TODO directive not recognized in KTAP
>>  - KTAP allows for an arbitrary number of tests to be nested
>>
> 
> Looks good here, cheers.
> 
> 
>>  The TAP14 specification does permit nested tests, but instead of using another
>> --
>> Frank Rowand <frank.rowand@sony.com>
>>