On Thu, Aug 12, 2021 at 12:09 AM David Gow <davidgow@xxxxxxxxxx> wrote: > > On Fri, Aug 6, 2021 at 7:51 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/ 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@xxxxxxxxxx> > > --- > > 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 > >
