| Message ID | 20210109122631.167314-8-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework iotests/check | expand |
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
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.
Am 13.01.2021 um 11:37 hat Vladimir Sementsov-Ogievskiy geschrieben: > 12.01.2021 19:42, Kevin Wolf wrote: > > Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > + 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. I was actually just thinking of a break after finding the "# group: " line. It would still be recognised as the third line, tenth line or anywhere else, but only one line in the file would be recognised. But if you think only looking at the second line is even easier, we can do that. However, then I wouldn't call it "as a convention" any more in the documentation because then it wouldn't work any other way any more. Kevin
On 1/9/21 6:26 AM, Vladimir Sementsov-Ogievskiy wrote: > 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) > > > This commit don't update check behavior (which will be don in further done > 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 ++++++++++++++++++++++++++++++++ > 2 files changed, 278 insertions(+), 1 deletion(-) > create mode 100755 tests/qemu-iotests/findtests.py In addition to Kevin's review, > +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: Kevin suggested s/disable/disabling/; I'd go one step further: /temporary disable/temporarily disabling/ > + > +- quick : Tests in this group should finish within some few seconds. s/some/a/ Also, for each group name, the space before ':' looks odd. > +++ 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 Worth claiming 2021 as well? Otherwise, my python review is weak, but I didn't spot anything blatantly wrong.
12.01.2021 19:42, Kevin Wolf wrote: >> + 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. None comes when cmdline argument is not set, will sequence as you propose I get Traceback (most recent call last): File "/work/src/qemu/up-refactor-iotests/build/tests/qemu-iotests/./check", line 37, in <module> tests, remaining_argv = find_tests(env.remaining_argv, File "/work/src/qemu/up-refactor-iotests/tests/qemu-iotests/findtests.py", line 208, in find_tests return tf.find_tests_argv(argv) File "/work/src/qemu/up-refactor-iotests/tests/qemu-iotests/findtests.py", line 193, in find_tests_argv return self.find_tests(**vars(args)), remaining File "/work/src/qemu/up-refactor-iotests/tests/qemu-iotests/findtests.py", line 165, in find_tests if 'disabled' not in groups and 'disabled' not in exclude_groups: TypeError: argument of type 'NoneType' is not iterable So, seems simpler to keep all as is than modifying find_tests_argv to not pass None arguments.
Am 14.01.2021 um 08:38 hat Vladimir Sementsov-Ogievskiy geschrieben: > 12.01.2021 19:42, Kevin Wolf wrote: > > > + 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. > > None comes when cmdline argument is not set, will sequence as you propose I get > > Traceback (most recent call last): > File "/work/src/qemu/up-refactor-iotests/build/tests/qemu-iotests/./check", line 37, in <module> > tests, remaining_argv = find_tests(env.remaining_argv, > File "/work/src/qemu/up-refactor-iotests/tests/qemu-iotests/findtests.py", line 208, in find_tests > return tf.find_tests_argv(argv) > File "/work/src/qemu/up-refactor-iotests/tests/qemu-iotests/findtests.py", line 193, in find_tests_argv > return self.find_tests(**vars(args)), remaining > File "/work/src/qemu/up-refactor-iotests/tests/qemu-iotests/findtests.py", line 165, in find_tests > if 'disabled' not in groups and 'disabled' not in exclude_groups: > TypeError: argument of type 'NoneType' is not iterable > > So, seems simpler to keep all as is than modifying find_tests_argv to not pass None arguments. Ah, I wasn't aware that you would get explicit None. Fair enough. Kevin
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. 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 (not exhaustive) list of groups: + +- 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: + 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) + + 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 + + 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 + + def find_tests(self, groups: Optional[List[str]] = None, + exclude_groups: Optional[List[str]] = None, + tests: Optional[List[str]] = None, + 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)
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 ++++++++++++++++++++++++++++++++ 2 files changed, 278 insertions(+), 1 deletion(-) create mode 100755 tests/qemu-iotests/findtests.py