Message-ID: <CABVgOSmbROsRBgA+rfJGwTcsFCmgUmrEgLLVxxr7A-EKLvcj_Q@mail.gmail.com>
Date:   Wed, 29 Sep 2021 12:38:21 +0800
From:   David Gow <davidgow@...gle.com>
To:     Daniel Latypov <dlatypov@...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 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
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.

Also, nit: "ouptut" should be "output"
>
> 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