Re: [PATCH v7 07/11] iotests: add findtests.py

From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, den@openvz.org, jsnow@redhat.com,
	qemu-devel@nongnu.org, mreitz@redhat.com
Subject: Re: [PATCH v7 07/11] iotests: add findtests.py
Date: Thu, 21 Jan 2021 10:18:12 -0600	[thread overview]
Message-ID: <3c0222d6-f8b2-be1b-efec-399ed8c0483a@redhat.com> (raw)
In-Reply-To: <20210116134424.82867-8-vsementsov@virtuozzo.com>

On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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 findtests.py:
>  - group file is dropped
>  - tests are all files in tests/ subdirectory (except for .out files),
>    so it's not needed more to "register the test", just create it with
>    appropriate name in tests/ subdirectory. Old names like
>    [0-9][0-9][0-9] (in root iotests directory) 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
>    (to support restarting failed ./check command from the middle of the
>     process, new argument is added: --start-from)
> 
> 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 don't update check behavior (which will be done in further

doesn't

> commit), 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.
> 
> The file findtests.py is self-executable and may be used for debugging
> purposes.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/devel/testing.rst          |  50 +++++++++-
>  tests/qemu-iotests/findtests.py | 159 ++++++++++++++++++++++++++++++++
>  2 files changed, 208 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemu-iotests/findtests.py
> 

> +++ b/tests/qemu-iotests/findtests.py

> +class TestFinder:
> +    def __init__(self, test_dir: Optional[str] = None) -> 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('tests/*')
> +                               if not f.endswith('.out') and
> +                               os.path.isfile(f + '.out')]

Interesting that 'NNN' is a test even if 'NNN.out' is not present, but
'tests/NNN' is not.  Not sure if it is worth tweaking, though.

> +    def parse_test_name(self, name: str) -> str:
> +        if '/' in name:
> +            raise ValueError('Paths are unsupported for test selecting, '

selection

> +                             f'requiring "{name}" is wrong')
> +
> +        if re.fullmatch(r'\d+', name):
> +            # Numbered tests are old naming convetion. We should convert them

convention

> +            # to three-digit-length, like 1 --> 001.
> +            name = f'{int(name):03}'
> +        else:
> +            # Named tests all should be in tests/ subdirectory
> +            name = os.path.join('tests', name)
> +
> +        if name not in self.all_tests:
> +            raise ValueError(f'Test "{name}" is not found')
> +
> +        return name
> +
> +    def find_tests(self, groups: Optional[List[str]] = None,
> +                   exclude_groups: Optional[List[str]] = None,
> +                   tests: Optional[List[str]] = None,
> +                   start_from: Optional[str] = None) -> List[str]:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org