Re: [Qemu-block] [PATCH v3 7/7] tests: Run the iotests during "make check" again

[Alex Bennée]

Thomas Huth <address@hidden> writes:

> People often forget to run the iotests before submitting patches or
> pull requests - this is likely due to the fact that we do not run the
> tests during our mandatory "make check" tests yet. Now that we've got
> a proper "auto" group of iotests that should be fine to run in every
> environment, we can enable the iotests during "make check" again by
> running the "auto" tests by default from the check-block.sh script.
>
> Some cases still need to be checked first, though: iotests need bash
> and GNU sed (otherwise they fail), and if gprof is enabled, it spoils
> the output of some test cases causing them to fail. So if we detect
> that one of the required programs is missing or that gprof is enabled,
> we still have to skip the iotests to avoid failures.
>
> And finally, since we are using check-block.sh now again, this patch also
> removes the qemu-iotests-quick.sh script since we do not need that anymore
> (and having two shell wrapper scripts around the block tests seem
> rather confusing than helpful).
>
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  tests/Makefile.include      |  8 +++----
>  tests/check-block.sh        | 44 ++++++++++++++++++++++++++++---------
>  tests/qemu-iotests-quick.sh |  8 -------
>  3 files changed, 38 insertions(+), 22 deletions(-)
>  delete mode 100755 tests/qemu-iotests-quick.sh
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e2432d5e77..3bb7793d4a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -140,7 +140,7 @@ check-unit-y += tests/test-uuid$(EXESUF)
>  check-unit-y += tests/ptimer-test$(EXESUF)
>  check-unit-y += tests/test-qapi-util$(EXESUF)
>
> -check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
> +check-block-$(CONFIG_POSIX) += tests/check-block.sh
>
>  # All QTests for now are POSIX-only, but the dependencies are
>  # really in libqtest, not in the testcases themselves.
> @@ -1096,8 +1096,8 @@ clean-tcg: $(CLEAN_TCG_TARGET_RULES)
>
>  QEMU_IOTESTS_HELPERS-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_LINUX)) = 
> tests/qemu-iotests/socket_scm_helper$(EXESUF)
>
> -.PHONY: check-tests/qemu-iotests-quick.sh
> -check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh 
> qemu-img$(EXESUF) qemu-io$(EXESUF) qemu-nbd$(EXESUF) $(QEMU_IOTESTS_HELPERS-y)
> +.PHONY: check-tests/check-block.sh
> +check-tests/check-block.sh: tests/check-block.sh qemu-img$(EXESUF) 
> qemu-io$(EXESUF) qemu-nbd$(EXESUF) $(QEMU_IOTESTS_HELPERS-y)
>       $<
>
>  .PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
> @@ -1168,7 +1168,7 @@ check-acceptance: check-venv $(TESTS_RESULTS_DIR)
>  check-qapi-schema: $(patsubst %,check-%, $(check-qapi-schema-y)) 
> check-tests/qapi-schema/doc-good.texi
>  check-qtest: $(patsubst %,check-qtest-%, $(QTEST_TARGETS))
>  check-block: $(patsubst %,check-%, $(check-block-y))
> -check: check-qapi-schema check-unit check-softfloat check-qtest 
> check-decodetree
> +check: check-qapi-schema check-unit check-softfloat check-qtest 
> check-decodetree check-block
>  check-clean:
>       rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y)
>       rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), 
> $(check-qtest-$(target)-y)) $(check-qtest-generic-y))
> diff --git a/tests/check-block.sh b/tests/check-block.sh
> index f3d12fd602..3b971d6cf4 100755
> --- a/tests/check-block.sh
> +++ b/tests/check-block.sh
> @@ -1,24 +1,48 @@
>  #!/bin/sh
>
> -FORMAT_LIST="raw qcow2 qed vmdk vpc"
> +# Honor the SPEED environment variable, just like we do it for the qtests.
> +if [ "$SPEED" = "slow" ]; then
> +    format_list="raw qcow2"
> +    group=
> +elif [ "$SPEED" = "thorough" ]; then
> +    format_list="raw qcow2 qed vmdk vpc"
> +    group=
> +else
> +    format_list=qcow2
> +    group="-g auto"
> +fi
> +
>  if [ "$#" -ne 0 ]; then
> -    FORMAT_LIST="$@"
> +    format_list="$@"
> +fi
> +
> +if grep -q "TARGET_GPROF=y" *-softmmu/config-target.mak 2>/dev/null ; then
> +    echo "GPROF is enabled ==> Not running the qemu-iotests."
> +    exit 0
>  fi
>
> -export QEMU_PROG="$PWD/x86_64-softmmu/qemu-system-x86_64"
> -export QEMU_IMG_PROG="$PWD/qemu-img"
> -export QEMU_IO_PROG="$PWD/qemu-io"
> +if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
> +    echo "No qemu-system binary available ==> Not running the qemu-iotests."
> +    exit 0
> +fi
> +
> +if ! command -v bash >/dev/null 2>&1 ; then
> +    echo "bash not available ==> Not running the qemu-iotests."
> +    exit 0
> +fi
>
> -if [ ! -x $QEMU_PROG ]; then
> -    echo "'make check-block' requires qemu-system-x86_64"
> -    exit 1
> +if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
> +    if ! command -v gsed >/dev/null 2>&1; then
> +        echo "GNU sed not available ==> Not running the qemu-iotests."
> +        exit 0
> +    fi
>  fi
>
>  cd tests/qemu-iotests
>
>  ret=0
> -for FMT in $FORMAT_LIST ; do
> -    ./check -T -nocache -$FMT || ret=1
> +for fmt in $format_list ; do
> +    ./check -$fmt $group || ret=1

This is all looking good and my only remaining objection is aesthetic.
If you run "make check -jN" you end up with the with the block test
output intermingled with the rest of the output which is now fairly
uniform.

Any chance we could hide the verbosity unless requested and have
something like:

    TEST    check-block: test xxx

emitted for each passing test? It's OK to show more in the failing case
to help with diagnosis but the passing tests should be easy on the eye
when scanning through the logs.

>  done
>
>  exit $ret
> diff --git a/tests/qemu-iotests-quick.sh b/tests/qemu-iotests-quick.sh
> deleted file mode 100755
> index 0e554bb972..0000000000
> --- a/tests/qemu-iotests-quick.sh
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -#!/bin/sh
> -
> -cd tests/qemu-iotests
> -
> -ret=0
> -TEST_DIR=${TEST_DIR:-/tmp/qemu-iotests-quick-$$} ./check -T -qcow2 -g quick 
> || ret=1
> -
> -exit $ret


--
Alex Bennée