MIME-Version: 1.0
References: <20230310222002.3633162-1-rmoar@google.com> <490271eb-1429-2217-6e38-837c6e5e328b@gmail.com>
In-Reply-To: <490271eb-1429-2217-6e38-837c6e5e328b@gmail.com>
From:   Rae Moar <rmoar@google.com>
Date:   Tue, 14 Mar 2023 18:20:42 -0400
Message-ID: <CA+GJov4hw7yDRo2_uYWWe0chmBEtUh+t8aYhxSDXp2zch9cYvg@mail.gmail.com>
Subject: Re: [KTAP V2 PATCH] ktap_v2: add skip test result
To:     Frank Rowand <frowand.list@gmail.com>
Cc:     davidgow@google.com, skhan@linuxfoundation.org,
        keescook@chromium.org, Tim.Bird@sony.com,
        brendanhiggins@google.com, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Mar 11, 2023 at 10:52=E2=80=AFPM Frank Rowand <frowand.list@gmail.c=
om> 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 quic=
kly
> > skimming through KTAP results.
> >
> > The "skip" result would provide an alternative that could make it clear=
er
> > 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 d=
ata>]]
> >
> >  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 addi=
tional
> > @@ -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 s=
ingle
> >  keyword preceding the diagnostic data. In the event that a parser enco=
unters
> >  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 t=
est 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 alternat=
ive 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" reflect=
s 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 schedu=
led
> removal in the V3 specification - that would allow plenty of time for tes=
t
> parsers to process both V1 and V2 data, before removing processing of V1 =
data.
>
> If so, the deprecation plan should be documented.
>

Hi Frank!

This is a great point. I think it is necessary to add specifications
on how the SKIP directive will be deprecated. I will be taking all of
these suggestions when I make a v2.

Also, just letting you know I am planning on sending out two more KTAP
v2 proposals in the next few days.

Thanks for your insight.
-Rae

> >  - "TODO", which indicates that a test is not expected to pass at the m=
oment,
> >    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 discoura=
ged.
> > @@ -110,7 +112,7 @@ Currently accepted directives are:
> >
> >  The diagnostic data is a plain-text field which contains any additiona=
l 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 r=
esult.
> > +or failed tests, or a description of missing dependencies for a skippe=
d 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 belo=
w
> "ok ... # SKIP..." example below.
>

Will add this to v2.

> >
> > -The test "test" was SKIPPED with the diagnostic message "necessary dep=
endency
> > +The test "test" was skipped with the diagnostic message "necessary dep=
endency
> >  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.
>

WIll add this to v2.

> > +
> >  ::
> >
> >       not ok 1 test # TIMEOUT 30 seconds
> > @@ -225,7 +234,7 @@ An example format with multiple levels of nested te=
sting:
> >           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
>