| Message ID | 20191111160216.197086-19-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix check_to_replace_node() | expand |
11.11.2019 19:02, Max Reitz wrote: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > 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'([^\[]+)\[([^\]]+)\]') > >
On 03.12.19 13:59, Vladimir Sementsov-Ogievskiy wrote: > 11.11.2019 19:02, Max Reitz wrote: >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> 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? I don’t. It just looked better to me. (One reason would be so it could be empty to refer to @root, but as I said that isn’t very useful.) > 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 '/' Hence “hypothetically”. > 2. path can be '/', which does exactly what you don't allow, and I don't see, > where it is restricted in code No, it doesn’t. That refers to a child of @root with an empty name. >> + >> + @expected_node may be None. > > Which means that, we assert existence of the path except its last element, > yes? Worth mention this behavior here. “(All elements of the path but the leaf must still exist.)”? OK. >> + >> + @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. But that makes the code uglier, in my opinion. We’d then have to set node_id to e.g. None in the except branch (or rather just abolish the try-except then) and check whether it’s None before assigning node. Like this: node_id = next(..., None) if node_id is not None: node = next(...) else: node = None I prefer the current try-except construct over that. >> + 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 Yes, but I wanted to cover all four cases explicitly. (In the usual 00, 01, 10, 11 manner. Well, except it’s 10, 01, 11, 00.) >> + 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) Works for me, too. > (I've checked: > >>> 'erger %s erg' % None > 'erger None erg' > > Also, %-style formatting is old, as I understand it's better always use .format() > ) OK. Max >> >> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') >> >>
09.12.2019 18:10, Max Reitz wrote: > On 03.12.19 13:59, Vladimir Sementsov-Ogievskiy wrote: >> 11.11.2019 19:02, Max Reitz wrote: >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> 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? > > I don’t. It just looked better to me. > > (One reason would be so it could be empty to refer to @root, but as I > said that isn’t very useful.) > >> 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 '/' > > Hence “hypothetically”. > >> 2. path can be '/', which does exactly what you don't allow, and I don't see, >> where it is restricted in code > > No, it doesn’t. That refers to a child of @root with an empty name. Hmm, yes, OK. > >>> + >>> + @expected_node may be None. >> >> Which means that, we assert existence of the path except its last element, >> yes? Worth mention this behavior here. > > “(All elements of the path but the leaf must still exist.)”? OK. OK > >>> + >>> + @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. > > But that makes the code uglier, in my opinion. We’d then have to set > node_id to e.g. None in the except branch (or rather just abolish the > try-except then) and check whether it’s None before assigning node. > Like this: > > node_id = next(..., None) > > if node_id is not None: > node = next(...) > else: > node = None > > I prefer the current try-except construct over that. OK > >>> + 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 > > Yes, but I wanted to cover all four cases explicitly. (In the usual 00, > 01, 10, 11 manner. Well, except it’s 10, 01, 11, 00.) > >>> + 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) > > Works for me, too. > >> (I've checked: >> >>> 'erger %s erg' % None >> 'erger None erg' >> >> Also, %-style formatting is old, as I understand it's better always use .format() >> ) > > OK. > > Max > >>> >>> index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') >>> >>> > >
11.11.2019 19:02, Max Reitz wrote: > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > 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. > + > + 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. > + > + @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, 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: I'd rename path_node to child or edge, to not interfere with block nodes here. > + 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) > + 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: > + assert node['name'] == expected_node, \ > + 'Found node %s under %s (but expected %s)' % \ > + (node['name'], path, expected_node) > > index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') > >
On 13.12.19 12:27, Vladimir Sementsov-Ogievskiy wrote: > 11.11.2019 19:02, Max Reitz wrote: >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> 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. >> + >> + 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. >> + >> + @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, 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: > > I'd rename path_node to child or edge, to not interfere with block nodes here. Sure. Or maybe child_name. Max
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. + + 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. + + @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, 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) + 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: + assert node['name'] == expected_node, \ + 'Found node %s under %s (but expected %s)' % \ + (node['name'], path, expected_node) index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/iotests.py | 59 +++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)