Re: [PATCH for-5.0 v2 18/23] iotests: Add VM.assert_block_path()

[Vladimir Sementsov-Ogievskiy]

11.11.2019 19:02, Max Reitz wrote:
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   tests/qemu-iotests/iotests.py | 59 +++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d34305ce69..3e03320ce3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -681,6 +681,65 @@ class VM(qtest.QEMUQtestMachine):
>   
>           return fields.items() <= ret.items()
>   
> +    """
> +    Check whether the node under the given path in the block graph is
> +    @expected_node.
> +
> +    @root is the node name of the node where the @path is rooted.
> +
> +    @path is a string that consists of child names separated by
> +    slashes.  It must begin with a slash.

Why do you need this slash? To stress that we are starting from root?
But root is not global, it's selected by previous argument, so for me the
path is more like relative than absolute..

> +
> +    Examples for @root + @path:
> +      - root="qcow2-node", path="/backing/file"
> +      - root="quorum-node", path="/children.2/file"
> +
> +    Hypothetically, @path could be empty, in which case it would point
> +    to @root.  However, in practice this case is not useful and hence
> +    not allowed.

1. path can't be empty, as accordingly to previous point, it must start with '/'
2. path can be '/', which does exactly what you don't allow, and I don't see,
where it is restricted in code

> +
> +    @expected_node may be None.

Which means that, we assert existence of the path except its last element,
yes? Worth mention this behavior here.

> +
> +    @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, root, path, expected_node, graph=None):
> +        if graph is None:
> +            graph = self.qmp('x-debug-query-block-graph')['return']
> +
> +        iter_path = iter(path.split('/'))
> +
> +        # Must start with a /
> +        assert next(iter_path) == ''
> +
> +        node = next((node for node in graph['nodes'] if node['name'] == 
> root),
> +                    None)
> +
> +        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)
> +
> +                node = next(node for node in graph['nodes'] \
> +                                 if node['id'] == node_id)

this line cant fail. If it fail, it means a bug in x-debug-query-block-graph, 
so,
I'd prefer to move it out of try:except block.

> +            except StopIteration:
> +                node = None
> +
> +        assert node is not None or expected_node is None, \
> +               'No node found under %s (but expected %s)' % \
> +               (path, expected_node)
> +
> +        assert expected_node is not None or node is None, \
> +               'Found node %s under %s (but expected none)' % \
> +               (node['name'], path)
> +
> +        if node is not None and expected_node is not None:

[1]
second part of condition already asserted by previous assertion

> +            assert node['name'] == expected_node, \
> +                   'Found node %s under %s (but expected %s)' % \
> +                   (node['name'], path, expected_node)

IMHO, it would be easier to read like:

           if node is None:
               assert  expected_node is None, \
                  'No node found under %s (but expected %s)' % \
                  (path, expected_node)
           else:
               assert expected_node is not None, \
                  'Found node %s under %s (but expected none)' % \
                  (node['name'], path)

               assert node['name'] == expected_node, \
                      'Found node %s under %s (but expected %s)' % \
                      (node['name'], path, expected_node)

Or even just

           if node is None:
               assert expected_node is None, \
                  'No node found under %s (but expected %s)' % \
                  (path, expected_node)
           else:
               assert node['name'] == expected_node, \
                      'Found node %s under %s (but expected %s)' % \
                      (node['name'], path, expected_node)

(I've checked:
 >>> 'erger %s erg' % None
'erger None erg'

Also, %-style formatting is old, as I understand it's better always use 
.format()
)

>   
>   index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>   
> 
-- 
Best regards,
Vladimir