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