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

[John Snow]

On 1/13/21 6:20 PM, Eric Blake wrote:

On 1/9/21 6:26 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.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/check | 994 ++-
  tests/qemu-iotests/group | 321 -
  2 files changed, 28 insertions(+), 1287 deletions(-)
  delete mode 100644 tests/qemu-iotests/group


The bulk of the work was done in the earlier patches, and my python
review is weak; but I can confirm that with this patch applied, my usual
attempts at running ./check still appeared to work for me.  That's not
the same as proving you did 1:1 feature translation (and in fact, your
commit message documented dropping some features like -v), but if 'make
check' and CI tools still run, I'm okay leaving it up to developers to
complain about any other feature that they used but which go missing (or
to implement it).

Tested-by: Eric Blake 



I will *try* to give this a look, but I am still buried under post-PTO 
mail; and I feel comfortable with Kevin's Python review otherwise, so 
don't hold your breath waiting to hear from me.




diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 952762d5ed..48bb3128c3 100755
--- a/tests/qemu-iotests/check
+++ 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 Virtuozzo International GmbH


You may want to claim 2021 as well.


+import sys
+import os
+from findtests import find_tests, TestFinder
+from testenv import TestEnv
+from testrunner import TestRunner
+
+if __name__ == '__main__':
+if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']:
+print('Usage: ./check [options] [testlist]')
+print()
+TestFinder.get_argparser().print_help()
+print()
+TestEnv.get_argparser().print_help()
+print()
+TestRunner.get_argparser().print_help()
+exit()
+
+env = TestEnv(sys.argv[1:])
+tests, remaining_argv = find_tests(env.remaining_argv,
+   test_dir=env.source_iotests)
+
+with TestRunner(remaining_argv, env) as tr:
+assert not tr.remaining_argv
+tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])


A lot shorter for the main engine ;)


diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
deleted file mode 100644
index bc5bc324fe..00
--- a/tests/qemu-iotests/group
+++ /dev/null
@@ -1,321 +0,0 @@
-#
-# QA groups control file
-# Defines test groups


Happy to see this conflict magnet go!



This rules.

--js

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

[Eric Blake]

On 1/9/21 6:26 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.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/check | 994 ++-
>  tests/qemu-iotests/group | 321 -
>  2 files changed, 28 insertions(+), 1287 deletions(-)
>  delete mode 100644 tests/qemu-iotests/group

The bulk of the work was done in the earlier patches, and my python
review is weak; but I can confirm that with this patch applied, my usual
attempts at running ./check still appeared to work for me.  That's not
the same as proving you did 1:1 feature translation (and in fact, your
commit message documented dropping some features like -v), but if 'make
check' and CI tools still run, I'm okay leaving it up to developers to
complain about any other feature that they used but which go missing (or
to implement it).

Tested-by: Eric Blake 

> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 952762d5ed..48bb3128c3 100755
> --- a/tests/qemu-iotests/check
> +++ 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 Virtuozzo International GmbH

You may want to claim 2021 as well.

> +import sys
> +import os
> +from findtests import find_tests, TestFinder
> +from testenv import TestEnv
> +from testrunner import TestRunner
> +
> +if __name__ == '__main__':
> +if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']:
> +print('Usage: ./check [options] [testlist]')
> +print()
> +TestFinder.get_argparser().print_help()
> +print()
> +TestEnv.get_argparser().print_help()
> +print()
> +TestRunner.get_argparser().print_help()
> +exit()
> +
> +env = TestEnv(sys.argv[1:])
> +tests, remaining_argv = find_tests(env.remaining_argv,
> +   test_dir=env.source_iotests)
> +
> +with TestRunner(remaining_argv, env) as tr:
> +assert not tr.remaining_argv
> +tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])

A lot shorter for the main engine ;)

> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> deleted file mode 100644
> index bc5bc324fe..00
> --- a/tests/qemu-iotests/group
> +++ /dev/null
> @@ -1,321 +0,0 @@
> -#
> -# QA groups control file
> -# Defines test groups

Happy to see this conflict magnet go!

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

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

[Kevin Wolf]

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Just use classes introduced in previous three commits. Behavior
> difference is described in these three commits.
> 
> Drop group file, as it becomes unused.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/check | 994 ++-
>  tests/qemu-iotests/group | 321 -
>  2 files changed, 28 insertions(+), 1287 deletions(-)
>  delete mode 100644 tests/qemu-iotests/group

> +import sys
> +import os
> +from findtests import find_tests, TestFinder
> +from testenv import TestEnv
> +from testrunner import TestRunner
> +
> +if __name__ == '__main__':
> +if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']:
> +print('Usage: ./check [options] [testlist]')
> +print()
> +TestFinder.get_argparser().print_help()
> +print()
> +TestEnv.get_argparser().print_help()
> +print()
> +TestRunner.get_argparser().print_help()
> +exit()

+check:34:8: R1722: Consider using sys.exit() (consider-using-sys-exit)

> +
> +env = TestEnv(sys.argv[1:])
> +tests, remaining_argv = find_tests(env.remaining_argv,
> +   test_dir=env.source_iotests)
> +
> +with TestRunner(remaining_argv, env) as tr:
> +assert not tr.remaining_argv
> +tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])

The assertion means that giving an unknown option results in an error
message like this:

$ build/check -T -raw
Traceback (most recent call last):
  File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 41, in 

assert not tr.remaining_argv
AssertionError

I think this could be a bit friendlier (especially since this doesn't
even tell you which of your options was wrong).

Kevin

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

[Vladimir Sementsov-Ogievskiy]

Just use classes introduced in previous three commits. Behavior
difference is described in these three commits.

Drop group file, as it becomes unused.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/check | 994 ++-
 tests/qemu-iotests/group | 321 -
 2 files changed, 28 insertions(+), 1287 deletions(-)
 delete mode 100644 tests/qemu-iotests/group

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 952762d5ed..48bb3128c3 100755
--- a/tests/qemu-iotests/check
+++ 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 Virtuozzo International GmbH
 #
 # This program is free software; you can redistribute it and/or
 # modify it under the terms of the GNU General Public License as
@@ -14,967 +15,28 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
-#
-#
-# Control script for QA
-#
-
-status=0
-needwrap=true
-try=0
-n_bad=0
-bad=""
-notrun=""
-casenotrun=""
-interrupt=true
-makecheck=false
-
-_init_error()
-{
-echo "check: $1" >&2
-exit 1
-}
-
-if [ -L "$0" ]
-then
-# called from the build tree
-source_iotests=$(dirname "$(readlink "$0")")
-if [ -z "$source_iotests" ]
-then
-_init_error "failed to obtain source tree name from check symlink"
-fi
-source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to 
enter source tree"
-build_iotests=$(cd "$(dirname "$0")"; pwd)
-else
-# called from the source tree
-source_iotests=$PWD
-# this may be an in-tree build (note that in the following code we may not
-# assume that it truly is and have to test whether the build results
-# actually exist)
-build_iotests=$PWD
-fi
-
-build_root="$build_iotests/../.."
-
-# we need common.env
-if ! . "$build_iotests/common.env"
-then
-_init_error "failed to source common.env (make sure the qemu-iotests are 
run from tests/qemu-iotests in the build tree)"
-fi
-
-# we need common.config
-if ! . "$source_iotests/common.config"
-then
-_init_error "failed to source common.config"
-fi
-
-_full_imgfmt_details()
-{
-if [ -n "$IMGOPTS" ]; then
-echo "$IMGFMT ($IMGOPTS)"
-else
-echo "$IMGFMT"
-fi
-}
-
-_full_platform_details()
-{
-os=$(uname -s)
-host=$(hostname -s)
-kernel=$(uname -r)
-platform=$(uname -m)
-echo "$os/$platform $host $kernel"
-}
-
-_full_env_details()
-{
-cat < /dev/null)
-if [ -n "$p" -a -x "$p" ]; then
-type -p "$p"
-else
-return 1
-fi
-}
-
-if [ -z "$TEST_DIR" ]; then
-TEST_DIR=$PWD/scratch
-fi
-mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'
-
-tmp_sock_dir=false
-if [ -z "$SOCK_DIR" ]; then
-SOCK_DIR=$(mktemp -d)
-tmp_sock_dir=true
-fi
-mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
-
-diff="diff -u"
-verbose=false
-debug=false
-group=false
-xgroup=false
-imgopts=false
-showme=false
-sortme=false
-expunge=true
-have_test_arg=false
-cachemode=false
-aiomode=false
-
-tmp="${TEST_DIR}"/$$
-rm -f $tmp.list $tmp.tmp $tmp.sed
-
-export IMGFMT=raw
-export IMGFMT_GENERIC=true
-export IMGPROTO=file
-export IMGOPTS=""
-export CACHEMODE="writeback"
-export AIOMODE="threads"
-export QEMU_IO_OPTIONS=""
-export QEMU_IO_OPTIONS_NO_FMT=""
-export CACHEMODE_IS_DEFAULT=true
-export VALGRIND_QEMU=
-export IMGKEYSECRET=
-export IMGOPTSSYNTAX=false
-
-# Save current tty settings, since an aborting qemu call may leave things
-# screwed up
-STTY_RESTORE=
-if test -t 0; then
-STTY_RESTORE=$(stty -g)
-fi
-
-for r
-do
-
-if $group
-then
-# arg after -g
-group_list=$(sed -n <"$source_iotests/group" -e 's/$/ /' -e 
"/^[0-9][0-9][0-9].* $r /"'{
-s/ .*//p
-}')
-if [ -z "$group_list" ]
-then
-echo "Group \"$r\" is empty or not defined?"
-exit 1
-fi
-[ ! -s $tmp.list ] && touch $tmp.list
-for t in $group_list
-do
-if grep -s "^$t\$" $tmp.list >/dev/null
-then
-:
-else
-echo "$t" >>$tmp.list
-fi
-done
-group=false
-continue
-
-elif $xgroup
-then
-# arg after -x
-# Populate $tmp.list with all tests
-awk '/^[0-9]{3,}/ {print $1}' "${source_iotests}/group" > $tmp.list 
2>/dev/null
-group_list=$(sed -n <"$source_iotests/group" -e 's/$/ /' -e 
"/^[0-9][0-9][0-9].* $r /"'{
-s/ .*//p
-}')
-if [ -z "$group_list" ]
-then
-echo "Group \"$r\" is empty or not defined?"
-exit 1
-fi
-numsed=0
-rm -f $tmp.sed
-for t in $group_list
-