Re: [PATCH 4/7] qapi: Speed up frontend tests

[Eric Blake <eblake@redhat.com>]

On 10/1/19 2:15 PM, Markus Armbruster wrote:
> "make check-qapi-schema" takes around 10s user + system time for me.
> With -j, it takes a bit over 3s real time.  We have worse tests.  It's
> still annoying when you work on the QAPI generator.

All true :)


> test-qapi.py is designed to be the simplest possible building block
> for a shell script to do the complete job (it's actually a Makefile,
> not a shell script; no real difference).  Python is just not meant for
> that.  It's for bigger blocks.
> 
> Move the post-processing and diffing into test-qapi.py, and make it
> capable of testing multiple schema files.

Sounds reasonable.

> 
> Running it once per test case now takes slightly longer than 8s.  But
> running it once for all of them takes under 0.2s.
> 
> Messing with the Makefile to run it only on the tests that need
> retesting is clearly not worth the bother.
> 
> Expected error output changes because the new normalization strips off
> $(SRCDIR)/tests/qapi-schema/ instead of just $(SRCDIR)/.
> 
> The .exit files go away, because there is no exit status to test
> anymore.

So the bulk of the patch is the mechanical fallout.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/Makefile.include                        |  16 +--

>   tests/qapi-schema/test-qapi.py                | 119 +++++++++++++++---

while these are the interesting parts.  (Using git diff -O path/to/file 
with a listing that floats those two files to the top of the diff would 
ease review slightly)

>   364 files changed, 413 insertions(+), 515 deletions(-)

>   delete mode 100644 tests/qapi-schema/struct-member-invalid.exit
>   mode change 100644 => 100755 tests/qapi-schema/test-qapi.py

I'm guessing this one is intentional.

> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 214fbd941c..1b24b8ba10 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -1102,17 +1102,11 @@ check-tests/check-block.sh: tests/check-block.sh qemu-img$(EXESUF) \
>   		$(patsubst %,%/all,$(filter %-softmmu,$(TARGET_DIRS)))
>   	@$<
>   
> -.PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
> -$(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json
> +.PHONY: check-tests/qapi-schema/frontend
> +check-tests/qapi-schema/frontend: $(addprefix $(SRC_PATH)/, $(check-qapi-schema-y))
>   	$(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts \
> -		PYTHONIOENCODING=utf-8 $(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py \
> -		$^ >$*.test.out 2>$*.test.err; \
> -		echo $$? >$*.test.exit, \
> -		"TEST","$*.out")
> -	@# Sanitize error messages (make them independent of build directory)
> -	@perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -u $(SRC_PATH)/$*.err -
> -	@diff -u $(SRC_PATH)/$*.out $*.test.out
> -	@diff -u $(SRC_PATH)/$*.exit $*.test.exit
> +	  PYTHONIOENCODING=utf-8 $(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py $^, \
> +	  TEST, check-qapi-schema)
>   

So instead of one $(call $(PYTHON)) per test, it is now one call for all 
files at once.


> +++ b/tests/qapi-schema/allow-preconfig-test.err
> @@ -1,2 +1,2 @@
> -tests/qapi-schema/allow-preconfig-test.json: In command 'allow-preconfig-test':
> -tests/qapi-schema/allow-preconfig-test.json:2: flag 'allow-preconfig' may only use true value
> +allow-preconfig-test.json: In command 'allow-preconfig-test':
> +allow-preconfig-test.json:2: flag 'allow-preconfig' may only use true value

Normalization to strip off a longer prefix could have been done in a 
separate patch, but it doesn't matter.

> +++ b/tests/qapi-schema/test-qapi.py
> @@ -1,3 +1,4 @@
> +#!/usr/bin/env python
>   #
>   # QAPI parser test harness
>   #
> @@ -11,7 +12,14 @@
>   #
>   
>   from __future__ import print_function
> +import argparse
> +import difflib
> +import os
>   import sys
> +if sys.version_info[0] < 3:
> +    from cStringIO import StringIO
> +else:
> +    from io import StringIO

I guess we can't get rid of Python 2 during 'make check' just yet?


> +
> +    print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> +          file=sys.stderr)
> +    out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
> +    err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
> +    sys.stdout.writelines(out_diff)
> +    sys.stdout.writelines(err_diff)
> +
> +    if not update:
> +        return 1
> +
> +    try:
> +        outfp.truncate(0)
> +        outfp.seek(0)
> +        outfp.writelines(actual_out)

Handy for mass-rebuilding of the directory after an error message tweak.


> +def main(argv):
> +    parser = argparse.ArgumentParser(
> +        description='QAPI schema tester')
> +    parser.add_argument('-d', '--dir', action='store', default='',
> +                        help="directory containing tests")
> +    parser.add_argument('-u', '--update', action='store_true',
> +                        help="update expected test results")
> +    parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
> +    args = parser.parse_args()
> +

No --help mode? Not strictly necessary, but can be useful.

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

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