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: <dd3372a2-98f0-dcbd-1e04-7d1e19bdd231@gmail.com>
Date:   Sun, 26 Mar 2023 18:36:02 -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: add skip test result
Content-Language: en-US
From:   Frank Rowand <frowand.list@gmail.com>
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@groups.io,
        kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230310222002.3633162-1-rmoar@google.com>
 <490271eb-1429-2217-6e38-837c6e5e328b@gmail.com>
 <93e35a77-c00b-5c66-a460-6018dab98175@gmail.com>
In-Reply-To: <93e35a77-c00b-5c66-a460-6018dab98175@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/13/23 09:41, Frank Rowand wrote:
> On 3/11/23 21:52, Frank Rowand wrote:
>> On 3/10/23 16:20, Rae Moar wrote:
>>> Add the test result "skip" to KTAP version 2 as an alternative way to
>>> indicate a test was skipped.
>>>
>>> The current spec uses the "#SKIP" directive to indicate that a test was
>>> skipped. However, the "#SKIP" directive is not always evident when quickly
>>> skimming through KTAP results.
>>>
>>> The "skip" result would provide an alternative that could make it clearer
>>> that a test has not successfully passed because it was skipped.
>>>
>>> Before:
>>>
>>>  KTAP version 1
>>>  1..1
>>>    KTAP version 1
>>>    1..2
>>>    ok 1 case_1
>>>    ok 2 case_2 #SKIP
>>>  ok 1 suite
>>>
>>> After:
>>>
>>>  KTAP version 2
>>>  1..1
>>>    KTAP version 2
>>>    1..2
>>>    ok 1 case_1
>>>    skip 2 case_2
>>>  ok 1 suite
>>>
>>> Here is a link to a version of the KUnit parser that is able to parse
>>> the skip test result for KTAP version 2. Note this parser is still able
>>> to parse the "#SKIP" directive.
>>>
>>> Link: https://kunit-review.googlesource.com/c/linux/+/5689
>>>
>>> Signed-off-by: Rae Moar <rmoar@google.com>
>>> ---> 
>>> Note: this patch is based on Frank's ktap_spec_version_2 branch.
>>>
>>>  Documentation/dev-tools/ktap.rst | 27 ++++++++++++++++++---------
>>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/Documentation/dev-tools/ktap.rst b/Documentation/dev-tools/ktap.rst
>>> index ff77f4aaa6ef..f48aa00db8f0 100644
>>> --- a/Documentation/dev-tools/ktap.rst
>>> +++ b/Documentation/dev-tools/ktap.rst
>>> @@ -74,7 +74,8 @@ They are required and must have the format:
>>>  	<result> <number> [<description>][ # [<directive>] [<diagnostic data>]]
>>>  
>>>  The result can be either "ok", which indicates the test case passed,
>>> -or "not ok", which indicates that the test case failed.
>>> +"not ok", which indicates that the test case failed, or "skip", which indicates
>>> +the test case did not run.
>>>  
>>>  <number> represents the number of the test being performed. The first test must
>>>  have the number 1 and the number then must increase by 1 for each additional
>>> @@ -91,12 +92,13 @@ A directive is a keyword that indicates a different outcome for a test other
>>>  than passed and failed. The directive is optional, and consists of a single
>>>  keyword preceding the diagnostic data. In the event that a parser encounters
>>>  a directive it doesn't support, it should fall back to the "ok" / "not ok"
>>> -result.
>>> +/ "skip" result.
>>>  
>>>  Currently accepted directives are:
>>>  
>>> -- "SKIP", which indicates a test was skipped (note the result of the test case
>>> -  result line can be either "ok" or "not ok" if the SKIP directive is used)
>>
>>> +- "SKIP", which indicates a test was skipped (note this is an alternative to
>>> +  the "skip" result type and if the SKIP directive is used, the
>>> +  result can be any type - "ok", "not ok", or "skip")
>>
>> For the "SKIP" directive, result type of either "ok", or "not ok" reflects the
>> current real world usage, which is mixed.  I agree is makes sense to also
>> allow the result type of "skip" with the "SKIP directive.
>>
> 
>> I think it would be good to deprecate the "SKIP" directive, with a scheduled
>> removal in the V3 specification - that would allow plenty of time for test
>> parsers to process both V1 and V2 data, before removing processing of V1 data.
> 
> Since I wrote that paragraph, I have pondered the process of transition from
> V1 to V2, to possibly V3.  It seems to be a complex enough issue that I will
> start a different email thread to gather thoughts, issues, and possible
> directions.

The new thread is now started at:

  https://lore.kernel.org/all/6d4afb49-3cb9-f176-61a2-5bbaab698644@gmail.com/T/#u

-Frank

> 
> -Frank
> 
>>
>> If so, the deprecation plan should be documented.
>>
>>>  - "TODO", which indicates that a test is not expected to pass at the moment,
>>>    e.g. because the feature it is testing is known to be broken. While this>    directive is inherited from TAP, its use in the kernel is discouraged.
>>> @@ -110,7 +112,7 @@ Currently accepted directives are:
>>>  
>>>  The diagnostic data is a plain-text field which contains any additional details
>>>  about why this result was produced. This is typically an error message for ERROR
>>> -or failed tests, or a description of missing dependencies for a SKIP result.
>>> +or failed tests, or a description of missing dependencies for a skipped test.
>>>  
>>>  The diagnostic data field is optional, and results which have neither a
>>>  directive nor any diagnostic data do not need to include the "#" field
>>> @@ -130,11 +132,18 @@ The test "test_case_name" failed.
>>>  
>>>  ::
>>>  
>>> -	ok 1 test # SKIP necessary dependency unavailable
>>> +	skip 1 test # necessary dependency unavailable
>>
>> Maybe add a note that the "skip" result method is preferred over the below
>> "ok ... # SKIP..." example below.
>>
>>>  
>>> -The test "test" was SKIPPED with the diagnostic message "necessary dependency
>>> +The test "test" was skipped with the diagnostic message "necessary dependency
>>>  unavailable".
>>>  
>>> +::
>>> +
>>> +	ok 1 test_2 # SKIP this test should not run
>>> +
>>> +The test "test_2" was skipped with the diagnostic message "this test
>>> +should not run".
>>
>> Maybe add a deprecation note here.
>>
>>> +
>>>  ::
>>>  
>>>  	not ok 1 test # TIMEOUT 30 seconds
>>> @@ -225,7 +234,7 @@ An example format with multiple levels of nested testing:
>>>  	    not ok 1 test_1
>>>  	    ok 2 test_2
>>>  	  not ok 1 test_3
>>> -	  ok 2 test_4 # SKIP
>>> +	  skip 2 test_4
>>>  	not ok 1 example_test_1
>>>  	ok 2 example_test_2
>>>  
>>> @@ -262,7 +271,7 @@ Example KTAP output
>>>  	  ok 1 example_test_1
>>>  	    KTAP version 2
>>>  	    1..2
>>> -	    ok 1 test_1 # SKIP test_1 skipped
>>> +	    skip 1 test_1 # test_1 skipped
>>>  	    ok 2 test_2
>>>  	  ok 2 example_test_2
>>>  	    KTAP version 2
>>>
>>> base-commit: 906f02e42adfbd5ae70d328ee71656ecb602aaf5
>>
>