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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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.

Yes.  It's a program.  Our make recipe

    PYTHONIOENCODING=utf-8 $(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py $^

happens work without the executable bit.

>> 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.

Correct.

>> +++ 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?

I'd prefer to keep the tests working with Python 2 as long as we use
Python 2 with qapi-gen.py.  2 months, 29 days, 6 hours and chump change.

>> +
>> +    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.

I could've used this so many times...

>> +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.

We get that for free with argparse:

$ tests/qapi-schema/test-qapi.py -h
usage: test-qapi.py [-h] [-d DIR] [-u] [TEST [TEST ...]]

QAPI schema tester

positional arguments:
  TEST

optional arguments:
  -h, --help         show this help message and exit
  -d DIR, --dir DIR  directory containing tests
  -u, --update       update expected test results

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

Thanks!