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

[Kevin Wolf <kwolf@redhat.com>]

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 <vsementsov@virtuozzo.com>
> > > ---
> > >   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