On Wed, Sep 29, 2021 at 7:27 PM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Thu, Sep 30, 2021 at 3:54 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> >
> > The new --run_isolated flag makes the tool boot the kernel once per
> > suite or test, preventing leftover state from one suite to impact the
> > other. This can be useful as a starting point to debugging test
> > hermeticity issues.
> >
> > Note: it takes a lot longer, so people should not use it normally.
> >
> > Consider the following very simplified example:
> >
> > bool disable_something_for_test = false;
> > void function_being_tested() {
> > ...
> > if (disable_something_for_test) return;
> > ...
> > }
> >
> > static void test_before(struct kunit *test)
> > {
> > disable_something_for_test = true;
> > function_being_tested();
> > /* oops, we forgot to reset it back to false */
> > }
> >
> > static void test_after(struct kunit *test)
> > {
> > /* oops, now "fixing" test_before can cause test_after to fail! */
> > function_being_tested();
> > }
> >
> > Presented like this, the issues are obvious, but it gets a lot more
> > complicated to track down as the amount of test setup and helper
> > functions increases.
> >
> > Another use case is memory corruption. It might not be surfaced as a
> > failure/crash in the test case or suite that caused it. I've noticed in
> > kunit's own unit tests, the 3rd suite after might be the one to finally
> > crash after an out-of-bounds write, for example.
> >
> > Example usage:
> >
> > Per suite:
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit --run_isolated=suite
> > ...
> > Starting KUnit Kernel (1/7)...
> > ============================================================
> > ======== [PASSED] kunit_executor_test ========
> > ....
> > Testing complete. 5 tests run. 0 failed. 0 crashed. 0 skipped.
> > Starting KUnit Kernel (2/7)...
> > ============================================================
> > ======== [PASSED] kunit-try-catch-test ========
> > ...
> >
> > Per test:
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit --run_isolated=test
> > Starting KUnit Kernel (1/23)...
> > ============================================================
> > ======== [PASSED] kunit_executor_test ========
> > [PASSED] parse_filter_test
> > ============================================================
> > Testing complete. 1 tests run. 0 failed. 0 crashed. 0 skipped.
> > Starting KUnit Kernel (2/23)...
> > ============================================================
> > ======== [PASSED] kunit_executor_test ========
> > [PASSED] filter_subsuite_test
> > ...
> >
> > It works with filters as well:
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=lib/kunit --run_isolated=suite example
> > ...
> > Starting KUnit Kernel (1/1)...
> > ============================================================
> > ======== [PASSED] example ========
> > ...
> >
> > It also handles test filters, '*.*skip*' runs these 3 tests:
> > kunit_status.kunit_status_mark_skipped_test
> > example.example_skip_test
> > example.example_mark_skipped_test
> >
> > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
> > ---
>
> Thanks. This is good. A part of me still would've preferred the TAP
> header to have been altered, but it probably makes more sense to leave
> that until after Rae's parser rework patch anyway, which has better
> support for multiple possible TAP headers anyway.
>
> I did find an issue when running this under qemu/i386: a timing
> problem with interleaved lines. We could do something drastic, like
> having a marker at the start of every line to identify which ones are
> tests, but that does seem like overkill for a (hopefully) rare
> problem. Just ignoring obviously invalid lines should do it. Futher
> details below.
>
> -- David
>
> > tools/testing/kunit/kunit.py | 95 ++++++++++++++++++++------
> > tools/testing/kunit/kunit_tool_test.py | 40 +++++++++++
> > 2 files changed, 114 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index 5e717594df5b..b9d63f558765 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -16,7 +16,7 @@ assert sys.version_info >= (3, 7), "Python version is too old"
> >
> > from collections import namedtuple
> > from enum import Enum, auto
> > -from typing import Iterable
> > +from typing import Iterable, List
> >
> > import kunit_config
> > import kunit_json
> > @@ -31,13 +31,13 @@ KunitBuildRequest = namedtuple('KunitBuildRequest',
> > ['jobs', 'build_dir', 'alltests',
> > 'make_options'])
> > KunitExecRequest = namedtuple('KunitExecRequest',
> > - ['timeout', 'build_dir', 'alltests',
> > - 'filter_glob', 'kernel_args'])
> > + ['timeout', 'build_dir', 'alltests',
> > + 'filter_glob', 'kernel_args', 'run_isolated'])
> > KunitParseRequest = namedtuple('KunitParseRequest',
> > ['raw_output', 'build_dir', 'json'])
> > KunitRequest = namedtuple('KunitRequest', ['raw_output','timeout', 'jobs',
> > 'build_dir', 'alltests', 'filter_glob',
> > - 'kernel_args', 'json', 'make_options'])
> > + 'kernel_args', 'run_isolated', 'json', 'make_options'])
> >
> > KernelDirectoryPath = sys.argv[0].split('tools/testing/kunit/')[0]
> >
> > @@ -91,23 +91,68 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > 'built kernel successfully',
> > build_end - build_start)
> >
> > +def _list_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -> List[str]:
> > + args = ['kunit.action=list']
> > + if request.kernel_args:
> > + args.extend(request.kernel_args)
> > +
> > + output = linux.run_kernel(args=args,
> > + timeout=None if request.alltests else request.timeout,
> > + filter_glob=request.filter_glob,
> > + build_dir=request.build_dir)
> > + lines = kunit_parser.extract_tap_lines(output)
> > + # Hack! Drop the dummy TAP version header that the executor prints out.
> > + lines.pop()
> > + return list(lines)
> > +
> > +def _suites_from_test_list(tests: List[str]) -> List[str]:
> > + """Extracts all the suites from an ordered list of tests."""
> > + suites = [] # type: List[str]
> > + for t in tests:
> > + parts = t.split('.', maxsplit=2)
> > + if len(parts) != 2:
> > + raise ValueError(f'internal KUnit error, test name should be of the form "<suite>.<test>", got "{t}"')
>
> It turns out that this can trigger on some machines/architectures if
> there are other lines of kernel output which either get interspersed
> in the test list, or -- more likely -- between the test list and the
> "Restarting System" line.
>
> On i386, under qemu, I'm seeing this output:
> $ qemu-system-x86_64 -nodefaults -m 1024 -kernel
> .kunit/arch/x86/boot/bzImage -append 'kunit.action=list mem=1G
> console=tty kunit_shutdown=halt console=ttyS0 kunit_shutdown=reboot'
> -no-reboot -nographic -serial stdio
> ...
> property-entry.pe_test_reference
> random: fast init done
> input: ImExPS/2 Generic Explorer Mouse as
> /devices/platform/i8042/serio1/input/input2
> reboot: Restarting system
> reboot: machine restart
>
> Which translates into the following kunit_tool error:
> $ ./tools/testing/kunit/kunit.py run --run_isolated=suite --arch=i386
> ...
> File "./tools/testing/kunit/kunit.py", line 114, in _suites_from_test_list
> raise ValueError(f'internal KUnit error, test name should be of the
> form "<suite>.<test>", got "{t}"')
> ValueError: internal KUnit error, test name should be of the form
> "<suite>.<test>", got "random: fast init done"
>
>
> Could we maybe ignore entries of the incorrect form?
I'm thinking we change _list_tests() above like
- return list(lines)
+
+ # Filter out any extraneous non-test output that might have
gotten mixed in.
+ return [l for l in lines if re.match('^\w+\.\w+$', l)]
The problem with \w is that it doesn't match -.
So I'm thinking we maybe go with something very lax like
'^[^\s.]+\.[^\s.]+$'
Since we don't have any requirements on the naming convention, I don't
know if we can be stricter.
Like, KUNIT_CASE() sorta enforces that test cases follow C identifier
naming rules, but users could always work around it fairly easily by
instantiating the struct directly.
