Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Eric Blake]

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  tests/qemu-iotests/201     | 130 
> +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/201.out |   5 ++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 136 insertions(+)
>  create mode 100644 tests/qemu-iotests/201
>  create mode 100644 tests/qemu-iotests/201.out
> 

pep8 complains:

$ pep8 tests/qemu-iotests/201
tests/qemu-iotests/201:31:1: E302 expected 2 blank lines, found 1
tests/qemu-iotests/201:68:17: E128 continuation line under-indented for
visual indent
tests/qemu-iotests/201:69:17: E128 continuation line under-indented for
visual indent

I don't mind cleaning those up (if you don't have to respin because of
my comments on 5/6).

> +import os
> +import sys
> +import iotests
> +import time
> +from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive
> +
> +nbd_port = '10900'
> +nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp'

Is this still going to be safe when we improve the testsuite to run
multiple tests in parallel?  Wouldn't it be safer to dynamically choose
the port, instead of hard-coding one?

> +disk = os.path.join(iotests.test_dir, 'disk')
> +
> +class TestNbdServerRemove(iotests.QMPTestCase):
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
> +
> +        self.vm = iotests.VM().add_drive(disk)
> +        self.vm.launch()
> +
> +        address = {
> +            'type': 'inet',
> +            'data': {
> +                'host': 'localhost',
> +                'port': nbd_port

Again, for a one-shot test, this works; but it doesn't lend itself to
parallel testing.  Maybe do a loop with incrementing port numbers until
one succeeds, if we can reliably detect when a port is already in use?

> +    def assertConnectFailed(self, qemu_io_output):
> +        self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
> +                         "can't open device nbd+tcp://localhost:10900/exp: " 
> +

Worth parameterizing or filtering this assertion to match the rest of
the parameterization of nbd_port?

> +                         "Requested export not available\nserver reported: " 
> +
> +                         "export 'exp' not present")
> +
> +    def do_test_connect_after_remove(self, force=None):
> +        args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri)
> +        self.assertReadOk(qemu_io(*args))
> +        self.remove_export('exp', force)
> +        self.assertExportNotFound('exp')
> +        self.assertConnectFailed(qemu_io(*args))
> +
> +    def test_connect_after_remove_default(self):
> +        self.do_test_connect_after_remove()
> +
> +    def test_connect_after_remove_safe(self):
> +        self.do_test_connect_after_remove(False)
> +
> +    def test_connect_after_remove_force(self):
> +        self.do_test_connect_after_remove(True)

May need updates if my comments on 3/6 result in us having three states
rather than just 2 (the difference being whether there is a knob for
choosing to let existing clients gracefully disconnect with all pending
I/O completed, and/or failing the nbd-server-remove if a client is still
connected).

> +++ b/tests/qemu-iotests/201.out
> @@ -0,0 +1,5 @@
> +.......
> +----------------------------------------------------------------------
> +Ran 7 tests

I'm not a fan of python tests that are difficult to debug.  Your
additions to 147 in patch 4/6 are okay (hard to debug, but an
incremental addition); but is it possible to rewrite this test in a bit
more verbose manner?  See test 194 and this message for more details:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html

> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 3e688678dd..15df7678b3 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -197,3 +197,4 @@
>  197 rw auto quick
>  198 rw auto
>  200 rw auto
> +201 rw auto quick
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature