Re: [PATCH v3 4/4] kunit: tool: support running each suite/test separately

[Daniel Latypov <dlatypov@google.com>]

On Wed, Sep 29, 2021 at 7:27 PM David Gow <davidgow@google.com> wrote:
>
> On Thu, Sep 30, 2021 at 3:54 AM Daniel Latypov <dlatypov@google.com> 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@google.com>
> > Reviewed-by: David Gow <davidgow@google.com>
> > ---
>
> 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.