Re: [PATCH 17/22] iotests: Add VM.assert_block_path() - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Max Reitz <mreitz@redhat.com>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: Kevin Wolf <kwolf@redhat.com>, Alberto Garcia <berto@igalia.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [PATCH 17/22] iotests: Add VM.assert_block_path()
Date: Thu, 26 Sep 2019 14:07:04 +0000	[thread overview]
Message-ID: <de0a9fe2-ea84-7be9-6911-b806f032cfff@virtuozzo.com> (raw)
In-Reply-To: <20190920152804.12875-18-mreitz@redhat.com>

20.09.2019 18:27, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 48 +++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index daed4ee013..e6fb46287d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -670,6 +670,54 @@ class VM(qtest.QEMUQtestMachine):
>   
>           return fields.items() <= ret.items()
>   
> +    '''
> +    @path is a string whose components are separated by slashes.
> +    The first component is a node name, the rest are child names.
> +    Examples:
> +      - "qcow2-node/backing/file"
> +      - "quorum-node/children.2/file"

Possibly, separting node-name to first parameter and keeping child-path as
a second will simplify code a bit, and be more useful for cases when on caller
part node-name is in variable.

> +
> +    @expected_node may be None.
> +
> +    @graph may be None or the result of an x-debug-query-block-graph
> +    call that has already been performed.
> +    '''
> +    def assert_block_path(self, path, expected_node, graph=None):
> +        if graph is None:
> +            graph = self.qmp('x-debug-query-block-graph')['return']

Yay! I'm happy to see that it's useful.

> +
> +        iter_path = iter(path.split('/'))
> +        root = next(iter_path)
> +        try:
> +            node = next(node for node in graph['nodes'] if node['name'] == root)
> +        except StopIteration:
> +            node = None

for such usage next has second optional argument: next(iterator[, default])

(don't think I teach you Python, actually you teach me, as before I didn't know
correct way to search first element with condition)

> +
> +        for path_node in iter_path:
> +            assert node is not None, 'Cannot follow path %s' % path
> +
> +            try:
> +                node_id = next(edge['child'] for edge in graph['edges'] \
> +                                             if edge['parent'] == node['id'] and
> +                                                edge['name'] == path_node)

Hmm here you allow default StopIteration exception [1]

> +
> +                node = next(node for node in graph['nodes'] \
> +                                 if node['id'] == node_id)
> +            except StopIteration:
> +                node = None

actually, I think this will never happen, so we may simplify code and allow it to
throw StopIteration exception in this impossible case..

> +
> +        assert node is not None or expected_node is None, \
> +               'No node found under %s (but expected %s)' % \
> +               (path, expected_node)

node may be None here only from last iteration, but it can't happen: if we have edge
with child, we'll for sure have node with such node-name in graph

> +
> +        assert expected_node is not None or node is None, \
> +               'Found node %s under %s (but expected none)' % \
> +               (node['name'], path)

hmm, so expected_node=None means we want to prove that there is no such node? It should
be mentioned in comment above the function. But this don't work due to [1]

> +
> +        if node is not None and expected_node is not None:
> +            assert node['name'] == expected_node, \
> +                   'Found node %s under %s (but expected %s)' % \
> +                   (node['name'], path, expected_node)
>   
>   index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>   
> 

-- 
Best regards,
Vladimir