Re: [PATCH v7 10/11] iotests: rewrite check into python

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 10/11] iotests: rewrite check into python
Date: Thu, 21 Jan 2021 11:22:59 -0600	[thread overview]
Message-ID: <4b50156b-e5ca-f3f4-32de-6ce540640aef@redhat.com> (raw)
In-Reply-To: <20210116134424.82867-11-vsementsov@virtuozzo.com>

On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Just use classes introduced in previous three commits. Behavior
> difference is described in these three commits.
> 
> Drop group file, as it becomes unused.
> 
> Drop common.env: now check is in python, and for tests we use same
> python interpreter that runs the check itself. Use build environment
> PYTHON in check-block instead, to keep "make check" use the same
> python.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

> +++ b/tests/qemu-iotests/check
> @@ -1,7 +1,8 @@
> -#!/usr/bin/env bash
> +#!/usr/bin/env python3
>  #
> -# Copyright (C) 2009 Red Hat, Inc.
> -# Copyright (c) 2000-2002,2006 Silicon Graphics, Inc.  All Rights Reserved.
> +# Configure environment and run group of tests in it.
> +#
> +# Copyright (c) 2020-2021 Virtuozzo International GmbH

Normally dropping old copyrights is suspicious, but as this is a
complete rewrite, it makes sense here.

> -exit
> +import os
> +import sys
> +import argparse
> +from findtests import TestFinder
> +from testenv import TestEnv
> +from testrunner import TestRunner
> +
> +
> +def make_argparser() -> argparse.ArgumentParser:
> +    p = argparse.ArgumentParser(description="Test run options")
> +
> +    p.add_argument('-n', '--dry-run', action='store_true',
> +                   help='show me, do not run tests')
> +    p.add_argument('-makecheck', action='store_true',
> +                   help='pretty print output for make check')
> +
> +    p.add_argument('-d', dest='debug', action='store_true', help='debug')
> +    p.add_argument('-misalign', action='store_true',
> +                   help='misalign memory allocations')
> +
> +    g_env = p.add_argument_group('test environment options')
> +    mg = g_env.add_mutually_exclusive_group()
> +    # We don't set default for cachemode, as we need to distinguish dafult

default

> +    # from user input later.
> +    mg.add_argument('-nocache', dest='cachemode', action='store_const',
> +                    const='none', help='set cache mode "none" (O_DIRECT), '
> +                    'sets CACHEMODE environment variable')
> +    mg.add_argument('-c', dest='cachemode',
> +                    help='sets CACHEMODE environment variable')
> +
> +    g_env.add_argument('-i', dest='aiomode', default='threads',
> +                       help='sets AIOMODE environment variable')
> +
> +    p.set_defaults(imgfmt='raw', imgproto='file')
> +
> +    format_list = ['raw', 'bochs', 'cloop', 'parallels', 'qcow', 'qcow2',
> +                   'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks', 'dmg']
> +    g_fmt = p.add_argument_group(
> +        '  image format options',
> +        'The following options set the IMGFMT environment variable. '
> +        'At most one choice is allowed, default is "raw"')
> +    mg = g_fmt.add_mutually_exclusive_group()
> +    for fmt in format_list:
> +        mg.add_argument('-' + fmt, dest='imgfmt', action='store_const',
> +                        const=fmt, help=f'test {fmt}')
> +
> +    protocol_list = ['file', 'rbd', 'sheepdoc', 'nbd', 'ssh', 'nfs',

sheepdog

> +                     'fuse']
> +    g_prt = p.add_argument_group(
> +        '  image protocol options',
> +        'The following options set the IMGPROTO environment variable. '
> +        'At most one choice is allowed, default is "file"')
> +    mg = g_prt.add_mutually_exclusive_group()
> +    for prt in protocol_list:
> +        mg.add_argument('-' + prt, dest='imgproto', action='store_const',
> +                        const=prt, help=f'test {prt}')
> +
> +    g_bash = p.add_argument_group('bash tests options',
> +                                  'The following options are ignored by '
> +                                  'python tests.')
> +    # TODO: make support for the following options in iotests.py
> +    g_bash.add_argument('-o', dest='imgopts',
> +                        help='options to pass to qemu-img create/convert, '
> +                        'sets IMGOPTS environment variable')
> +    g_bash.add_argument('-valgrind', dest='VALGRIND_QEMU',
> +                        action='store_const', const='y',
> +                        help='use valgrind, sets VALGRIND_QEMU environment '
> +                        'variable')
> +
> +    g_sel = p.add_argument_group('test selecting options',
> +                                 'The following options specify test set '
> +                                 'to run.')
> +    g_sel.add_argument('-g', '--groups', metavar='group1,...',
> +                       help='include tests from these groups')
> +    g_sel.add_argument('-x', '--exclude-groups', metavar='group1,...',
> +                       help='exclude tests from these groups')
> +    g_sel.add_argument('--start-from', metavar='TEST',
> +                       help='Start from specified test: make sorted sequence '
> +                       'of tests as usual and then drop tests from the first '
> +                       'one to TEST (not inclusive). This may be used to '
> +                       'rerun failed ./check command, starting from the '
> +                       'middle of the process.')
> +    g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
> +                       help='tests to run')
> +
> +    return p

The change to drop ranges and instead use --start-from makes sense (you
can't do ranges of named files); it will take a minor adjustment to my
keyboarding habits to get used to, but I don't see it as a show-stopper
as iotests are for developers and not promised to be a stable interface.

> +
> +
> +if __name__ == '__main__':
> +    args = make_argparser().parse_args()
> +
> +    env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
> +                  aiomode=args.aiomode, cachemode=args.cachemode,
> +                  imgopts=args.imgopts, misalign=args.misalign,
> +                  debug=args.debug)
> +
> +    testfinder = TestFinder(test_dir=env.source_iotests)
> +
> +    groups = args.groups.split(',') if args.groups else None
> +    x_groups = args.exlude_groups.split(',') if args.exclude_groups else None
> +
> +    try:
> +        tests = testfinder.find_tests(groups=groups, exclude_groups=x_groups,
> +                                      tests=args.tests,
> +                                      start_from=args.start_from)
> +        if not tests:
> +            raise ValueError('No tests selected')
> +    except ValueError as e:
> +        sys.exit(e)
> +
> +    if args.dry_run:
> +        print('\n'.join(tests))
> +    else:
> +        with TestRunner(env, args.makecheck) as tr:
> +            tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])

Other than the typo, this looks like a fairly straightforward conversion
of the old shell code, but at present, I'm only comfortable giving:

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

(of course assuming you fix the typo that broke ./check -sheepdog)

The longer we sit on this series, the more merge conflicts it will
introduce (new tests need to add their own #group header line); the
changes to fix those conflicts should be obvious, but I hope we get
another reviewer soon.  I'm wondering if I should take the second half
of this series (since I already sent the pull request for the first half
since it touched NBD tests), or let Max take it.

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