[12/15] iotests/migration-permissions: use assertRaises() for qemu_io() negative test

Modify this test to use assertRaises for its negative testing of
qemu_io. If the exception raised does not match the one we tell it to
expect, we get *that* exception unhandled. If we get no exception, we
get a unittest assertion failure and the provided emsg printed to
screen.

If we get the CalledProcessError exception but the output is not what we
expect, we re-raise the original CalledProcessError.

Tidy.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

On Fri, Mar 18, 2022 at 04:36:52PM -0400, John Snow wrote:
> Modify this test to use assertRaises for its negative testing of
> qemu_io. If the exception raised does not match the one we tell it to
> expect, we get *that* exception unhandled. If we get no exception, we
> get a unittest assertion failure and the provided emsg printed to
> screen.
> 
> If we get the CalledProcessError exception but the output is not what we
> expect, we re-raise the original CalledProcessError.
> 
> Tidy.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
>  1 file changed, 14 insertions(+), 14 deletions(-)

The 11/15 patch is also included in my R-b of the squashed patch.

For this patch, it's a wash on lines of code, but certainly a more
robust test now than with the hack of the fixup patch 11/15.

> 
> diff --git a/tests/qemu-iotests/tests/migration-permissions b/tests/qemu-iotests/tests/migration-permissions
> index c7afb1bd2c..4e1da369c9 100755
> --- a/tests/qemu-iotests/tests/migration-permissions
> +++ b/tests/qemu-iotests/tests/migration-permissions
> @@ -18,6 +18,8 @@
>  #
>  
>  import os
> +from subprocess import CalledProcessError
> +
>  import iotests
>  from iotests import imgfmt, qemu_img_create, qemu_io
>  
> @@ -69,13 +71,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
>      def test_post_migration_permissions(self):
>          # Try to access the image R/W, which should fail because virtio-blk
>          # has not been configured with share-rw=on
> -        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
> -        if not log.strip():
> -            print('ERROR (pre-migration): qemu-io should not be able to '
> -                  'access this image, but it reported no error')
> -        else:
> -            # This is the expected output
> -            assert 'Is another process using the image' in log
> +        emsg = ('ERROR (pre-migration): qemu-io should not be able to '
> +                'access this image, but it reported no error')
> +        with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
> +            qemu_io('-f', imgfmt, '-c', 'quit', test_img)
> +        if 'Is another process using the image' not in ctx.exception.stdout:

Wait a minute - is 'ctx' still in scope after the 'with ... as ctx:'
with scope ends?  Python continues to surprise me.

> +            raise ctx.exception
>  
>          # Now migrate the VM
>          self.vm_s.qmp('migrate', uri=f'unix:{mig_sock}')
> @@ -84,13 +85,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
>  
>          # Try the same qemu-io access again, verifying that the WRITE
>          # permission remains unshared
> -        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
> -        if not log.strip():
> -            print('ERROR (post-migration): qemu-io should not be able to '
> -                  'access this image, but it reported no error')
> -        else:
> -            # This is the expected output
> -            assert 'Is another process using the image' in log
> +        emsg = ('ERROR (post-migration): qemu-io should not be able to '
> +                'access this image, but it reported no error')
> +        with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
> +            qemu_io('-f', imgfmt, '-c', 'quit', test_img)
> +        if 'Is another process using the image' not in ctx.exception.stdout:
> +            raise ctx.exception

The cover letter didn't mention a Based-on: tag, and so my first
attempt to reproduce this hit a rebase error on 5/15 followed by
'./check -raw migration-permissions' on this patch failing with:

+Traceback (most recent call last):
+  File "/home/eblake/qemu/tests/qemu-iotests/tests/migration-permissions", line 77, in test_post_migration_permissions
+    qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+  File "/home/eblake/qemu/tests/qemu-iotests/iotests.py", line 334, in qemu_io
+    return qemu_tool(*qemu_io_wrap_args(args),
+  File "/home/eblake/qemu/tests/qemu-iotests/iotests.py", line 249, in qemu_tool
+    raise VerboseProcessError(
+NameError: name 'VerboseProcessError' is not defined

so it's pretty obvious I didn't apply the pre-requisite patch that
introduced VerboseProcessError in order to fix qemu_img() first.  But
with more churn on my end (applying your v4 qemu_img series first), it
looks like accessing ctx.exception after the with clause ends (even
though the with clause is what brought ctx into existance) works, so:

Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>

On 18.03.22 21:36, John Snow wrote:
> Modify this test to use assertRaises for its negative testing of
> qemu_io. If the exception raised does not match the one we tell it to
> expect, we get *that* exception unhandled. If we get no exception, we
> get a unittest assertion failure and the provided emsg printed to
> screen.
>
> If we get the CalledProcessError exception but the output is not what we
> expect, we re-raise the original CalledProcessError.
>
> Tidy.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
>   1 file changed, 14 insertions(+), 14 deletions(-)

Just like Eric I don’t find it so tidy that `ctx` exists outside of the 
`with` block, but re-raising the exception is indeed better, so:

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

On Tue, Mar 22, 2022 at 12:37 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 18.03.22 21:36, John Snow wrote:
> > Modify this test to use assertRaises for its negative testing of
> > qemu_io. If the exception raised does not match the one we tell it to
> > expect, we get *that* exception unhandled. If we get no exception, we
> > get a unittest assertion failure and the provided emsg printed to
> > screen.
> >
> > If we get the CalledProcessError exception but the output is not what we
> > expect, we re-raise the original CalledProcessError.
> >
> > Tidy.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
> >   1 file changed, 14 insertions(+), 14 deletions(-)
>
> Just like Eric I don’t find it so tidy that `ctx` exists outside of the
> `with` block, but re-raising the exception is indeed better, so:
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>

"Sorry about Python."

You still have the object, but __exit__() will have been called, so
resources will have been freed, references to live objects severed,
etc. unittests is designed around this for Exception testing, etc.
It's not a *real* scope. It just looks like one.

I'll add a little note in the commit message for the next person who wonders.

--js