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;
MIME-Version: 1.0
References: <20221121184743.1123556-1-rmoar@google.com> <20221121184743.1123556-2-rmoar@google.com>
 <CADYN=9KUBaOG52SRtseLTkgkMzvzyCzCs3tR1vOk4yvkMSTJ2A@mail.gmail.com> <CAGS_qxp7eLWJ+2DdzcdEezw067xxGO+v6VQwJD=jkBR-YKCefQ@mail.gmail.com>
In-Reply-To: <CAGS_qxp7eLWJ+2DdzcdEezw067xxGO+v6VQwJD=jkBR-YKCefQ@mail.gmail.com>
From:   Rae Moar <rmoar@google.com>
Date:   Tue, 22 Nov 2022 15:19:28 -0500
Message-ID: <CA+GJov7v8hbJ6A2ehfpZvG3+e181D0JrJRYDTQc9_QMM4G0HQA@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] kunit: improve KTAP compliance of KUnit test output
To:     Daniel Latypov <dlatypov@google.com>,
        Anders Roxell <anders.roxell@linaro.org>
Cc:     brendanhiggins@google.com, davidgow@google.com,
        skhan@linuxfoundation.org, mauro.chehab@linux.intel.com,
        kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, isabbasso@riseup.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Nov 22, 2022 at 12:14 PM Daniel Latypov <dlatypov@google.com> wrote=
:
>
> On Tue, Nov 22, 2022 at 1:17 AM Anders Roxell <anders.roxell@linaro.org> =
wrote:
> >
> > On Mon, 21 Nov 2022 at 19:48, Rae Moar <rmoar@google.com> wrote:
> > >
> > > Change KUnit test output to better comply with KTAP v1 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 he=
ader
> > > 2) Remove '-' between test number and test name on test result lines
> > > 2) Add KTAP version lines to each subtest header as well
> > >
> > > Note that the new KUnit output still includes the =E2=80=9C# Subtest=
=E2=80=9D line now
> > > located after the KTAP version line. This does not completely match t=
he
> > > KTAP v1 spec but since it is classified as a diagnostic line, it is n=
ot
> > > expected to be disruptive or break any existing parsers. This
> > > =E2=80=9C# Subtest=E2=80=9D line comes from the TAP 14 spec
> > > (https://testanything.org/tap-version-14-specification.html)
> > > and it is used to define the test name before the results.
> > >
> > > 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
> > >    KTAP version 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
> > >
> > > Signed-off-by: Rae Moar <rmoar@google.com>
> > > Reviewed-by: Daniel Latypov <dlatypov@google.com>
> > > Reviewed-by: David Gow <davidgow@google.com>
> >
> > I tried this patch, see the full boot log [1] and I can still see some
> >  tests that output the "old" format with 'ok 1 - kunit_test_1', for exa=
mple
> >
> > ok 1 - 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
> >
> > Isn't this something that should be converted too?

Hello! Sorry for missing that. You are definitely right. Those results shou=
ld
be converted as well.

>
> Yes, thanks for catching that.
> That's what I get from only looking over the diff this time since I
> thought I remembered the code...
>
> We also want this diff to fix a) debugfs, b) subtests.
>
> diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
> index 1048ef1b8d6e..de0ee2e03ed6 100644
> --- a/lib/kunit/debugfs.c
> +++ b/lib/kunit/debugfs.c
> @@ -63,7 +63,7 @@ static int debugfs_print_results(struct seq_file
> *seq, void *v)
>         kunit_suite_for_each_test_case(suite, test_case)
>                 debugfs_print_result(seq, suite, test_case);
>
> -       seq_printf(seq, "%s %d - %s\n",
> +       seq_printf(seq, "%s %d %s\n",
>                    kunit_status_to_ok_not_ok(success), 1, suite->name);
>         return 0;
>  }
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 19344cb501c5..c9d57a1d9524 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -556,7 +556,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>
>                                 kunit_log(KERN_INFO, &test,
>                                           KUNIT_SUBTEST_INDENT
> KUNIT_SUBTEST_INDENT
> -                                         "%s %d - %s",
> +                                         "%s %d %s",
>
> kunit_status_to_ok_not_ok(test.status),
>                                           test.param_index + 1, param_des=
c);
>
> Daniel

Thanks Daniel!

I think that should do the trick with the addition of adding the "KTAP
version 1" line, which you can do with the following diff:

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c9d57a1d9524..1c9d8d962d67 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -543,6 +543,8 @@ int kunit_run_tests(struct kunit_suite *suite)
                        /* Get initial param. */
                        param_desc[0] =3D '\0';
                        test.param_value =3D
test_case->generate_params(NULL, param_desc);
+                       kunit_log(KERN_INFO, &test,
KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
+                                 "KTAP version 1\n");
                        kunit_log(KERN_INFO, &test,
KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
                                  "# Subtest: %s", test_case->name);

Going to run through some more examples to make sure this version
works otherwise I will make a v3 with the addition of fixing any merge
conflicts.

Rae