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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

12.01.2021 19:42, Kevin Wolf wrote:
> 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?

Hmm, if we defined it's as a "convention", I think we can just check only the second line of the 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.

agree.

> 
>> +                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().

will try.


thanks, I'll take all the suggestions.


-- 
Best regards,
Vladimir