[v1,1/2] kunit: improve KTAP compliance of KUnit test output

  Change KUnit test output to comply with KTAP version 1 specifications
found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html.
1) Use "KTAP version 1" instead of "TAP version 14" as test output header
2) Remove '-' between test number and test name on test result lines
2) Add KTAP version lines to each subtest header as well

Original output:

 TAP version 14
 1..1
   # Subtest: kunit-test-suite
   1..3
   ok 1 - kunit_test_1
   ok 2 - kunit_test_2
   ok 3 - kunit_test_3
 # kunit-test-suite: pass:3 fail:0 skip:0 total:3
 # Totals: pass:3 fail:0 skip:0 total:3
 ok 1 - kunit-test-suite

New output:

 KTAP version 1
 1..1
   # Subtest: kunit-test-suite
   KTAP version 1
   1..3
   ok 1 kunit_test_1
   ok 2 kunit_test_2
   ok 3 kunit_test_3
 # kunit-test-suite: pass:3 fail:0 skip:0 total:3
 # Totals: pass:3 fail:0 skip:0 total:3
 ok 1 kunit-test-suite

Signed-off-by: Rae Moar <rmoar@google.com>
---
 lib/kunit/executor.c | 6 +++---
 lib/kunit/test.c     | 5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)


base-commit: 6fe1ad4a156095859721fef85073df3ed43081d4
  

On Fri, Nov 4, 2022 at 12:48 PM Rae Moar <rmoar@google.com> wrote:
>
> Change KUnit test output to comply with KTAP version 1 specifications
> found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html.
> 1) Use "KTAP version 1" instead of "TAP version 14" as test output header
> 2) Remove '-' between test number and test name on test result lines
> 2) Add KTAP version lines to each subtest header as well
>
> Original output:
>
>  TAP version 14
>  1..1
>    # Subtest: kunit-test-suite
>    1..3
>    ok 1 - kunit_test_1
>    ok 2 - kunit_test_2
>    ok 3 - kunit_test_3
>  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
>  # Totals: pass:3 fail:0 skip:0 total:3
>  ok 1 - kunit-test-suite
>
> New output:
>
>  KTAP version 1
>  1..1
>    # Subtest: kunit-test-suite
>    KTAP version 1
>    1..3
>    ok 1 kunit_test_1
>    ok 2 kunit_test_2
>    ok 3 kunit_test_3
>  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
>  # Totals: pass:3 fail:0 skip:0 total:3
>  ok 1 kunit-test-suite
>
> Signed-off-by: Rae Moar <rmoar@google.com>

Reviewed-by: Daniel Latypov <dlatypov@google.com>

I had been worried initially that this needed kunit_parser.py changes to work.
But it doesn't, this change can go in now with minimal side effects.

More details below:
This code treats the new "KTAP version 1" subheaders as random kernel
output, so it puts it into the `test.log`

E.g. if I change to `not ok 1 kunit_test_1`, it'll print
  KTAP version 1
  1..3
  not ok 1 kunit_test_1

After the next patch, it just prints
  not ok 1 kunit_test_1

This bit of extra output on failure is something we can live with,
esp. since it gets addressed by the next patch.
  

On Sat, Nov 5, 2022 at 3:48 AM Rae Moar <rmoar@google.com> wrote:
>
> Change KUnit test output to comply with KTAP version 1 specifications
> found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html.
> 1) Use "KTAP version 1" instead of "TAP version 14" as test output header
> 2) Remove '-' between test number and test name on test result lines
> 2) Add KTAP version lines to each subtest header as well
>
> Original output:
>
>  TAP version 14
>  1..1
>    # Subtest: kunit-test-suite
>    1..3
>    ok 1 - kunit_test_1
>    ok 2 - kunit_test_2
>    ok 3 - kunit_test_3
>  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
>  # Totals: pass:3 fail:0 skip:0 total:3
>  ok 1 - kunit-test-suite
>
> New output:
>
>  KTAP version 1
>  1..1
>    # Subtest: kunit-test-suite
>    KTAP version 1
>    1..3
>    ok 1 kunit_test_1
>    ok 2 kunit_test_2
>    ok 3 kunit_test_3
>  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
>  # Totals: pass:3 fail:0 skip:0 total:3
>  ok 1 kunit-test-suite
>
> Signed-off-by: Rae Moar <rmoar@google.com>
> ---

Thanks very much for doing this. It's always been slightly
embarrassing that we weren't perfectly following our own spec!

I think this series is good enough, but have a minor suggestion: could
we swap the order of these two patches?
I'd rather have the parser changes go in first.

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@google.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.


>         kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd",
>                   kunit_suite_num_test_cases(suite));
>  }
> @@ -175,13 +176,13 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
>          * representation.
>          */
>         if (suite)
> -               pr_info("%s %zd - %s%s%s\n",
> +               pr_info("%s %zd %s%s%s\n",
>                         kunit_status_to_ok_not_ok(status),
>                         test_number, description, directive_header,
>                         (status == KUNIT_SKIPPED) ? directive : "");
>         else
>                 kunit_log(KERN_INFO, test,
> -                         KUNIT_SUBTEST_INDENT "%s %zd - %s%s%s",
> +                         KUNIT_SUBTEST_INDENT "%s %zd %s%s%s",
>                           kunit_status_to_ok_not_ok(status),
>                           test_number, description, directive_header,
>                           (status == KUNIT_SKIPPED) ? directive : "");
>
> base-commit: 6fe1ad4a156095859721fef85073df3ed43081d4
> --
> 2.38.1.431.g37b22c650d-goog
>
  

On Tue, Nov 15, 2022 at 2:27 AM David Gow <davidgow@google.com> wrote:
>
> On Sat, Nov 5, 2022 at 3:48 AM Rae Moar <rmoar@google.com> wrote:
> >
> > Change KUnit test output to comply with KTAP version 1 specifications
> > found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html.
> > 1) Use "KTAP version 1" instead of "TAP version 14" as test output header
> > 2) Remove '-' between test number and test name on test result lines
> > 2) Add KTAP version lines to each subtest header as well
> >
> > Original output:
> >
> >  TAP version 14
> >  1..1
> >    # Subtest: kunit-test-suite
> >    1..3
> >    ok 1 - kunit_test_1
> >    ok 2 - kunit_test_2
> >    ok 3 - kunit_test_3
> >  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
> >  # Totals: pass:3 fail:0 skip:0 total:3
> >  ok 1 - kunit-test-suite
> >
> > New output:
> >
> >  KTAP version 1
> >  1..1
> >    # Subtest: kunit-test-suite
> >    KTAP version 1
> >    1..3
> >    ok 1 kunit_test_1
> >    ok 2 kunit_test_2
> >    ok 3 kunit_test_3
> >  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
> >  # Totals: pass:3 fail:0 skip:0 total:3
> >  ok 1 kunit-test-suite
> >
> > Signed-off-by: Rae Moar <rmoar@google.com>
> > ---
>
> Thanks very much for doing this. It's always been slightly
> embarrassing that we weren't perfectly following our own spec!
>
> I think this series is good enough, but have a minor suggestion: could
> we swap the order of these two patches?
> I'd rather have the parser changes go in first.

Hi David!
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@google.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.

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.

>
> >         kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd",
> >                   kunit_suite_num_test_cases(suite));
> >  }
> > @@ -175,13 +176,13 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
> >          * representation.
> >          */
> >         if (suite)
> > -               pr_info("%s %zd - %s%s%s\n",
> > +               pr_info("%s %zd %s%s%s\n",
> >                         kunit_status_to_ok_not_ok(status),
> >                         test_number, description, directive_header,
> >                         (status == KUNIT_SKIPPED) ? directive : "");
> >         else
> >                 kunit_log(KERN_INFO, test,
> > -                         KUNIT_SUBTEST_INDENT "%s %zd - %s%s%s",
> > +                         KUNIT_SUBTEST_INDENT "%s %zd %s%s%s",
> >                           kunit_status_to_ok_not_ok(status),
> >                           test_number, description, directive_header,
> >                           (status == KUNIT_SKIPPED) ? directive : "");
> >
> > base-commit: 6fe1ad4a156095859721fef85073df3ed43081d4
> > --
> > 2.38.1.431.g37b22c650d-goog
> >
  

On Tue, Nov 15, 2022 at 12:18 PM Rae Moar <rmoar@google.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@google.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.
  

On Tue, Nov 15, 2022 at 5:41 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Tue, Nov 15, 2022 at 12:18 PM Rae Moar <rmoar@google.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@google.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
>


This was partially discussed off the mailing list. I am still a bit
wary to change the output to this format because it is not strictly
compliant with the KTAP v1 spec which requires the test plan to be the
line directly below the version line. However, I do think that this
location makes much more sense for the "# Subtest" line and since this
is a diagnostic line and likely won't break any existing parsers, I
will be changing to this format in v2 as well as adjusting the parser
to allow for this format instead.

The optional top-level "# Subtest" line was also discussed off the
mailing list. It was generally agreed that this could be useful in the
future but not as an addition for this patch. However, the parser
should be altered such that if the line is present, the parser will
not crash but also not actively print the provided test name.

As discussed previously in this patch series, a test name line could
be helpful to propose for KTAP v2 as a replacement to this "# Subtest"
line, which is present in the TAP version 14 spec.

Rae


>
> >
> > 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.