Date:   Tue, 15 Nov 2022 14:41:05 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Rae Moar <rmoar@...gle.com>
Cc:     David Gow <davidgow@...gle.com>, brendanhiggins@...gle.com,
        skhan@...uxfoundation.org, mauro.chehab@...ux.intel.com,
        kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        Isabella Basso <isabbasso@...eup.net>,
        Anders Roxell <anders.roxell@...aro.org>
Subject: Re: [PATCH v1 1/2] kunit: improve KTAP compliance of KUnit test output

On Tue, Nov 15, 2022 at 12:18 PM Rae Moar <rmoar@...gle.com> wrote:
> Yes, I agree. I think it does make more sense to provide KTAP
> compatibility with the parser before changing the test output. This
> would also help to solve the issue that Daniel brought up on this
> patch about the "KTAP version 1" line and test plan being stored
> in the test.log as random kernel output. I will swap the patches in
> the v2 of this patch series.
>
> >
> > I'd also be curious to see if this is likely to break anyone else's
> > (K)TAP parsers.
> >
> > +Isabella, Anders: do these changes break the IGT or LKFT TAP/KTAP
> > parsing? From a quick look at [1] and [2], we're probably okay??
> >
> > [1]: https://gitlab.freedesktop.org/isinyaaa/igt-gpu-tools/-/commit/1a84306425e975377eb79c031bf1de147bd44e46
> > [2]: https://github.com/Linaro/test-definitions/blob/master/automated/linux/kunit/kunit.sh
> >
> > I also looked into the possibility of moving or removing the Subtest
> > line, but upon further thought (see below), it's probably best to keep
> > it as-is here for now. That should be the closest to the current spec,
> > and we can possibly find a better way to provide the name in KTAPv2.
> >
> > Reviewed-by: David Gow <davidgow@...gle.com>
> >
> > Cheers,
> > -- David
> >
> > >  lib/kunit/executor.c | 6 +++---
> > >  lib/kunit/test.c     | 5 +++--
> > >  2 files changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > > index 9bbc422c284b..74982b83707c 100644
> > > --- a/lib/kunit/executor.c
> > > +++ b/lib/kunit/executor.c
> > > @@ -166,7 +166,7 @@ static void kunit_exec_run_tests(struct suite_set *suite_set)
> > >  {
> > >         size_t num_suites = suite_set->end - suite_set->start;
> > >
> > > -       pr_info("TAP version 14\n");
> > > +       pr_info("KTAP version 1\n");
> > >         pr_info("1..%zu\n", num_suites);
> > >
> > >         __kunit_test_suites_init(suite_set->start, num_suites);
> > > @@ -177,8 +177,8 @@ static void kunit_exec_list_tests(struct suite_set *suite_set)
> > >         struct kunit_suite * const *suites;
> > >         struct kunit_case *test_case;
> > >
> > > -       /* Hack: print a tap header so kunit.py can find the start of KUnit output. */
> > > -       pr_info("TAP version 14\n");
> > > +       /* Hack: print a ktap header so kunit.py can find the start of KUnit output. */
> > > +       pr_info("KTAP version 1\n");
> > >
> > >         for (suites = suite_set->start; suites < suite_set->end; suites++)
> > >                 kunit_suite_for_each_test_case((*suites), test_case) {
> > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > > index 90640a43cf62..b541d59a05c3 100644
> > > --- a/lib/kunit/test.c
> > > +++ b/lib/kunit/test.c
> > > @@ -151,6 +151,7 @@ static void kunit_print_suite_start(struct kunit_suite *suite)
> > >  {
> > >         kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
> > >                   suite->name);
> > > +       kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "KTAP version 1\n");
> >
> > Would it make sense to have the Subtest line _after_ the KTAP line here?
> >
> > Given KTAP doesn't specify the "Subtest" line at all, I guess it doesn't matter.
> >
> > A part of me feels that the "KTAP version 1" line should be the
> > _first_ line of the subtest output, but that would introduce a line
> > between it and the test plan, which goes against the spec.
> >
> > We could also just get rid of the "Subtest" line, given it's not
> > technically required, though having the test name before the results
> > is really useful.
> >
> > Having tried all three options while writing this email, I think it's
> > probably best to leave this patch as is (with the Subtest line
> > followed by the KTAP line), and discuss standardising something
> > similar as part of the KTAP v2 spec.
> >
>
> I also struggle to decide how the "Subtest" line should be handled. Since
> the current KTAP v1 spec does not provide a way to declare the name of
> a test with subtests prior to the results, I think it is important to continue
> to include this Subtest line because it provides that functionality.
> Additionally,
> the line is not expected to be very disruptive for other parsers because it
> is read as a diagnostic line.

Yeah, since it's going to be ignored as a diagnostic line, I think
we're largely free to put it where we want?

I'm actually leaning towards making things more uniform e.g.

KTAP version 1
# Subtest: optionally set for the top-level test!
1..2
  KTAP version 1
  # Subtest: suite1
  1..1
  ok 1 - test1
 ok 1 -suite1
 // etc.

Then we can simplify the parser by not differentiating (as much)
between the top-level test and subtests.
This also simplifies parsing multiple KTAP documents (e.g. for
supporting modules, etc.)

We'll probably talk about this offline soon, but I wanted to put this out there.

Daniel


>
> The location of the "Subtest" line before the KTAP version line is potentially
> not the most optimal but it seems to be the best choice while ensuring
> compatibility with the current KTAP v1 spec. I recommend that we discuss
> a standardized replacement for this "Subtest" line in the KTAP v2 spec.

Powered by blists - more mailing lists