diff mbox series

[13/15] iotests: Accommodate async QMP Exception classes

Message ID 20210917054047.2042843-14-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series Switch iotests to using Async QMP | expand

Commit Message

John Snow Sept. 17, 2021, 5:40 a.m. UTC
(But continue to support the old ones for now, too.)

There are very few cases of any user of QEMUMachine or a subclass
thereof relying on a QMP Exception type. If you'd like to check for
yourself, you want to grep for all of the derivatives of QMPError,
excluding 'AQMPError' and its derivatives. That'd be these:

- QMPError
- QMPConnectError
- QMPCapabilitiesError
- QMPTimeoutError
- QMPProtocolError
- QMPResponseError
- QMPBadPortError


Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/simplebench/bench_block_job.py    | 3 ++-
 tests/qemu-iotests/tests/mirror-top-perms | 6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Hanna Reitz Sept. 17, 2021, 2:35 p.m. UTC | #1
On 17.09.21 07:40, John Snow wrote:
> (But continue to support the old ones for now, too.)
>
> There are very few cases of any user of QEMUMachine or a subclass
> thereof relying on a QMP Exception type. If you'd like to check for
> yourself, you want to grep for all of the derivatives of QMPError,
> excluding 'AQMPError' and its derivatives. That'd be these:
>
> - QMPError
> - QMPConnectError
> - QMPCapabilitiesError
> - QMPTimeoutError
> - QMPProtocolError
> - QMPResponseError
> - QMPBadPortError
>
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   scripts/simplebench/bench_block_job.py    | 3 ++-
>   tests/qemu-iotests/tests/mirror-top-perms | 6 +++++-
>   2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
> index 4f03c12169..a403c35b08 100755
> --- a/scripts/simplebench/bench_block_job.py
> +++ b/scripts/simplebench/bench_block_job.py
> @@ -28,6 +28,7 @@
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>   from qemu.machine import QEMUMachine
>   from qemu.qmp import QMPConnectError
> +from qemu.aqmp import ConnectError
>   
>   
>   def bench_block_job(cmd, cmd_args, qemu_args):
> @@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
>           vm.launch()
>       except OSError as e:
>           return {'error': 'popen failed: ' + str(e)}
> -    except (QMPConnectError, socket.timeout):
> +    except (QMPConnectError, ConnectError, socket.timeout):
>           return {'error': 'qemu failed: ' + str(vm.get_log())}
>   
>       try:
> diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
> index 451a0666f8..7d448f4d23 100755
> --- a/tests/qemu-iotests/tests/mirror-top-perms
> +++ b/tests/qemu-iotests/tests/mirror-top-perms
> @@ -103,7 +103,11 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
>               print('ERROR: VM B launched successfully, this should not have '
>                     'happened')
>           except qemu.qmp.QMPConnectError:
> -            assert 'Is another process using the image' in self.vm_b.get_log()
> +            pass
> +        except qemu.aqmp.ConnectError:
> +            pass
> +
> +        assert 'Is another process using the image' in self.vm_b.get_log()

But this assertion will fail if there was no exception, and so we won’t 
get to see the real problem, which is the original VM aborting (see the 
doc string).

It doesn’t really matter that much that VM B can start (hence it being a 
logged error message, not a fatal error), and when it can start, of 
course it won’t print an error – but what’s important is that the 
original VM will then abort.

I mean, not an absolute showstopper by any means, but still, the 
assertion was deliberately placed into the `except` block.

Hanna
John Snow Sept. 18, 2021, 1:12 a.m. UTC | #2
On Fri, Sep 17, 2021 at 10:35 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 17.09.21 07:40, John Snow wrote:
> > (But continue to support the old ones for now, too.)
> >
> > There are very few cases of any user of QEMUMachine or a subclass
> > thereof relying on a QMP Exception type. If you'd like to check for
> > yourself, you want to grep for all of the derivatives of QMPError,
> > excluding 'AQMPError' and its derivatives. That'd be these:
> >
> > - QMPError
> > - QMPConnectError
> > - QMPCapabilitiesError
> > - QMPTimeoutError
> > - QMPProtocolError
> > - QMPResponseError
> > - QMPBadPortError
> >
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/simplebench/bench_block_job.py    | 3 ++-
> >   tests/qemu-iotests/tests/mirror-top-perms | 6 +++++-
> >   2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/simplebench/bench_block_job.py
> b/scripts/simplebench/bench_block_job.py
> > index 4f03c12169..a403c35b08 100755
> > --- a/scripts/simplebench/bench_block_job.py
> > +++ b/scripts/simplebench/bench_block_job.py
> > @@ -28,6 +28,7 @@
> >   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'python'))
> >   from qemu.machine import QEMUMachine
> >   from qemu.qmp import QMPConnectError
> > +from qemu.aqmp import ConnectError
> >
> >
> >   def bench_block_job(cmd, cmd_args, qemu_args):
> > @@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
> >           vm.launch()
> >       except OSError as e:
> >           return {'error': 'popen failed: ' + str(e)}
> > -    except (QMPConnectError, socket.timeout):
> > +    except (QMPConnectError, ConnectError, socket.timeout):
> >           return {'error': 'qemu failed: ' + str(vm.get_log())}
> >
> >       try:
> > diff --git a/tests/qemu-iotests/tests/mirror-top-perms
> b/tests/qemu-iotests/tests/mirror-top-perms
> > index 451a0666f8..7d448f4d23 100755
> > --- a/tests/qemu-iotests/tests/mirror-top-perms
> > +++ b/tests/qemu-iotests/tests/mirror-top-perms
> > @@ -103,7 +103,11 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
> >               print('ERROR: VM B launched successfully, this should not
> have '
> >                     'happened')
> >           except qemu.qmp.QMPConnectError:
> > -            assert 'Is another process using the image' in
> self.vm_b.get_log()
> > +            pass
> > +        except qemu.aqmp.ConnectError:
> > +            pass
> > +
> > +        assert 'Is another process using the image' in
> self.vm_b.get_log()
>
> But this assertion will fail if there was no exception, and so we won’t
> get to see the real problem, which is the original VM aborting (see the
> doc string).
>

Uh, hm. OK, so the intent was that if vm_b somehow starts successfully that
we will fail the test based on output in the diff, but we'll continue on to
see other kinds of explosions so that the output is more useful for
diagnosing the failure. Gotcha.

It doesn’t really matter that much that VM B can start (hence it being a
> logged error message, not a fatal error), and when it can start, of
> course it won’t print an error – but what’s important is that the
> original VM will then abort.
>

> I mean, not an absolute showstopper by any means, but still, the
> assertion was deliberately placed into the `except` block.
>
> Hanna
>

I misunderstood the "test style" here. I'll fix it.

(Uh, I also forgot to explicitly import qemu.aqmp. It happens to work
anyway because of $reasons, but it's not very good style. I'll fix that,
too.)

--js
diff mbox series

Patch

diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index 4f03c12169..a403c35b08 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -28,6 +28,7 @@ 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import QEMUMachine
 from qemu.qmp import QMPConnectError
+from qemu.aqmp import ConnectError
 
 
 def bench_block_job(cmd, cmd_args, qemu_args):
@@ -49,7 +50,7 @@  def bench_block_job(cmd, cmd_args, qemu_args):
         vm.launch()
     except OSError as e:
         return {'error': 'popen failed: ' + str(e)}
-    except (QMPConnectError, socket.timeout):
+    except (QMPConnectError, ConnectError, socket.timeout):
         return {'error': 'qemu failed: ' + str(vm.get_log())}
 
     try:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 451a0666f8..7d448f4d23 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -103,7 +103,11 @@  class TestMirrorTopPerms(iotests.QMPTestCase):
             print('ERROR: VM B launched successfully, this should not have '
                   'happened')
         except qemu.qmp.QMPConnectError:
-            assert 'Is another process using the image' in self.vm_b.get_log()
+            pass
+        except qemu.aqmp.ConnectError:
+            pass
+
+        assert 'Is another process using the image' in self.vm_b.get_log()
 
         result = self.vm.qmp('block-job-cancel',
                              device='mirror')