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

[Vladimir Sementsov-Ogievskiy]

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>
...

> >   - 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.

> > 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?

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.

> >   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?

> > @@ -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..



--
Best regards,
Vladimir