Eric Blake <ebl...@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 <arm...@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 <ebl...@redhat.com>
Thanks!
