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

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.

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 +++++++++++++++++++++++++++++++
 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.
+
 .. _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
@@ -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",
+                                    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))
+                    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)
-- 
2.21.0

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.

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

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

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

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

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

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

Kevin

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

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

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

22.04.2020 14:53, Kevin Wolf wrote:
> 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.
> 

It's not and error, as usual case will leave some arguments for TestEnv and TestRunner.
Assume you run ./check with some arguments.. And it works bad. You assume that problem is in testfinder.py. Then, you just take the entire list of options and call ./testfinder.py with them. And it shows, how it parses its arguments, and what is reminded. Seems correct and shouldn't be an error.

Still, maybe better to print unhandled options first, to be more noticeable.


-- 
Best regards,
Vladimir

Am 22.04.2020 um 14:49 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.04.2020 14:53, Kevin Wolf wrote:
> > 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:
> > > > > +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.
> > 
> 
> It's not and error, as usual case will leave some arguments for
> TestEnv and TestRunner.  Assume you run ./check with some arguments..
>
> And it works bad. You assume that problem is in testfinder.py. Then,
> you just take the entire list of options and call ./testfinder.py with
> them. And it shows, how it parses its arguments, and what is reminded.
> Seems correct and shouldn't be an error.

Hm, I didn't consider this use case. Fair enough then.

> Still, maybe better to print unhandled options first, to be more
> noticeable.

Actually, I think it's more noticeable at the end when you execute the
script standalone, especially when it prints a long list.

Kevin

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

It will make more difficult to import iotests.py.. Calling common.rc from
bash tests will need to be modified too.

So, we'll need additional line in all python tests:

sys.path.append(os.path.join(os.path.dirname(__file__), '..'))

which doesn't seem to be good practice.. So, instead we'd better call tests with PYTHONPATH set appropriately..

and modify bash tests to do
. ../common.rc
. ../common.filter
<etc>

or again, better to export BASH_TEST_LIB directory.

Is it worth doing?

I think, there are two variants:

1) keep as is: all in one directory, add *-test notation

2) go further and restructure to something like:

iotests/<only check script and its dependencies here>
iotests/tests/<all-tests-here>
iotests/lib/python/iotests.py
iotests/lib/bash/<common.rc and friends here>

And then, check script will export PYTHONPATH and BASH_TEST_LIB variables.

In case of [2], I definitely prefer to split directory tree refactoring to the separate series. So this series just rewrite check into python and get rid of group file.

What do you prefer?

-- 
Best regards,
Vladimir

Am 07.05.2020 um 19:43 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.
> > 
> 
> It will make more difficult to import iotests.py.. Calling common.rc from
> bash tests will need to be modified too.
> 
> So, we'll need additional line in all python tests:
> 
> sys.path.append(os.path.join(os.path.dirname(__file__), '..'))

Hm, yes, this is nasty.

Would it be any better have a wrapper script that just imports the
actual test so that the import path would still contain the main
qemu-iotests/ directory?

> which doesn't seem to be good practice.. So, instead we'd better call
> tests with PYTHONPATH set appropriately..

This is another option, especially if we do want to create a lib/.

> and modify bash tests to do
> . ../common.rc
> . ../common.filter
> <etc>
> 
> or again, better to export BASH_TEST_LIB directory.

I don't think this is necessary because the working directory wouldn't
change, so bash scripts should just keep working as before.

> Is it worth doing?
> 
> I think, there are two variants:
> 
> 1) keep as is: all in one directory, add *-test notation

I think it would make it rather hard to find the files that belong to
the test harness implementation between all the tests. Currently, 'ls'
in the qemu-iotests directory is still kind of usable because all the
test cases are at the start and everything that comes later is not a
test.

> 2) go further and restructure to something like:
> 
> iotests/<only check script and its dependencies here>
> iotests/tests/<all-tests-here>
> iotests/lib/python/iotests.py
> iotests/lib/bash/<common.rc and friends here>
> 
> And then, check script will export PYTHONPATH and BASH_TEST_LIB
> variables.

I think leaving everything except named tests where it is, but setting
PYTHONPATH or having a wrapper script, is still a third option that's
worth considering. It sounds like the most attrative option to me.

Kevin

08.05.2020 11:49, Kevin Wolf wrote:
> Am 07.05.2020 um 19:43 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.
>>>
>>
>> It will make more difficult to import iotests.py.. Calling common.rc from
>> bash tests will need to be modified too.
>>
>> So, we'll need additional line in all python tests:
>>
>> sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
> 
> Hm, yes, this is nasty.
> 
> Would it be any better have a wrapper script that just imports the
> actual test so that the import path would still contain the main
> qemu-iotests/ directory?

I think, that better to keep all tests self-executable without any additional preparations, just set all interface environment variables and run test (or use check to automate environment initialization). In this way we can support any executable, not even limited to  python and bash (I doubt that we need more, but keeping test interface full-defined by environment variables seems a good thing).

> 
>> which doesn't seem to be good practice.. So, instead we'd better call
>> tests with PYTHONPATH set appropriately..
> 
> This is another option, especially if we do want to create a lib/.
> 
>> and modify bash tests to do
>> . ../common.rc
>> . ../common.filter
>> <etc>
>>
>> or again, better to export BASH_TEST_LIB directory.
> 
> I don't think this is necessary because the working directory wouldn't
> change, so bash scripts should just keep working as before.

Hmm, right. Probably same works for python tests as well?

> 
>> Is it worth doing?
>>
>> I think, there are two variants:
>>
>> 1) keep as is: all in one directory, add *-test notation
> 
> I think it would make it rather hard to find the files that belong to
> the test harness implementation between all the tests. Currently, 'ls'
> in the qemu-iotests directory is still kind of usable because all the
> test cases are at the start and everything that comes later is not a
> test.
> 
>> 2) go further and restructure to something like:
>>
>> iotests/<only check script and its dependencies here>
>> iotests/tests/<all-tests-here>
>> iotests/lib/python/iotests.py
>> iotests/lib/bash/<common.rc and friends here>
>>
>> And then, check script will export PYTHONPATH and BASH_TEST_LIB
>> variables.
> 
> I think leaving everything except named tests where it is, but setting
> PYTHONPATH or having a wrapper script, is still a third option that's
> worth considering. It sounds like the most attrative option to me.
> 

Works for me too. OK, let's go this way.

-- 
Best regards,
Vladimir

On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote:
> 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.
>>
> 
> It will make more difficult to import iotests.py.. Calling common.rc from
> bash tests will need to be modified too.
> 
> So, we'll need additional line in all python tests:
> 
> sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
> 
> which doesn't seem to be good practice.. So, instead we'd better call
> tests with PYTHONPATH set appropriately..
> 

Just chiming in to say that it's largely bad practice because it
confuses pylint, mypy and friends -- if we want to keep pushing our CI
code analysis for python in that direction, this will be a barrier.

Using PYTHONPATH is better, because it isolates the script itself from
the environment, but requires you to now always set PYTHONPATH to
execute any of the individual iotests.

Not actually a big deal, because iotests already expect a large number
of environment variables to be set. It's not really a huge net loss in
convenience, I think.

looks like that's the direction you're headed in anyway based on
discussion, so that's good.

--js

14.05.2020 00:58, John Snow wrote:
> 
> 
> On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
>>>
>>
>> It will make more difficult to import iotests.py.. Calling common.rc from
>> bash tests will need to be modified too.
>>
>> So, we'll need additional line in all python tests:
>>
>> sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
>>
>> which doesn't seem to be good practice.. So, instead we'd better call
>> tests with PYTHONPATH set appropriately..
>>
> 
> Just chiming in to say that it's largely bad practice because it
> confuses pylint, mypy and friends -- if we want to keep pushing our CI
> code analysis for python in that direction, this will be a barrier.
> 
> Using PYTHONPATH is better, because it isolates the script itself from
> the environment, but requires you to now always set PYTHONPATH to
> execute any of the individual iotests.
> 
> Not actually a big deal, because iotests already expect a large number
> of environment variables to be set. It's not really a huge net loss in
> convenience, I think.
> 
> looks like that's the direction you're headed in anyway based on
> discussion, so that's good.
> 

Hm, does PYTHONPATH-way works good with mypy and friends? Probably, it should
be set when checking the code? So, actually developers will have to set
PYTHONPATH by hand to always contain some directories within qemu source tree?

-- 
Best regards,
Vladimir

On 5/14/20 12:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2020 00:58, John Snow wrote:
>>
>>
>> On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 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.
>>>>
>>>
>>> It will make more difficult to import iotests.py.. Calling common.rc
>>> from
>>> bash tests will need to be modified too.
>>>
>>> So, we'll need additional line in all python tests:
>>>
>>> sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
>>>
>>> which doesn't seem to be good practice.. So, instead we'd better call
>>> tests with PYTHONPATH set appropriately..
>>>
>>
>> Just chiming in to say that it's largely bad practice because it
>> confuses pylint, mypy and friends -- if we want to keep pushing our CI
>> code analysis for python in that direction, this will be a barrier.
>>
>> Using PYTHONPATH is better, because it isolates the script itself from
>> the environment, but requires you to now always set PYTHONPATH to
>> execute any of the individual iotests.
>>
>> Not actually a big deal, because iotests already expect a large number
>> of environment variables to be set. It's not really a huge net loss in
>> convenience, I think.
>>
>> looks like that's the direction you're headed in anyway based on
>> discussion, so that's good.
>>
> 
> Hm, does PYTHONPATH-way works good with mypy and friends? Probably, it
> should
> be set when checking the code? So, actually developers will have to set
> PYTHONPATH by hand to always contain some directories within qemu source
> tree?
> 

pylint respects PYTHONPATH but mypy doesn't. mypy uses MYPYPATH, but I
wouldn't worry about accommodating it. It's a fussy tool and we're only
ever going to run it from very specific environments.

You don't need to worry too much about what environment variables these
tools take; it's only worth noting that "sys.path" hacks tend to make
these tools harder to use.


As for setting PYTHONPATH by hand ... There are a few places in the QEMU
tree where we set PYTHONPATH already, and the individual iotests already
don't work if they're not launched by `check`, because they're missing a
ton of environment variables.

It's not going to be too bad to set PYTHONPATH in the launcher script,
is it?

(Or are we replacing the top-level script with a python one?)




Really, the same is true of pylint, too. It's only annoying to deal with
sys.path hacking because it can't be worked around in those CQA tools.

14.05.2020 08:06, John Snow wrote:
> 
> 
> On 5/14/20 12:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 14.05.2020 00:58, John Snow wrote:
>>>
>>>
>>> On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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.
>>>>>
>>>>
>>>> It will make more difficult to import iotests.py.. Calling common.rc
>>>> from
>>>> bash tests will need to be modified too.
>>>>
>>>> So, we'll need additional line in all python tests:
>>>>
>>>> sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
>>>>
>>>> which doesn't seem to be good practice.. So, instead we'd better call
>>>> tests with PYTHONPATH set appropriately..
>>>>
>>>
>>> Just chiming in to say that it's largely bad practice because it
>>> confuses pylint, mypy and friends -- if we want to keep pushing our CI
>>> code analysis for python in that direction, this will be a barrier.
>>>
>>> Using PYTHONPATH is better, because it isolates the script itself from
>>> the environment, but requires you to now always set PYTHONPATH to
>>> execute any of the individual iotests.
>>>
>>> Not actually a big deal, because iotests already expect a large number
>>> of environment variables to be set. It's not really a huge net loss in
>>> convenience, I think.
>>>
>>> looks like that's the direction you're headed in anyway based on
>>> discussion, so that's good.
>>>
>>
>> Hm, does PYTHONPATH-way works good with mypy and friends? Probably, it
>> should
>> be set when checking the code? So, actually developers will have to set
>> PYTHONPATH by hand to always contain some directories within qemu source
>> tree?
>>
> 
> pylint respects PYTHONPATH but mypy doesn't. mypy uses MYPYPATH, but I
> wouldn't worry about accommodating it. It's a fussy tool and we're only
> ever going to run it from very specific environments.
> 

Hmm, recently I installed dense-analysis/ale plugin into my vim which does mypy checking (among other things).. And most probably, I'll have to set these variables to keep it working. But it's not a big problem.

> You don't need to worry too much about what environment variables these
> tools take; it's only worth noting that "sys.path" hacks tend to make
> these tools harder to use.
> 
> 
> As for setting PYTHONPATH by hand ... There are a few places in the QEMU
> tree where we set PYTHONPATH already, and the individual iotests already
> don't work if they're not launched by `check`, because they're missing a
> ton of environment variables.
> 
> It's not going to be too bad to set PYTHONPATH in the launcher script,
> is it?
> 
> (Or are we replacing the top-level script with a python one?)

Yes we do, bright future is near:) But it's not a problem to set PYTHONPATH in it. Anyway, we run all tests as executables, so passing PYTHONPATH is a valid thing to do.

> 
> 
> 
> 
> Really, the same is true of pylint, too. It's only annoying to deal with
> sys.path hacking because it can't be worked around in those CQA tools.
> 


-- 
Best regards,
Vladimir