Message-ID: <CABVgOSmoNaYVcckw3PX7Cf32NjYPSpX3JqGESCqYJvTi6dwAsQ@mail.gmail.com>
Date:   Thu, 12 Aug 2021 15:09:07 +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 2/2] kunit: add 'kunit.action' param to allow listing
 out tests

On Fri, Aug 6, 2021 at 7:51 AM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> Context:
> It's difficult to map a given .kunitconfig => set of enabled tests.
>
> Having a standard, easy way of getting the list could be useful in a
> number of ways. For example, if we also extended kunit.filter_glob to
> allow filtering on tests, this would allow users to run tests cases one
> by one if they wanted to debug hermeticity issues.
>
> This patch:
> * adds a kunit.action module param with one valid non-null value, "list"
> * for the "list" action, it simply prints out "<suite>.<test>"
> * does not itself introduce kunit.py changes to make use of this [1].

I really like this feature, and could live with the implementation,
but do feel like we should probably have a more detailed idea of how
the kunit_tool changes are going to work before settling on it for
sure.

In particular, is the "<suite>.<test>" format the best way of giving
test information, or do we want something more (K)TAP-like. (e.g., a
test hierarchy with all tests marked SKIPed, or otherwise limited in
some way). Maybe that'd allow some of the parser code to be re-used,
and have fewer issues with having two separate ways of representing
the test hierarchy. (What if, e.g., there were test or suite names
with '.' in them?)

On the other hand, this format does make it easier to use the
filter_glob stuff, so it could go either way...

> Note: kunit.filter_glob is respected for this and all future actions.
> Note: we need a TAP header for kunit.py to isolate the KUnit output.

I don't mind having a TAP header here, but we could either:
(a) have a separate header for a test list, and have kunit_tool look
for that as well, to avoid treating this as TAP when it isn't; or
(b) try to standardise a "test list" format as part of KTAP:
https://lore.kernel.org/linux-kselftest/CA+GJov6tdjvY9x12JsJT14qn6c7NViJxqaJk+r-K1YJzPggFDQ@mail.gmail.com/

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

The "action" parameter is fine by me. Do we want to give the default
"run" action a name as well as making it the default?

> Tested:
> $ ./tools/testing/kunit/kunit.py run --kernel_arg=kunit.action=list --raw_output=kunit
> ...
> TAP version 14
> 1..1

I really don't like the test plan line combined with the
"<suite>.<test>" format, particularly since this example notes that
there's only one test (presumably the suite), and then proceeds to
have three top-level things afterwards. It seems pretty misleading to
me.

> example.example_simple_test
> example.example_skip_test
> example.example_mark_skipped_test
> reboot: System halted
>
> [1] The interface for this can work in a few ways. We could add a
> --list_tests flag or a new subcommand. But this change is enough to
> allow people to split each suite into its own invocation, e.g. via a
> short script like:
>
>   #!/bin/bash
>
>   cd $(git rev-parse --show-toplevel)
>
>   for suite in $(
>     ./tools/testing/kunit/kunit.py run --kernel_args=kunit.action=list --raw_output=kunit |
>     sed -n '/^TAP version/,$p' | grep -P -o '^[a-z][a-z0-9_-]+\.' | tr -d '.' | sort -u);
>   do
>     ./tools/testing/kunit/kunit.py run "${suite}"
>   done
>
> Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> ---
> v1 -> v2: write about potential other "actions" in commit desc.
> ---
>  lib/kunit/executor.c | 46 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index acd1de436f59..77d99ee5ed64 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 run at boot-time, e.g. list*");
> +                "Filter which KUnit test suites run at boot-time, e.g. list*");
> +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");
>
>  static char *kunit_shutdown;
>  core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
> @@ -109,6 +116,33 @@ static void kunit_print_tap_header(struct suite_set *suite_set)
>         pr_info("1..%d\n", num_of_suites);
>  }
>
> +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);
> +
> +       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 kunit_suite * const * const *suites;
> @@ -120,10 +154,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 */
>                 for (suites = suite_set.start; suites < suite_set.end; suites++)
> --
> 2.32.0.605.g8dce9f2422-goog
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4000 bytes)

Powered by blists - more mailing lists