[PATCH v2 0/8] Improve integration of iotests in the meson test harness

[PATCH v2 0/8] Improve integration of iotests in the meson test harness

[Thomas Huth]

Though "make check-block" is currently already run via the meson test
runner, it still looks like an oddball in the output of "make check" since
the tests are still run separately via the check-block.sh script. It would
be nicer if the iotests would show up like the other tests suites. For this
we have to tweak the tests/qemu-iotests/check script so that it can already
be run with "-g auto -n" during the configuration step [*], then we can
directly add the individual tests in the tests/qemu-iotests/meson.build file
already and finally get rid of the check-block.sh script.

[*] Alternatively, we could also get rid of the "auto" group and add
the test list to the tests/qemu-iotests/meson.build file directly ... 

Note: There's still something really weird that happens sometimes after
running "make check-block" with this patch set: Sometimes the terminal
is in non-echo mode, so that you have to run "reset" to get the terminal
back to normal ... I currently don't have clue what's causing that
issue ... Paolo, did you ever experience something like that with the
meson test runner?

v2:
 - Add new 1st patch to fix "make check-block SPEED=thorough"
 - Improve bash version check
 - Rewrite the 'Allow to run "./check -n"' patch to be less ugly
 - Add patch to print the "diff" of the iotests to stderr, so that
   it shows up in TAP mode, too

Thomas Huth (8):
  tests/qemu-iotests/testrunner: Allow parallel test invocations
  tests/qemu-iotests: Improve the check for GNU sed
  tests/qemu-iotests/meson.build: Improve the indentation
  tests/qemu-iotests: Allow to run "./check -n" from the source
    directory, too
  tests/qemu-iotests/meson.build: Call the 'check' script directly
  tests: Do not treat the iotests as separate meson test target anymore
  tests/qemu-iotests/testrunner: Print diff to stderr in TAP mode
  tests: Remove check-block.sh

 meson.build                      |  6 +--
 scripts/mtest2make.py            |  4 --
 tests/Makefile.include           |  9 +---
 tests/check-block.sh             | 85 --------------------------------
 tests/qemu-iotests/check         |  2 +-
 tests/qemu-iotests/common.rc     | 26 +++++-----
 tests/qemu-iotests/meson.build   | 83 +++++++++++++++++++++----------
 tests/qemu-iotests/testenv.py    |  9 +++-
 tests/qemu-iotests/testrunner.py |  7 ++-
 9 files changed, 88 insertions(+), 143 deletions(-)
 delete mode 100755 tests/check-block.sh

-- 
2.27.0

[PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Thomas Huth]

If multiple tests run in parallel, they must use unique file
names for the test output.

Suggested-by: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 0eace147b8..9d20f51bb1 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
         """
 
         f_test = Path(test)
-        f_bad = Path(f_test.name + '.out.bad')
+        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
         f_notrun = Path(f_test.name + '.notrun')
         f_casenotrun = Path(f_test.name + '.casenotrun')
         f_reference = Path(self.find_reference(test))
-- 
2.27.0

[PATCH v2 2/8] tests/qemu-iotests: Improve the check for GNU sed

[Thomas Huth]

Instead of failing the iotests if GNU sed is not available (or skipping
them completely in the check-block.sh script), it would be better to
simply skip the bash-based tests, so that the python-based tests could
still be run. Thus add the check for BusyBox sed to common.rc and mark
the tests as "not run" if GNU sed is not available. Then we can also
remove the sed checks from the check-block.sh script.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/check-block.sh         | 12 ------------
 tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
 2 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 720a46bc36..af0c574812 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then
     skip "bash version too old ==> Not running the qemu-iotests."
 fi
 
-if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
-    if ! command -v gsed >/dev/null 2>&1; then
-        skip "GNU sed not available ==> Not running the qemu-iotests."
-    fi
-else
-    # Double-check that we're not using BusyBox' sed which says
-    # that "This is not GNU sed version 4.0" ...
-    if sed --version | grep -q 'not GNU sed' ; then
-        skip "BusyBox sed not supported ==> Not running the qemu-iotests."
-    fi
-fi
-
 cd tests/qemu-iotests
 
 # QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9885030b43..9ea504810c 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,17 +17,27 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+# bail out, setting up .notrun file
+_notrun()
+{
+    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
+    echo "$seq not run: $*"
+    status=0
+    exit
+}
+
+# We need GNU sed for the iotests. Make sure to not use BusyBox sed
+# which says that "This is not GNU sed version 4.0"
 SED=
 for sed in sed gsed; do
-    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
+    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 2>&1
     if [ "$?" -eq 0 ]; then
         SED=$sed
         break
     fi
 done
 if [ -z "$SED" ]; then
-    echo "$0: GNU sed not found"
-    exit 1
+    _notrun "GNU sed not found"
 fi
 
 dd()
@@ -722,16 +732,6 @@ _img_info()
         done
 }
 
-# bail out, setting up .notrun file
-#
-_notrun()
-{
-    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
-    echo "$seq not run: $*"
-    status=0
-    exit
-}
-
 # bail out, setting up .casenotrun file
 # The function _casenotrun() is used as a notifier. It is the
 # caller's responsibility to make skipped a particular test.
-- 
2.27.0

[PATCH v2 3/8] tests/qemu-iotests/meson.build: Improve the indentation

[Thomas Huth]

By using subdir_done(), we can get rid of one level of indentation
in this file. This will make it easier to add more conditions to
skip the iotests in future patches.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/meson.build | 61 ++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index 5be3c74127..e1832c90e0 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -1,30 +1,33 @@
-if have_tools and targetos != 'windows'
-  qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
-  qemu_iotests_env = {'PYTHON': python.full_path()}
-  qemu_iotests_formats = {
-    'qcow2': 'quick',
-    'raw': 'slow',
-    'qed': 'thorough',
-    'vmdk': 'thorough',
-    'vpc': 'thorough'
-  }
-
-  foreach k, v : emulators
-    if k.startswith('qemu-system-')
-      qemu_iotests_binaries += v
-    endif
-  endforeach
-  foreach format, speed: qemu_iotests_formats
-    if speed == 'quick'
-      suites = 'block'
-    else
-      suites = ['block-' + speed, speed]
-    endif
-    test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
-         depends: qemu_iotests_binaries, env: qemu_iotests_env,
-         protocol: 'tap',
-         suite: suites,
-         timeout: 0,
-         is_parallel: false)
-  endforeach
+if not have_tools or targetos == 'windows'
+  subdir_done()
 endif
+
+qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
+qemu_iotests_env = {'PYTHON': python.full_path()}
+qemu_iotests_formats = {
+  'qcow2': 'quick',
+  'raw': 'slow',
+  'qed': 'thorough',
+  'vmdk': 'thorough',
+  'vpc': 'thorough'
+}
+
+foreach k, v : emulators
+  if k.startswith('qemu-system-')
+    qemu_iotests_binaries += v
+  endif
+endforeach
+
+foreach format, speed: qemu_iotests_formats
+  if speed == 'quick'
+    suites = 'block'
+  else
+    suites = ['block-' + speed, speed]
+  endif
+  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
+       depends: qemu_iotests_binaries, env: qemu_iotests_env,
+       protocol: 'tap',
+       suite: suites,
+       timeout: 0,
+       is_parallel: false)
+endforeach
-- 
2.27.0

[PATCH v2 4/8] tests/qemu-iotests: Allow to run "./check -n" from the source directory, too

[Thomas Huth]

For better integration of the iotests into the meson build system, it
would be very helpful to get the list of the tests in the "auto" group
during the "configure" step already. However, "check -n -g auto"
currently only works if the binaries have already been built. To fix this,
skip the check for the binaries while setting up the TestEnv.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/check      | 2 +-
 tests/qemu-iotests/testenv.py | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 75de1b4691..a67b831534 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -124,7 +124,7 @@ if __name__ == '__main__':
                   aiomode=args.aiomode, cachemode=args.cachemode,
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind,
-                  gdb=args.gdb, qprint=args.print)
+                  gdb=args.gdb, qprint=args.print, dry_run=args.dry_run)
 
     if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
         if not args.tests:
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 0f32897fe8..ec8c83ab07 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -180,7 +180,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
                  debug: bool = False,
                  valgrind: bool = False,
                  gdb: bool = False,
-                 qprint: bool = False) -> None:
+                 qprint: bool = False,
+                 dry_run: bool = False) -> None:
         self.imgfmt = imgfmt
         self.imgproto = imgproto
         self.aiomode = aiomode
@@ -226,6 +227,12 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
         self.build_root = os.path.join(self.build_iotests, '..', '..')
 
         self.init_directories()
+
+        # Don't try to look for binaries etc. in dry run mode, so that
+        # the dry run mode also works without building the binaries first
+        if dry_run:
+            return
+
         self.init_binaries()
 
         self.malloc_perturb_ = os.getenv('MALLOC_PERTURB_',
-- 
2.27.0

[PATCH v2 5/8] tests/qemu-iotests/meson.build: Call the 'check' script directly

[Thomas Huth]

We can get a nicer progress indication if we add the iotests
individually via the 'check' script instead of going through
the check-block.sh wrapper.

For this, we have to add some of the sanity checks that have
originally been done in the tests/check-block.sh script (whether
"bash" is available or whether CFLAGS contain -fsanitize switches)
to the meson.build file now, and add the environment variables
that have been set up by the tests/check-block.sh script before.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/meson.build | 44 +++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index e1832c90e0..a9eade902f 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -1,9 +1,28 @@
-if not have_tools or targetos == 'windows'
+if not have_tools or targetos == 'windows' or \
+   config_host.has_key('CONFIG_GPROF')
   subdir_done()
 endif
 
+bash = find_program('bash', required: false, version: '>= 4.0')
+if not bash.found()
+  message('bash >= v4.0 not available ==> Disabled the qemu-iotests.')
+  subdir_done()
+endif
+
+foreach cflag: config_host['QEMU_CFLAGS'].split()
+  if cflag.startswith('-fsanitize') and \
+     not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
+    message('Sanitizers are enabled ==> Disabled the qemu-iotests.')
+    subdir_done()
+  endif
+endforeach
+
 qemu_iotests_binaries = [qemu_img, qemu_io, qemu_nbd, qsd]
-qemu_iotests_env = {'PYTHON': python.full_path()}
+qemu_iotests_env = {
+  'PYTHON': python.full_path(),
+  'PYTHONUTF8': '1',
+  'QEMU_CHECK_BLOCK_AUTO': '1'
+}
 qemu_iotests_formats = {
   'qcow2': 'quick',
   'raw': 'slow',
@@ -18,16 +37,25 @@ foreach k, v : emulators
   endif
 endforeach
 
+check_script = find_program(meson.current_build_dir() / 'check')
+iotests = run_command(python, [check_script.full_path(), '-g', 'auto', '-n'],
+                      check: true).stdout().strip().replace('tests/', '').split('\n')
+
 foreach format, speed: qemu_iotests_formats
   if speed == 'quick'
     suites = 'block'
   else
     suites = ['block-' + speed, speed]
   endif
-  test('qemu-iotests ' + format, sh, args: [files('../check-block.sh'), format],
-       depends: qemu_iotests_binaries, env: qemu_iotests_env,
-       protocol: 'tap',
-       suite: suites,
-       timeout: 0,
-       is_parallel: false)
+  foreach tst: iotests
+    test('iotest-' + format + '-' + tst,
+         python, args: [check_script.full_path(), '-tap', '-' + format, tst],
+         depends: qemu_iotests_binaries,
+         env: qemu_iotests_env + \
+              { 'TEST_DIR':
+                meson.current_build_dir() / 'scratch' / format + '-' + tst },
+         protocol: 'tap',
+         suite: suites,
+         timeout: 0)
+  endforeach
 endforeach
-- 
2.27.0

[PATCH v2 6/8] tests: Do not treat the iotests as separate meson test target anymore

[Thomas Huth]

Now that we add the single iotests directly in meson.build, we do
not have to separate the block suite from the other suits anymore.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 meson.build            | 6 +++---
 scripts/mtest2make.py  | 4 ----
 tests/Makefile.include | 9 +--------
 3 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/meson.build b/meson.build
index 5f43355071..b203402ee1 100644
--- a/meson.build
+++ b/meson.build
@@ -3,9 +3,9 @@ project('qemu', ['c'], meson_version: '>=0.58.2',
                           'b_staticpic=false', 'stdsplit=false'],
         version: files('VERSION'))
 
-add_test_setup('quick', exclude_suites: ['block', 'slow', 'thorough'], is_default: true)
-add_test_setup('slow', exclude_suites: ['block', 'thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
-add_test_setup('thorough', exclude_suites: ['block'], env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
+add_test_setup('quick', exclude_suites: ['slow', 'thorough'], is_default: true)
+add_test_setup('slow', exclude_suites: ['thorough'], env: ['G_TEST_SLOW=1', 'SPEED=slow'])
+add_test_setup('thorough', env: ['G_TEST_SLOW=1', 'SPEED=thorough'])
 
 not_found = dependency('', required: false)
 keyval = import('keyval')
diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
index 4d542e8aaa..304634b71e 100644
--- a/scripts/mtest2make.py
+++ b/scripts/mtest2make.py
@@ -101,10 +101,6 @@ def emit_suite(name, suite, prefix):
 testsuites = defaultdict(Suite)
 for test in introspect['tests']:
     process_tests(test, targets, testsuites)
-# HACK: check-block is a separate target so that it runs with --verbose;
-# only write the dependencies
-emit_suite_deps('block', testsuites['block'], 'check')
-del testsuites['block']
 emit_prolog(testsuites, 'check')
 for name, suite in testsuites.items():
     emit_suite(name, suite, 'check')
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 9157a57b1a..f93ae5b479 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -151,16 +151,9 @@ check-acceptance: check-acceptance-deprecated-warning | check-avocado
 
 # Consolidated targets
 
-.PHONY: check-block check check-clean get-vm-images
+.PHONY: check check-clean get-vm-images
 check:
 
-ifeq ($(CONFIG_TOOLS)$(CONFIG_POSIX),yy)
-check: check-block
-check-block: run-ninja
-	$(if $(MAKE.n),,+)$(MESON) test $(MTESTARGS) $(.mtestargs) --verbose \
-		--logbase iotestslog $(call .speed.$(SPEED), block block-slow block-thorough)
-endif
-
 check-build: run-ninja
 
 check-clean:
-- 
2.27.0

[PATCH v2 7/8] tests/qemu-iotests/testrunner: Print diff to stderr in TAP mode

[Thomas Huth]

When running in TAP mode, stdout is reserved for the TAP protocol.
To see the "diff" of the failed test, we have to print it to
stderr instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 9d20f51bb1..1f7ca1f2f9 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -404,7 +404,10 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
             if res.status == 'fail':
                 failed.append(name)
                 if res.diff:
-                    print('\n'.join(res.diff))
+                    if self.tap:
+                        print('\n'.join(res.diff), file=sys.stderr)
+                    else:
+                        print('\n'.join(res.diff))
             elif res.status == 'not run':
                 notrun.append(name)
             elif res.status == 'pass':
-- 
2.27.0

[PATCH v2 8/8] tests: Remove check-block.sh

[Thomas Huth]

Now that the iotests are added by the meson.build file already,
we do not need the check-block.sh wrapper script anymore.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/check-block.sh | 73 --------------------------------------------
 1 file changed, 73 deletions(-)
 delete mode 100755 tests/check-block.sh

diff --git a/tests/check-block.sh b/tests/check-block.sh
deleted file mode 100755
index af0c574812..0000000000
--- a/tests/check-block.sh
+++ /dev/null
@@ -1,73 +0,0 @@
-#!/bin/sh
-
-if [ "$#" -eq 0 ]; then
-    echo "Usage: $0 fmt..." >&2
-    exit 99
-fi
-
-# Honor the SPEED environment variable, just like we do it for "meson test"
-format_list="$@"
-if [ "$SPEED" = "slow" ] || [ "$SPEED" = "thorough" ]; then
-    group=
-else
-    group="-g auto"
-fi
-
-skip() {
-    echo "1..0 #SKIP $*"
-    exit 0
-}
-
-if grep -q "CONFIG_GPROF=y" config-host.mak 2>/dev/null ; then
-    skip "GPROF is enabled ==> Not running the qemu-iotests."
-fi
-
-# Disable tests with any sanitizer except for specific ones
-SANITIZE_FLAGS=$( grep "CFLAGS.*-fsanitize" config-host.mak 2>/dev/null )
-ALLOWED_SANITIZE_FLAGS="safe-stack cfi-icall"
-#Remove all occurrencies of allowed Sanitize flags
-for j in ${ALLOWED_SANITIZE_FLAGS}; do
-    TMP_FLAGS=${SANITIZE_FLAGS}
-    SANITIZE_FLAGS=""
-    for i in ${TMP_FLAGS}; do
-        if ! echo ${i} | grep -q "${j}" 2>/dev/null; then
-            SANITIZE_FLAGS="${SANITIZE_FLAGS} ${i}"
-        fi
-    done
-done
-if echo ${SANITIZE_FLAGS} | grep -q "\-fsanitize" 2>/dev/null; then
-    # Have a sanitize flag that is not allowed, stop
-    skip "Sanitizers are enabled ==> Not running the qemu-iotests."
-fi
-
-if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
-    skip "No qemu-system binary available ==> Not running the qemu-iotests."
-fi
-
-if ! command -v bash >/dev/null 2>&1 ; then
-    skip "bash not available ==> Not running the qemu-iotests."
-fi
-
-if LANG=C bash --version | grep -q 'GNU bash, version [123]' ; then
-    skip "bash version too old ==> Not running the qemu-iotests."
-fi
-
-cd tests/qemu-iotests
-
-# QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
-export QEMU_CHECK_BLOCK_AUTO=1
-export PYTHONUTF8=1
-# If make was called with -jN we want to call ./check with -j N. Extract the
-# flag from MAKEFLAGS, so that if it absent (or MAKEFLAGS is not defined), JOBS
-# would be an empty line otherwise JOBS is prepared string of flag with value:
-# "-j N"
-# Note, that the following works even if make was called with "-j N" or even
-# "--jobs N", as all these variants becomes simply "-jN" in MAKEFLAGS variable.
-JOBS=$(echo "$MAKEFLAGS" | sed -n 's/\(^\|.* \)-j\([0-9]\+\)\( .*\|$\)/-j \2/p')
-
-ret=0
-for fmt in $format_list ; do
-    ${PYTHON} ./check $JOBS -tap -$fmt $group || ret=1
-done
-
-exit $ret
-- 
2.27.0

Re: [PATCH v2 2/8] tests/qemu-iotests: Improve the check for GNU sed

[Kevin Wolf]

Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
> Instead of failing the iotests if GNU sed is not available (or skipping
> them completely in the check-block.sh script), it would be better to
> simply skip the bash-based tests, so that the python-based tests could
> still be run. Thus add the check for BusyBox sed to common.rc and mark
> the tests as "not run" if GNU sed is not available. Then we can also
> remove the sed checks from the check-block.sh script.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

I agree that skipping bash tests is slightly better than skipping all
tests. And that the skipping should really be done in qemu-iotests
itself and not in a wrapper around it.

But can't we make it even better and skip only bash tests that actually
use sed?

> +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
> +# which says that "This is not GNU sed version 4.0"
>  SED=
>  for sed in sed gsed; do
> -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
> +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > /dev/null 2>&1
>      if [ "$?" -eq 0 ]; then
>          SED=$sed
>          break
>      fi
>  done
>  if [ -z "$SED" ]; then
> -    echo "$0: GNU sed not found"
> -    exit 1
> +    _notrun "GNU sed not found"
>  fi

Couldn't we just define 'sed' as a function or alias here that skips the
test with _notrun only when it's actually called?

Kevin

Re: [PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Kevin Wolf]

Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
> If multiple tests run in parallel, they must use unique file
> names for the test output.
> 
> Suggested-by: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/qemu-iotests/testrunner.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> index 0eace147b8..9d20f51bb1 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
>          """
>  
>          f_test = Path(test)
> -        f_bad = Path(f_test.name + '.out.bad')
> +        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
>          f_notrun = Path(f_test.name + '.notrun')
>          f_casenotrun = Path(f_test.name + '.casenotrun')
>          f_reference = Path(self.find_reference(test))

Hmm... It does make sense, but nobody ever cleans those files up.
Currently, the next run of the test will just overwrite the existing
file or delete it when the test succeeds. So after running the test
suite, you have .out.bad files for every failed test, but not for those
that succeeded.

After this change, won't the test directory accumulate tons of .out.bad
files over time?

Kevin

Re: [PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Vladimir Sementsov-Ogievskiy]

11.02.2022 12:29, Kevin Wolf wrote:
> Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
>> If multiple tests run in parallel, they must use unique file
>> names for the test output.
>>
>> Suggested-by: Hanna Reitz <hreitz@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/qemu-iotests/testrunner.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
>> index 0eace147b8..9d20f51bb1 100644
>> --- a/tests/qemu-iotests/testrunner.py
>> +++ b/tests/qemu-iotests/testrunner.py
>> @@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
>>           """
>>   
>>           f_test = Path(test)
>> -        f_bad = Path(f_test.name + '.out.bad')
>> +        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
>>           f_notrun = Path(f_test.name + '.notrun')
>>           f_casenotrun = Path(f_test.name + '.casenotrun')
>>           f_reference = Path(self.find_reference(test))
> 
> Hmm... It does make sense, but nobody ever cleans those files up.
> Currently, the next run of the test will just overwrite the existing
> file or delete it when the test succeeds. So after running the test
> suite, you have .out.bad files for every failed test, but not for those
> that succeeded.
> 
> After this change, won't the test directory accumulate tons of .out.bad
> files over time?
> 

Actually, .out.bad files are put not to TEST_DIR but to build/tests/qemu-iotests..

If we want to do several runs in parallel, I think all files that test-run produces should be in TEST_DIR and SOCK_DIR. And we should care to keep TEST_DIR/*.out.bad after test-run, so user can examine them.


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Thomas Huth]

On 11/02/2022 10.29, Kevin Wolf wrote:
> Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
>> If multiple tests run in parallel, they must use unique file
>> names for the test output.
>>
>> Suggested-by: Hanna Reitz <hreitz@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   tests/qemu-iotests/testrunner.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
>> index 0eace147b8..9d20f51bb1 100644
>> --- a/tests/qemu-iotests/testrunner.py
>> +++ b/tests/qemu-iotests/testrunner.py
>> @@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
>>           """
>>   
>>           f_test = Path(test)
>> -        f_bad = Path(f_test.name + '.out.bad')
>> +        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
>>           f_notrun = Path(f_test.name + '.notrun')
>>           f_casenotrun = Path(f_test.name + '.casenotrun')
>>           f_reference = Path(self.find_reference(test))
> 
> Hmm... It does make sense, but nobody ever cleans those files up.
> Currently, the next run of the test will just overwrite the existing
> file or delete it when the test succeeds. So after running the test
> suite, you have .out.bad files for every failed test, but not for those
> that succeeded.
> 
> After this change, won't the test directory accumulate tons of .out.bad
> files over time?

True ... but we certainly want to keep the file for failed tests for further 
analysis instead of immediately deleting them ... maybe it would be enough 
to encode the image format (qcow2, qed, vmdk, ...) into the output name, 
instead of using the PID, so that "make check SPEED=thorough" works as 
expected here?

  Thomas

Re: [PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Kevin Wolf]

Am 11.02.2022 um 14:53 hat Thomas Huth geschrieben:
> On 11/02/2022 10.29, Kevin Wolf wrote:
> > Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
> > > If multiple tests run in parallel, they must use unique file
> > > names for the test output.
> > > 
> > > Suggested-by: Hanna Reitz <hreitz@redhat.com>
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   tests/qemu-iotests/testrunner.py | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> > > index 0eace147b8..9d20f51bb1 100644
> > > --- a/tests/qemu-iotests/testrunner.py
> > > +++ b/tests/qemu-iotests/testrunner.py
> > > @@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
> > >           """
> > >           f_test = Path(test)
> > > -        f_bad = Path(f_test.name + '.out.bad')
> > > +        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
> > >           f_notrun = Path(f_test.name + '.notrun')
> > >           f_casenotrun = Path(f_test.name + '.casenotrun')
> > >           f_reference = Path(self.find_reference(test))
> > 
> > Hmm... It does make sense, but nobody ever cleans those files up.
> > Currently, the next run of the test will just overwrite the existing
> > file or delete it when the test succeeds. So after running the test
> > suite, you have .out.bad files for every failed test, but not for those
> > that succeeded.
> > 
> > After this change, won't the test directory accumulate tons of .out.bad
> > files over time?
> 
> True ... but we certainly want to keep the file for failed tests for further
> analysis instead of immediately deleting them ... maybe it would be enough
> to encode the image format (qcow2, qed, vmdk, ...) into the output name,
> instead of using the PID, so that "make check SPEED=thorough" works as
> expected here?

It depends on what the supported use case for test suites running in
parallel is. If it's just for testing multiple formats at the same time,
then this would work, yes.

I could think of more test runs that you might want to do in parallel,
like different protocols, different image format options, maybe even
different host file system. I'm not sure if all (or any) of these are
relevant, though.

Supporting only things that "make check" uses might be a good
compromise.

Kevin

Re: [PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Hanna Reitz]

On 11.02.22 17:00, Kevin Wolf wrote:
> Am 11.02.2022 um 14:53 hat Thomas Huth geschrieben:
>> On 11/02/2022 10.29, Kevin Wolf wrote:
>>> Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
>>>> If multiple tests run in parallel, they must use unique file
>>>> names for the test output.
>>>>
>>>> Suggested-by: Hanna Reitz <hreitz@redhat.com>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    tests/qemu-iotests/testrunner.py | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
>>>> index 0eace147b8..9d20f51bb1 100644
>>>> --- a/tests/qemu-iotests/testrunner.py
>>>> +++ b/tests/qemu-iotests/testrunner.py
>>>> @@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
>>>>            """
>>>>            f_test = Path(test)
>>>> -        f_bad = Path(f_test.name + '.out.bad')
>>>> +        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
>>>>            f_notrun = Path(f_test.name + '.notrun')
>>>>            f_casenotrun = Path(f_test.name + '.casenotrun')
>>>>            f_reference = Path(self.find_reference(test))
>>> Hmm... It does make sense, but nobody ever cleans those files up.
>>> Currently, the next run of the test will just overwrite the existing
>>> file or delete it when the test succeeds. So after running the test
>>> suite, you have .out.bad files for every failed test, but not for those
>>> that succeeded.
>>>
>>> After this change, won't the test directory accumulate tons of .out.bad
>>> files over time?
>> True ... but we certainly want to keep the file for failed tests for further
>> analysis instead of immediately deleting them ... maybe it would be enough
>> to encode the image format (qcow2, qed, vmdk, ...) into the output name,
>> instead of using the PID, so that "make check SPEED=thorough" works as
>> expected here?
> It depends on what the supported use case for test suites running in
> parallel is. If it's just for testing multiple formats at the same time,
> then this would work, yes.
>
> I could think of more test runs that you might want to do in parallel,
> like different protocols, different image format options, maybe even
> different host file system. I'm not sure if all (or any) of these are
> relevant, though.
>
> Supporting only things that "make check" uses might be a good
> compromise.

Personally and originally, I wrote that diff to allow me to actually run 
the very same test many times in parallel.  If an error occurs only very 
rarely, then I like running like 24 loops of the same test with exactly 
the same configuration (just different TEST_DIRs, of course) in parallel.

The fact that the .out.bad files tend to accumulate is why I haven’t 
sent it upstream so far.  Personally, I like Vladimir’s idea to put them 
into TEST_DIR, but probably just because this works so well for my usual 
case where TEST_DIR is on tmpfs and I thus don’t have to clean it up.

Hanna

Re: [PATCH v2 1/8] tests/qemu-iotests/testrunner: Allow parallel test invocations

[Kevin Wolf]

Am 11.02.2022 um 17:14 hat Hanna Reitz geschrieben:
> On 11.02.22 17:00, Kevin Wolf wrote:
> > Am 11.02.2022 um 14:53 hat Thomas Huth geschrieben:
> > > On 11/02/2022 10.29, Kevin Wolf wrote:
> > > > Am 09.02.2022 um 11:15 hat Thomas Huth geschrieben:
> > > > > If multiple tests run in parallel, they must use unique file
> > > > > names for the test output.
> > > > > 
> > > > > Suggested-by: Hanna Reitz <hreitz@redhat.com>
> > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > > ---
> > > > >    tests/qemu-iotests/testrunner.py | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> > > > > index 0eace147b8..9d20f51bb1 100644
> > > > > --- a/tests/qemu-iotests/testrunner.py
> > > > > +++ b/tests/qemu-iotests/testrunner.py
> > > > > @@ -259,7 +259,7 @@ def do_run_test(self, test: str, mp: bool) -> TestResult:
> > > > >            """
> > > > >            f_test = Path(test)
> > > > > -        f_bad = Path(f_test.name + '.out.bad')
> > > > > +        f_bad = Path(f'{os.getpid()}-{f_test.name}.out.bad')
> > > > >            f_notrun = Path(f_test.name + '.notrun')
> > > > >            f_casenotrun = Path(f_test.name + '.casenotrun')
> > > > >            f_reference = Path(self.find_reference(test))
> > > > Hmm... It does make sense, but nobody ever cleans those files up.
> > > > Currently, the next run of the test will just overwrite the existing
> > > > file or delete it when the test succeeds. So after running the test
> > > > suite, you have .out.bad files for every failed test, but not for those
> > > > that succeeded.
> > > > 
> > > > After this change, won't the test directory accumulate tons of .out.bad
> > > > files over time?
> > > True ... but we certainly want to keep the file for failed tests for further
> > > analysis instead of immediately deleting them ... maybe it would be enough
> > > to encode the image format (qcow2, qed, vmdk, ...) into the output name,
> > > instead of using the PID, so that "make check SPEED=thorough" works as
> > > expected here?
> > It depends on what the supported use case for test suites running in
> > parallel is. If it's just for testing multiple formats at the same time,
> > then this would work, yes.
> > 
> > I could think of more test runs that you might want to do in parallel,
> > like different protocols, different image format options, maybe even
> > different host file system. I'm not sure if all (or any) of these are
> > relevant, though.
> > 
> > Supporting only things that "make check" uses might be a good
> > compromise.
> 
> Personally and originally, I wrote that diff to allow me to actually run the
> very same test many times in parallel.  If an error occurs only very rarely,
> then I like running like 24 loops of the same test with exactly the same
> configuration (just different TEST_DIRs, of course) in parallel.
> 
> The fact that the .out.bad files tend to accumulate is why I haven’t sent it
> upstream so far.  Personally, I like Vladimir’s idea to put them into
> TEST_DIR, but probably just because this works so well for my usual case
> where TEST_DIR is on tmpfs and I thus don’t have to clean it up.

I think it could actually work fine because if you don't override
TEST_DIR, it's the same every time, and then you get the old behaviour,
just with the .out.bad files moved into scratch/.

Kevin

Re: [PATCH v2 7/8] tests/qemu-iotests/testrunner: Print diff to stderr in TAP mode

[Paolo Bonzini]

On 2/9/22 11:15, Thomas Huth wrote:
> When running in TAP mode, stdout is reserved for the TAP protocol.
> To see the "diff" of the failed test, we have to print it to
> stderr instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qemu-iotests/testrunner.py | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> index 9d20f51bb1..1f7ca1f2f9 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -404,7 +404,10 @@ def run_tests(self, tests: List[str], jobs: int = 1) -> bool:
>               if res.status == 'fail':
>                   failed.append(name)
>                   if res.diff:
> -                    print('\n'.join(res.diff))
> +                    if self.tap:
> +                        print('\n'.join(res.diff), file=sys.stderr)
> +                    else:
> +                        print('\n'.join(res.diff))
>               elif res.status == 'not run':
>                   notrun.append(name)
>               elif res.status == 'pass':

Queued, thanks.

Paolo