Re: [PATCH v3 06/10] iotests: add testfinder.py

[Kevin Wolf]

Am 21.04.2020 um 09:35 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add python script with new logic of searching for tests:
> 
> Current ./check behavior:
>  - tests are named [0-9][0-9][0-9]
>  - tests must be registered in group file (even if test doesn't belong
>    to any group, like 142)
> 
> Behavior of new test:
>  - group file is dropped
>  - tests are searched by file-name instead of group file, so it's not
>    needed more to "register the test", just create it with name
>    *-test. Old names like [0-9][0-9][0-9] are supported too, but not
>    recommended for new tests

I wonder if a tests/ subdirectory instead of the -test suffix would
organise things a bit better.

>  - groups are parsed from '# group: ' line inside test files
>  - optional file group.local may be used to define some additional
>    groups for downstreams
>  - 'disabled' group is used to temporary disable tests. So instead of
>    commenting tests in old 'group' file you now can add them to
>    disabled group with help of 'group.local' file
>  - selecting test ranges like 5-15 are not supported more

Occasionally they were useful when something went wrong during the test
run and I only wanted to repeat the part after it happened. But it's a
rare case and we don't have a clear order any more with arbitrary test
names (which are an improvement otherwise), so we'll live with it.

> Benefits:
>  - no rebase conflicts in group file on patch porting from branch to
>    branch
>  - no conflicts in upstream, when different series want to occupy same
>    test number
>  - meaningful names for test files
>    For example, with digital number, when some person wants to add some
>    test about block-stream, he most probably will just create a new
>    test. But if there would be test-block-stream test already, he will
>    at first look at it and may be just add a test-case into it.
>    And anyway meaningful names are better.
> 
> This commit just adds class, which is unused now, and will be used in
> further patches, to finally substitute ./check selecting tests logic.

Maybe mention here that the file can be executed standalone, even if
it'S not used by check yet.

> Still, the documentation changed like new behavior is already here.
> Let's live with this small inconsistency for the following few commits,
> until final change.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  docs/devel/testing.rst           |  52 +++++++++-
>  tests/qemu-iotests/testfinder.py | 167 +++++++++++++++++++++++++++++++

A little bit of bikeshedding: As this can be executed as a standalone
tool, would a name like findtests.py be better?

>  2 files changed, 218 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/testfinder.py
> 
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 770a987ea4..6c9d5b126b 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -153,7 +153,7 @@ check-block
>  -----------
>  
>  ``make check-block`` runs a subset of the block layer iotests (the tests that
> -are in the "auto" group in ``tests/qemu-iotests/group``).
> +are in the "auto" group).
>  See the "QEMU iotests" section below for more information.
>  
>  GCC gcov support
> @@ -267,6 +267,56 @@ another application on the host may have locked the 
> file, possibly leading to a
>  test failure.  If using such devices are explicitly desired, consider adding
>  ``locking=off`` option to disable image locking.
>  
> +Test case groups
> +----------------
> +
> +Test may belong to some groups, you may define it in the comment inside the
> +test. By convention, test groups are listed in the second line of the test
> +file, after "#!/..." line, like this:
> +
> +.. code::
> +
> +  #!/usr/bin/env python3
> +  # group: auto quick
> +  #
> +  ...
> +
> +Additional way of defining groups is creating tests/qemu-iotests/group.local
> +file. This should be used only for downstream (this file should never appear
> +in upstream). This file may be used for defining some downstream test groups
> +or for temporary disable tests, like this:
> +
> +.. code::
> +
> +  # groups for some company downstream process
> +  #
> +  # ci - tests to run on build
> +  # down - our downstream tests, not for upstream
> +  #
> +  # Format of each line is:
> +  # TEST_NAME TEST_GROUP [TEST_GROUP ]...
> +
> +  013 ci
> +  210 disabled
> +  215 disabled
> +  our-ugly-workaround-test down ci
> +
> +The following groups are defined:
> +
> +- quick : Tests in this group should finish within some few seconds.
> +
> +- img : Tests in this group can be used to excercise the qemu-img tool.
> +
> +- auto : Tests in this group are used during "make check" and should be
> +  runnable in any case. That means they should run with every QEMU binary
> +  (also non-x86), with every QEMU configuration (i.e. must not fail if
> +  an optional feature is not compiled in - but reporting a "skip" is ok),
> +  work at least with the qcow2 file format, work with all kind of host
> +  filesystems and users (e.g. "nobody" or "root") and must not take too
> +  much memory and disk space (since CI pipelines tend to fail otherwise).
> +
> +- disabled : Tests in this group are disabled and ignored by check.

We have more groups than just these. We could either try and document
all of them here, or we could only keep auto/quick/disabled which have a
general meaning rather than defining an area of code that they test. If
we do the latter, I would clarify that this is not an exhaustive list,
and remove "img" from this section.

>  .. _docker-ref:
>  
>  Docker based tests
> diff --git a/tests/qemu-iotests/testfinder.py 
> b/tests/qemu-iotests/testfinder.py
> new file mode 100755
> index 0000000000..1da28564c0
> --- /dev/null
> +++ b/tests/qemu-iotests/testfinder.py

As this is a new code file, would you mind adding type hints from the
start?

> @@ -0,0 +1,167 @@
> +#!/usr/bin/env python3
> +#
> +# Parse command line options to define set of tests to run.
> +#
> +# Copyright (c) 2020 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import sys
> +import glob
> +import argparse
> +import re
> +from collections import defaultdict
> +from contextlib import contextmanager
> +
> +
> +@contextmanager
> +def chdir(path):
> +    if path is None:
> +        yield
> +        return
> +
> +    saved_dir = os.getcwd()
> +    os.chdir(path)
> +    try:
> +        yield
> +    finally:
> +        os.chdir(saved_dir)
> +
> +
> +class TestFinder:
> +    @staticmethod
> +    def create_argparser():
> +        p = argparse.ArgumentParser(description="Select set of tests",

"a set of tests"?

> +                                    add_help=False, usage=argparse.SUPPRESS)
> +
> +        p.add_argument('-g', metavar='group1,...', dest='groups',
> +                       help='include tests from these groups')
> +        p.add_argument('-x', metavar='group1,...', dest='exclude_groups',
> +                       help='exclude tests from these groups')
> +        p.add_argument('tests', metavar='TEST_FILES', nargs='*',
> +                       help='tests to run')
> +
> +        return p
> +
> +    argparser = create_argparser.__func__()
> +
> +    def __init__(self, test_dir=None):
> +        self.groups = defaultdict(set)
> +
> +        with chdir(test_dir):
> +            self.all_tests = glob.glob('[0-9][0-9][0-9]')
> +            self.all_tests += [f for f in glob.iglob('*-test')]
> +
> +            for t in self.all_tests:
> +                with open(t) as f:
> +                    for line in f:
> +                        if line.startswith('# group: '):
> +                            for g in line.split()[2:]:
> +                                self.groups[g].add(t)
> +
> +    def add_group_file(self, fname):
> +        with open(fname) as f:
> +            for line in f:
> +                line = line.strip()
> +
> +                if (not line) or line[0] == '#':
> +                    continue
> +
> +                words = line.split()
> +                test_file = words[0]
> +                groups = words[1:]
> +
> +                if test_file not in self.all_tests:
> +                    print('Warning: {}: "{}" test is not found. '
> +                          'Skip.'.format(fname, test_file))

You're using f strings in the later patches. Wouldn't it be more
consistent to use them here, too?

> +                    continue
> +
> +                for g in groups:
> +                    self.groups[g].add(test_file)
> +
> +    def find_tests(self, groups=None, exclude_groups=None, tests=None):
> +        """
> +        1. Take all tests from @groups,
> +           or just all tests if @groups is None and @tests is None
> +           or nothing if @groups is None and @tests is not None
> +        2. Drop tests, which are in at least one of @exclude_groups or in
> +           'disabled' group (if 'disabled' is not listed in @groups)
> +        3. Add tests from @tests
> +        """
> +        if groups is None:
> +            groups = []
> +        if exclude_groups is None:
> +            exclude_groups = []
> +        if tests is None:
> +            tests = []
> +
> +        if groups:
> +            res = set().union(*(self.groups[g] for g in groups))
> +        elif tests:
> +            res = set()
> +        else:
> +            res = set(self.all_tests)
> +
> +        if 'disabled' not in groups and 'disabled' not in exclude_groups:
> +            exclude_groups.append('disabled')
> +
> +        res = res.difference(*(self.groups[g] for g in exclude_groups))
> +
> +        # We want to add @tests. But for compatibility with old test names,
> +        # we should convert any number < 100 to number padded by
> +        # leading zeroes, like 1 -> 001 and 23 -> 023.
> +        for t in tests:
> +            if re.fullmatch(r'\d{1,2}', t):
> +                res.add('0' * (3 - len(t)) + t)
> +            else:
> +                res.add(t)
> +
> +        return sorted(res)
> +
> +    def find_tests_argv(self, argv):
> +        args, remaining = self.argparser.parse_known_args(argv)
> +
> +        if args.groups is not None:
> +            args.groups = args.groups.split(',')
> +
> +        if args.exclude_groups is not None:
> +            args.exclude_groups = args.exclude_groups.split(',')
> +
> +        return self.find_tests(**vars(args)), remaining
> +
> +
> +def find_tests(argv, test_dir=None):
> +    tf = TestFinder(test_dir)
> +
> +    if test_dir is None:
> +        group_local = 'group.local'
> +    else:
> +        group_local = os.path.join(test_dir, 'group.local')
> +    if os.path.isfile(group_local):
> +        tf.add_group_file(group_local)
> +
> +    return tf.find_tests_argv(argv)
> +
> +
> +if __name__ == '__main__':
> +    if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']:
> +        TestFinder.argparser.print_help()
> +        exit()
> +
> +    tests, remaining_argv = find_tests(sys.argv[1:])
> +    print('\n'.join(tests))
> +    if remaining_argv:
> +        print('\nUnhandled options: ', remaining_argv)

I think unhandled options shouldn't be considered an error and we
shouldn't even try to find and display any tests then. I'd either print
the help text or have an error message that refers to --help.

Kevin