lkml.org 
[lkml]   [2021]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v2 2/2] kunit: add 'kunit.action' param to allow listing out tests
    On Thu, Aug 12, 2021 at 12:09 AM David Gow <davidgow@google.com> wrote:
    >
    > On Fri, Aug 6, 2021 at 7:51 AM Daniel Latypov <dlatypov@google.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...

    Yeah, the main point of this is to be consistent with filter_glob and
    test-level filtering.
    If we can come up with a more generic, "TAP-like" identifier for
    tests, we could use that for here and for filtering.

    Using "suite.test" seems to be relatively standard, hence why I've
    gone with that for both.
    E.g. in python:
    https://docs.python.org/3/library/unittest.html#command-line-interface

    $ python -m unittest test_module.TestClass.test_method

    Though I've only ever used that without "test_module" as

    $ ./tools/testing/kunit/kunit_tool_test.py KconfigTest
    ...
    ----------------------------------------------------------------------
    Ran 3 tests in 0.001s

    OK
    $ ./tools/testing/kunit/kunit_tool_test.py KconfigTest.test_is_subset_of
    .
    ----------------------------------------------------------------------
    Ran 1 test in 0.001s

    OK

    So I'd really prefer we stick with the current format, tbh.

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

    b. feels a bit overkill.

    I agree it's very hacky.
    I didn't want to mess around too much with the parser code while Rae
    was working on it.

    But we could change the parser code:
    * `func extract_tap_lines()` => `func extract_kunit_lines()`
    * kunit_start_re => `TAP...|KUNIT OTHER OUTPUT START MARKER`

    and use that marker here.

    I'm fine with adding a new marker, but I assumed we'd probably need to
    spend a good amount of time bikeshedding what exactly this new header
    would be :P
    Whereas this works right now and is ugly in a way that I don't think
    most people would notice.

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

    I originally did that, but then realized we'd probably never use an
    explicit "run" action ever.
    I've added it back in locally and will include it in a v2.

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

    \
     
     \ /
      Last update: 2021-08-12 19:31    [W:3.339 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site