Re: [PATCH 17/22] iotests: Add VM.assert_block_path()

[Vladimir Sementsov-Ogievskiy]

02.10.2019 15:40, Max Reitz wrote:
> On 26.09.19 16:07, Vladimir Sementsov-Ogievskiy wrote:
>> 20.09.2019 18:27, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <address@hidden>
>>> ---
>>>    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.
> 
> Sounds good.
> 
>>> +
>>> +    @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.
> 
> :-)
> 
> It’s probably the best query function we have.
> 
>>> +
>>> +        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])
> 
> Great!
> 
>> (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)
> 
> We learn from one another, which is the best case.
> 
>>> +
>>> +        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]

brr, I just mistaken here: we are in same try-except as the following line, and 
we'll
catch it of course

>>
>>
>>> +
>>> +                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..
> 
> This is for a use case where the next child simply doesn’t exist, so you
> can do:
> 
> assert_block_path('qcow2-node/backing', None)
> 
> To verify that the qcow2 node has no backing file.
> 
>>> +
>>> +        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
> 
> node will always be set by the try-except block, won’t it?
> 
>>> +
>>> +        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]
> 
> Hm, I seem to remember I tested all cases locally and they all worked.
> 
> Max
> 
>>> +
>>> +        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