Message-ID: <CAGS_qxpDACGJ=sW7-3ivVw5X92K7rrmoiLyj+SS-OdvGg0i1rg@mail.gmail.com>
Date:   Wed, 29 Sep 2021 09:55:38 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2 1/3] kunit: add 'kunit.action' param to allow listing
 out tests

On Tue, Sep 28, 2021 at 9:38 PM David Gow <davidgow@...gle.com> wrote:
>
> On Wed, Sep 29, 2021 at 6:29 AM Daniel Latypov <dlatypov@...gle.com> wrote:
> >
> > Context:
> > It's difficult to map a given .kunitconfig => set of enabled tests.
> > Letting kunit.py figure that out would be useful.
> >
> > This patch:
> > * is intended to be an implementation detail used only by kunit.py
> > * adds a kunit.action module param with one valid non-null value, "list"
> > * for the "list" action, it simply prints out "<suite>.<test>"
> > * leaves the kunit.py changes to make use of this for another patch.
> >
> > Note: kunit.filter_glob is respected for this and all future actions.
> >
> > Hack: we print a TAP header and fake test plan to allow kunit.py to
> > use the same code to pick up KUnit output that it does for normal tests.
> > Since this is intended to be an implementation detail, it seems fine for
> > now. Maybe in the future we ouptut each test as SKIPPED or the like.
>
> I'm still a little uneasy using the "TAP version 14" header here, and
> then proceeding to include a list of tests which, in and of itself,
> isn't valid TAP.
> I don't think we need to solve this perfectly now: we can always
> change it if something comes out of, e.g., the KTAP standardisation,
> but I'd rather we have something -- even something temporary -- which
> is easily distinguishable from an actual TAP result.
>
> Even if we had "TAP version 14 - test list" or something so that

Currently, the regex kunit_parser.py uses is anchored with $.
So I'm not able to add anything at the end of the line.

We could potentially add a new line afterwards with "# KUnit Test
List" or similar?

I'd prefer not to since I'm not sure it matters _too_ much.
Someone would have to go out of their way to print the list of tests,
and if so, I'm not sure they should be trying to parse TAP output from
it.

> kunit_tool picked up on it without further changes, that'd be fine,
> though something like "KUnit Test List" would be better.
>
> Also, I'd still rather we lose the "1..1" test suite list, though I
> can live without if I have to, given that it is actually giving the
> correct number of suites.
>
> From the kernel side, all this should take is replacing the call to
> kunit_print_tap_header() with a direct pr_info() call. Maybe there'd
> need to be some minor kunit_tool changes in patch 3, too, but nothing
> excessive.

That's fair. I had mainly just wanted to avoid hard-copying a copy of
the TAP version.

I've made the change so it doesn't print the test plan and it works.

The only change needed in patch 3 is
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index e7b92caba53d..ca4f877234c2 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -101,8 +101,7 @@ def _list_tests(linux:
kunit_kernel.LinuxSourceTree, request: KunitExecRequest)
                           filter_glob=request.filter_glob,
                           build_dir=request.build_dir)
        output = kunit_parser.extract_tap_lines(output)
-       # Hack! Drop the TAP version header and top-level test plan.
-       output.pop()
+       # Hack! Drop the dummy TAP version header that the executor prints out.
        output.pop()
        return list(output)

>
> Also, nit: "ouptut" should be "output"

Ah, fixed.

> >
> > Go with a more generic "action" param, since it seems like we might
> > eventually have more modes besides just running or listing tests, e.g.
> > * perhaps a benchmark mode that reruns test cases and reports timing
> > * perhaps a deflake mode that reruns test cases that failed
> > * perhaps a mode where we randomize test order to try and catch
> >   hermeticity bugs like "test a only passes if run after test b"
> >
> > Tested:
> > $ ./tools/testing/kunit/kunit.py run --kernel_arg=kunit.action=list --raw_output=kunit
> > ...
> > TAP version 14
> > 1..1
> > example.example_simple_test
> > example.example_skip_test
> > example.example_mark_skipped_test
> > reboot: System halted
> >
> > Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> > ---
>
> Otherwise, I'm quite happy with this: it works well on my end, and the
> implementation makes sense.
>
> So this is:
> Reviewed-by: David Gow <davidgow@...gle.com>
>
> (But I'd rather the TAP header bit change if possible...)
>
> -- David
>
> >  lib/kunit/executor.c | 45 +++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 40 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index bab3ab940acc..8b38c91b4fac 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -15,9 +15,16 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
> >  #if IS_BUILTIN(CONFIG_KUNIT)
> >
> >  static char *filter_glob_param;
> > +static char *action_param;
> > +
> >  module_param_named(filter_glob, filter_glob_param, charp, 0);
> >  MODULE_PARM_DESC(filter_glob,
> >                 "Filter which KUnit test suites/tests run at boot-time, e.g. list* or list*.*del_test");
> > +module_param_named(action, action_param, charp, 0);
> > +MODULE_PARM_DESC(action,
> > +                "Changes KUnit executor behavior, valid values are:\n"
> > +                "<none>: run the tests like normal\n"
> > +                "'list' to list test names instead of running them.\n");
> >
> >  /* glob_match() needs NULL terminated strings, so we need a copy of filter_glob_param. */
> >  struct kunit_test_filter {
> > @@ -196,9 +203,35 @@ static void kunit_print_tap_header(struct suite_set *suite_set)
> >         pr_info("1..%d\n", num_of_suites);
> >  }
> >
> > -int kunit_run_all_tests(void)
> > +static void kunit_exec_run_tests(struct suite_set *suite_set)
> >  {
> >         struct kunit_suite * const * const *suites;
> > +
> > +       kunit_print_tap_header(suite_set);
> > +
> > +       for (suites = suite_set->start; suites < suite_set->end; suites++)
> > +               __kunit_test_suites_init(*suites);
> > +}
> > +
> > +static void kunit_exec_list_tests(struct suite_set *suite_set)
> > +{
> > +       unsigned int i;
> > +       struct kunit_suite * const * const *suites;
> > +       struct kunit_case *test_case;
> > +
> > +       /* Hack: print a tap header so kunit.py can find the start of KUnit output. */
> > +       kunit_print_tap_header(suite_set);
>
> As noted, would rather this be something like
> pr_info("KUnit Test List");
>
>
> > +
> > +       for (suites = suite_set->start; suites < suite_set->end; suites++)
> > +               for (i = 0; (*suites)[i] != NULL; i++) {
> > +                       kunit_suite_for_each_test_case((*suites)[i], test_case) {
> > +                               pr_info("%s.%s\n", (*suites)[i]->name, test_case->name);
> > +                       }
> > +               }
> > +}
> > +
> > +int kunit_run_all_tests(void)
> > +{
> >         struct suite_set suite_set = {
> >                 .start = __kunit_suites_start,
> >                 .end = __kunit_suites_end,
> > @@ -207,10 +240,12 @@ int kunit_run_all_tests(void)
> >         if (filter_glob_param)
> >                 suite_set = kunit_filter_suites(&suite_set, filter_glob_param);
> >
> > -       kunit_print_tap_header(&suite_set);
> > -
> > -       for (suites = suite_set.start; suites < suite_set.end; suites++)
> > -               __kunit_test_suites_init(*suites);
> > +       if (!action_param)
> > +               kunit_exec_run_tests(&suite_set);
> > +       else if (strcmp(action_param, "list") == 0)
> > +               kunit_exec_list_tests(&suite_set);
> > +       else
> > +               pr_err("kunit executor: unknown action '%s'\n", action_param);
> >
> >         if (filter_glob_param) { /* a copy was made of each array */
> >                 kunit_free_suite_set(suite_set);
> > --
> > 2.33.0.685.g46640cef36-goog
> >

Powered by blists - more mailing lists