Am 22.04.2020 um 07:35 hat Vladimir Sementsov-Ogievskiy geschrieben: > 21.04.2020 19:56, Kevin Wolf wrote: > > 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. > > No objections. > > I also thought about, may be, a tests/ subtree, so we'll have something like > > tests/jobs/<mirror, stream, backup tests> > tests/formats/<qcow2 tests and others, or may be one more subdirectory level> > ...
I thought of that, too, but then decided not to mention it because I feel it might be hard to decide where a test belongs. Many tests are qcow2 and mirror tests at the same time, every commit test is also a backing file test etc. This is essentially why we have groups and each test can be in more than one group. On the other hand, I assume that for some tests it would be quite clear where they belong. So if we're prepared to have some tests directly in tests/ and only some others in subdirectories, we can still do that. > > > - 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. > > Yes, I've used it for same thing. > > Actually, we still have the order, as I just sort iotests by name. I think, > we could add a parameter for testfinder (may be, as a separate step no in > these series), something like > > --start-from TEST : parse all other arguments as usual, make sorted sequence > and than 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. True, this would be a good replacement. I don't think it's a requirement to have it in this series, but I won't complain if you implement 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 <[email protected]> > > > --- > > > 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? > > Hmm. I named it by class name, considering possibility to execute is > just for module testing. So for module users, it's just a module with > class TestFinder, and it's called testfinder.. But I don't have strict > opinion in it, findtests sound more human-friendly. It was just a thought. If you prefer testfinder.py, I'm fine with it. > > > > > 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. > > OK. I'll drop img for now, documenting all the groups may be done in separate. > > > > > > .. _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? > > I added it in one-two non-obvious places. Is there any general thought > about using type-hints? Should modern coder use them absolutely > everywhere? Type checkers only work if things are consistently annotated. For example, if a function definition doesn't have type hints, the whole code in this function stays completely unchecked even if it contains an obvious error like passing a string literal to another function that is annotated to take an int. So I think we should add type hints everywhere. > > > > > @@ -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? > > Yes, thanks. I just wrote this code before learning f-strings) > > > > > > + 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. > > As I considered running this script as executable for testing purposes, it's > good to know, which options are not handled.. Yes, that makes sense. I just think it should be an error and not just an additional hint at the end. Kevin
