Re: [PATCH v2 2/2] iotests: rework test finding

[Max Reitz]

On 25.03.20 11:21, Vladimir Sementsov-Ogievskiy wrote:
> Add python script with new logic of searching for tests:
> 
> Old 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)
> 
> New behavior:
>  - 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
> 
> 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 and test-* notation is
>    already used in tests directory.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  docs/devel/testing.rst           |  51 +++++-
>  tests/qemu-iotests/check         |  20 +--
>  tests/qemu-iotests/find_tests.py |  72 ++++++++
>  tests/qemu-iotests/group         | 298 -------------------------------
>  4 files changed, 132 insertions(+), 309 deletions(-)
>  create mode 100755 tests/qemu-iotests/find_tests.py
>  delete mode 100644 tests/qemu-iotests/group

[...]

> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index f7a2d3d6c3..09b2ced2f0 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -168,9 +168,7 @@ do
>      if $group
>      then
>          # arg after -g
> -        group_list=$(sed -n <"$source_iotests/group" -e 's/$/ /' -e 
> "/^[0-9][0-9][0-9].* $r /"'{
> -s/ .*//p
> -}')
> +        group_list=$(./find_tests.py "$r")
>          if [ -z "$group_list" ]
>          then
>              echo "Group \"$r\" is empty or not defined?"
> @@ -193,10 +191,8 @@ s/ .*//p
>      then
>          # arg after -x
>          # Populate $tmp.list with all tests
> -        awk '/^[0-9]{3,}/ {print $1}' "${source_iotests}/group" > $tmp.list 
> 2>/dev/null
> -        group_list=$(sed -n <"$source_iotests/group" -e 's/$/ /' -e 
> "/^[0-9][0-9][0-9].* $r /"'{
> -s/ .*//p
> -}')
> +        ./find_tests.py > $tmp.list 2>/dev/null
> +        group_list=$(./find_tests.py "$r")
>          if [ -z "$group_list" ]
>          then
>              echo "Group \"$r\" is empty or not defined?"
> @@ -486,10 +482,14 @@ testlist options
>              fi
>              ;;
>  
> -        *)
> +        [0-9]*)
>              start=$r
>              end=$r
>              ;;
> +        *)

Should this be test-*, or do we really want to allow any filename here?

> +            echo $r >> $tmp.list
> +            xpand=false
> +            ;;
>  
>      esac
>  
> @@ -504,7 +504,7 @@ testlist options
>  BEGIN        { for (t='$start'; t<='$end'; t++) printf "%03d\n",t }' \
>          | while read id
>          do
> -            if grep -s "^$id\( \|\$\)" "$source_iotests/group" >/dev/null
> +            if (./find_tests.py | grep "$id")

I... Actually have no idea what this loop is supposed to do, but
couldn’t we cache the find_tests.py output?

>              then
>                  # in group file ... OK
>                  echo $id >>$tmp.list
> @@ -566,7 +566,7 @@ else
>          touch $tmp.list
>      else
>          # no test numbers, do everything from group file
> -        sed -n -e '/^[0-9][0-9][0-9]*/s/^\([0-9]*\).*/\1/p' 
> <"$source_iotests/group" >$tmp.list
> +        find_tests.py > $tmp.list
>      fi
>  fi
>  

The modifications to check seem a bit too tame to me.  Can’t we do some
more radical changes to drastically reduce the complexity of the check
script and move it to the new Python script?  Do we need the whole code
to handle a number of groups and excluded groups there?  Can’t we just
give the Python scripts a list of groups to include and a list of groups
to exclude and let it handle the rest?

> diff --git a/tests/qemu-iotests/find_tests.py 
> b/tests/qemu-iotests/find_tests.py
> new file mode 100755
> index 0000000000..5de0615ebc
> --- /dev/null
> +++ b/tests/qemu-iotests/find_tests.py
> @@ -0,0 +1,72 @@
> +#!/usr/bin/env python3
> +
> +import os
> +import glob
> +from collections import defaultdict
> +
> +
> +class TestFinder:
> +    def __init__(self):
> +        self.groups = defaultdict(set)
> +        self.all_tests = glob.glob('[0-9][0-9][0-9]')
> +
> +        self.all_tests += [f for f in glob.iglob('test-*')
> +                           if not f.endswith('.out')]
> +
> +        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, group=None):
> +        if group is None:
> +            tests = self.all_tests

Should we exclude the disabled group here?

> +        elif group not in self.groups:
> +            tests = []
> +        elif group != 'disabled' and 'disabled' in self.groups:
> +            tests = self.groups[group] - self.groups['disabled']
> +        else:
> +            tests = self.groups[group]
> +
> +        return sorted(tests)
> +
> +
> +if __name__ == '__main__':
> +    import sys
> +
> +    if len(sys.argv) > 2:
> +        print("Usage ./find_tests.py [group]")
> +        sys.exit(1)
> +
> +    tf = TestFinder()
> +    if os.path.isfile('group'):
> +        tf.add_group_file('group')

So is it “group” or ”group.local”? :)

Max

> +
> +    if len(sys.argv) == 2:
> +        tests = tf.find_tests(sys.argv[1])
> +    else:
> +        tests = tf.find_tests()
> +
> +    print('\n'.join(tests))

signature.asc
Description: OpenPGP digital signature