Re: [PATCH v7 08/11] iotests: add testenv.py

From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, den@openvz.org, jsnow@redhat.com,
	qemu-devel@nongnu.org, mreitz@redhat.com
Subject: Re: [PATCH v7 08/11] iotests: add testenv.py
Date: Thu, 21 Jan 2021 10:48:13 -0600	[thread overview]
Message-ID: <8088599e-fb3f-5296-6ced-12b09c911af2@redhat.com> (raw)
In-Reply-To: <20210116134424.82867-9-vsementsov@virtuozzo.com>

On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add TestEnv class, which will handle test environment in a new python
> iotests running framework.
> 
> Difference with current ./check interface:
> - -v (verbose) option dropped, as it is unused
> 
> - -xdiff option is dropped, until somebody complains that it is needed
> - same for -n option
> - same for looking for binaries in $build_iotests directory.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/testenv.py | 255 ++++++++++++++++++++++++++++++++++
>  1 file changed, 255 insertions(+)
>  create mode 100644 tests/qemu-iotests/testenv.py
> 
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> new file mode 100644
> index 0000000000..0fe5076088
> --- /dev/null
> +++ b/tests/qemu-iotests/testenv.py
> @@ -0,0 +1,255 @@
> +# TestEnv class to manage test environment variables.

Should this have permissions 755 and a #! python line?

> +def get_default_machine(qemu_prog: str) -> str:
> +    outp = subprocess.run([qemu_prog, '-machine', 'help'], check=True,
> +                          text=True, stdout=subprocess.PIPE).stdout
> +
> +    machines = outp.split('\n')
> +    default_machine = next(m for m in machines if m.endswith(' (default)'))
> +    default_machine = default_machine.split(' ', 1)[0]
> +
> +    alias_suf = ' (alias of {})'.format(default_machine)
> +    alias = next((m for m in machines if m.endswith(alias_suf)), None)
> +    if alias is not None:
> +        default_machine = alias.split(' ', 1)[0]
> +
> +    return default_machine

I have no idea if this is the most efficient and idiomatic way for
python to extract the default machine out of the output list, but it
seems to work.

> +
> +
> +class TestEnv(AbstractContextManager['TestEnv']):

> +        self.qsd_prog = os.getenv('QSD_PROG', root('storage-daemon',
> +                                                   'qemu-storage-daemon'))
> +
> +        for b in [self.qemu_img_prog, self.qemu_io_prog, self.qemu_nbd_prog,
> +                  self.qemu_prog, self.qsd_prog]:
> +            if not os.path.exists(b):
> +                sys.exit('Not such file: ' + b)

No such file

> +        if self.imgfmt == 'vmkd':

vmdk (which means we need to test that './check -vmdk' still works...)

I can see a rough correspondence to the existing shell code in ./check,
and with the entire series applied, things still work (other than vmdk,
which should work with the typo fix).  I'm not comfortable enough to
offer my full review of the python code, but I can give:

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

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