Re: [PATCH v6 07/11] iotests: add findtests.py

[Kevin Wolf]

Am 09.01.2021 um 13:26 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 findtests.py:
>  - group file is dropped
>  - tests are all files in tests/ subdirectory (except for .out files),
>    so it's not needed more to "register the test", just create it with
>    appropriate name in tests/ subdirectory. Old names like
>    [0-9][0-9][0-9] (in root iotests directory) are supported too, but
>    not recommended for new tests
>  - 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
>    (to support restarting failed ./check command from the middle of the
>     process, new argument is added: --start-from)
> 
> 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 don't update check behavior (which will be don in further
> commit), 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.
> 
> The file findtests.py is self-executable and may be used for debugging
> purposes.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/devel/testing.rst          |  50 ++++++-
>  tests/qemu-iotests/findtests.py | 229 ++++++++++++++++++++++++++++++++

I extended 297 so that it also checks the newly added Python file, and
pylint and mypy report some errors:

+************* Module findtests
+findtests.py:127:19: W0621: Redefining name 'tests' from outer scope (line 
226) (redefined-outer-name)
+findtests.py:224:8: R1722: Consider using sys.exit() (consider-using-sys-exit)
+findtests.py:32: error: Missing type parameters for generic type "Iterator"
+findtests.py:87: error: Function is missing a return type annotation
+findtests.py:206: error: Function is missing a type annotation for one or more 
arguments

I would suggest including a final patch in this series that adds all new
Python files to the checking in 297.

> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 0aa7a13bba..bfb6b65edc 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -111,7 +111,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
> @@ -224,6 +224,54 @@ 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.

Maybe: "Tests may belong to one or more test groups, which are defined
in the form of a comment in the test source file."

>         By convention, test groups are listed in the second line of the test
> +file, after "#!/..." line, like this:

"after the "#!/..." line"

> +
> +.. code::
> +
> +  #!/usr/bin/env python3
> +  # group: auto quick
> +  #
> +  ...
> +
> +Additional way of defining groups is creating tests/qemu-iotests/group.local

"An additional" or "Another".
"creating the ... file"

> +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:

"disabling"

> +
> +.. 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 (not exhaustive) list of groups:

Maybe just something like this?

"Note that the following group names have a special meaning:"

> +
> +- quick : Tests in this group should finish within some few seconds.
> +
> +- 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.
> +
>  .. _docker-ref:
>  
>  Docker based tests
> diff --git a/tests/qemu-iotests/findtests.py b/tests/qemu-iotests/findtests.py
> new file mode 100755
> index 0000000000..b053db48e8
> --- /dev/null
> +++ b/tests/qemu-iotests/findtests.py
> @@ -0,0 +1,229 @@
> +#!/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
> +from typing import Optional, List, Tuple, Iterator, Set
> +
> +
> +@contextmanager
> +def chdir(path: Optional[str] = None) -> Iterator:

As indicated by mypy, this should be Interator[None].

> +    if path is None:
> +        yield
> +        return
> +
> +    saved_dir = os.getcwd()
> +    os.chdir(path)
> +    try:
> +        yield
> +    finally:
> +        os.chdir(saved_dir)
> +
> +
> +class TestFinder:
> +    _argparser = None
> +    @classmethod
> +    def get_argparser(cls) -> argparse.ArgumentParser:
> +        if cls._argparser is not None:
> +            return cls._argparser
> +
> +        p = argparse.ArgumentParser(description="= test selecting options =",
> +                                    add_help=False, usage=argparse.SUPPRESS)
> +
> +        p.add_argument('-g', '--groups', metavar='group1,...',
> +                       help='include tests from these groups')
> +        p.add_argument('-x', '--exclude-groups', metavar='group1,...',
> +                       help='exclude tests from these groups')
> +        p.add_argument('--start-from', metavar='TEST',
> +                       help='Start from specified test: make sorted sequence 
> '
> +                       'of tests as usual and then drop tests from the first 
> '
> +                       'one to TEST (not inclusive). This may be used to '
> +                       'rerun failed ./check command, starting from the '
> +                       'middle of the process.')
> +        p.add_argument('tests', metavar='TEST_FILES', nargs='*',
> +                       help='tests to run')
> +
> +        cls._argparser = p
> +        return p
> +
> +    def __init__(self, test_dir: Optional[str] = None) -> 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('tests/*')
> +                               if not f.endswith('.out') and
> +                               os.path.isfile(f + '.out')]
> +
> +            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)

Do we need to allow more than one group comment in a test or could we
return early here, avoiding to read the whole file?

> +
> +    def add_group_file(self, fname: str):
> +        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(f'Warning: {fname}: "{test_file}" test is not 
> found.'
> +                          ' Skip.')
> +                    continue

Should test_file be passed through parse_test_name()? I noticed that I
have to explicitly specify a tests/ prefix in group.local, whereas
prefixing this on the command line seems to be forbidden.

> +                for g in groups:
> +                    self.groups[g].add(test_file)
> +
> +    def parse_test_name(self, name: str) -> str:
> +        if '/' in name:
> +            raise ValueError('Paths are unsupported for test selecting, '
> +                             f'requiring "{name}" is wrong')
> +
> +        if re.fullmatch(r'\d+', name):
> +            # Numbered tests are old naming convetion. We should convert them
> +            # to three-digit-length, like 1 --> 001.
> +            name = f'{int(name):03}'
> +        else:
> +            # Named tests all should be in tests/ subdirectory
> +            name = os.path.join('tests', name)
> +
> +        if name not in self.all_tests:
> +            raise ValueError(f'Test "{name}" is not found')
> +
> +        return name

check should probably catch these ValueError exceptions. Currently, you
get a stack trace, which does include the exception message, but it
doesn't look very nice.

> +    def find_tests(self, groups: Optional[List[str]] = None,
> +                   exclude_groups: Optional[List[str]] = None,
> +                   tests: Optional[List[str]] = None,

group and tests seem to be read-only, so this can be simplified to
'groups: Sequence[str] = ()' etc. without the explicit handling of None
below.

Maybe exclude_groups should then be treated the same for consistency.
This would mean creating a new list instead of calling .append().

> +                   start_from: Optional[str] = None) -> List[str]:
> +        """Find tests
> +
> +        Algorithm:
> +
> +        1. a. if some @groups specified
> +             a.1 Take all tests from @groups
> +             a.2 Drop tests, which are in at least one of @exclude_groups or 
> in
> +                 'disabled' group (if 'disabled' is not listed in @groups)
> +             a.3 Add tests from @tests (don't exclude anything from them)
> +
> +           b. else, if some @tests specified:
> +             b.1 exclude_groups must be not specified, so just take @tests
> +
> +           c. else (only @exclude_groups list is non-empty):
> +             c.1 Take all tests
> +             c.2 Drop tests, which are in at least one of @exclude_groups or 
> in
> +                 'disabled' group
> +
> +        2. sort
> +
> +        3. If start_from specified, drop tests from first one to @start_from
> +           (not inclusive)
> +        """
> +        if groups is None:
> +            groups = []
> +        if exclude_groups is None:
> +            exclude_groups = []
> +        if tests is None:
> +            tests = []
> +
> +        res: Set[str] = set()
> +        if groups:
> +            # Some groups specified. exclude_groups supported, additionally
> +            # selecting some individual tests supported as well.
> +            res.update(*(self.groups[g] for g in groups))
> +        elif tests:
> +            # Some individual tests specified, but no groups. In this case
> +            # we don't support exclude_groups.
> +            if exclude_groups:
> +                raise ValueError("Can't exclude from individually specified "
> +                                 "tests.")
> +        else:
> +            # No tests no groups: start from all tests, exclude_groups
> +            # supported.
> +            res.update(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:
> +            res.add(self.parse_test_name(t))
> +
> +        sequence = sorted(res)
> +
> +        if start_from is not None:
> +            del sequence[:sequence.index(self.parse_test_name(start_from))]
> +
> +        return sequence
> +
> +    def find_tests_argv(self, argv: List[str]) -> Tuple[List[str], 
> List[str]]:
> +        """Returns tuple of tests list and remaining arguments list"""
> +        args, remaining = self.get_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: Optional[str] = None) -> Tuple[List[str],
> +                                                              List[str]]:
> +    """Returns tuple of tests list and remaining arguments list"""
> +    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.get_argparser().print_help()
> +        exit()
> +
> +    tests, remaining_argv = find_tests(sys.argv[1:])
> +    print('\n'.join(tests))
> +    if remaining_argv:
> +        print('\nUnhandled options: ', remaining_argv)

Kevin