Re: [Qemu-devel] [PATCH v2 1/9] tests: QAPI schema parser tests

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 07/27/13 17:41, Markus Armbruster wrote:
>> The parser handles erroneous input badly.  To be improved shortly.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>
>> diff --git a/tests/Makefile b/tests/Makefile
>> index cdbb79e..ddb957c 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>
>> @@ -233,13 +242,24 @@ check-report.html: check-report.xml
>>  check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh 
>> qemu-img$(EXESUF) qemu-io$(EXESUF)
>>      $<
>>  
>> +.PHONY: check-tests/test-qapi.py
>> +check-tests/test-qapi.py: tests/test-qapi.py
>> +
>> +.PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
>> +$(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: 
>> $(SRC_PATH)/%.json
>> +    $(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts $(PYTHON) 
>> $(SRC_PATH)/tests/qapi-schema/test-qapi.py <$^ >$*.out 2>$*.err; echo $$? 
>> >$*.exit, "  TEST  $*.out")
>> +    @diff -q $(SRC_PATH)/$*.out $*.out
>> +    @diff -q $(SRC_PATH)/$*.err $*.err
>> +    @diff -q $(SRC_PATH)/$*.exit $*.exit
>> +
>
> Unfortunately, this doesn't catch errors/differences when someone builds
> qemu in the source tree, and runs "make check" there. In that case,
> whatever output "test-qapi.py" produces, overwrites the in-tree file,
> and the diff commands compare files with themselves.

D'uh!  Will fix.  Thanks!

> Also, why is 2/9 a good idea, namely, using "qapi-schema-test.json" as a
> test file itself? Whoever adds a new test, for anything that uses the
> schema, now has to update the .out file as well.

The parser needs to be tested with input that exercises all schema
features.  Since such a test already existed, I extended it to
additionally test the parser.

If this coupling turns out to be inconvenient, we can split the test in
two: qapi-schema-test.json goes back to just its original purpose, and a
(possibly simplified) copy is used for testing the parser.

Drawback: when you add schema features, you have to update both tests to
cover them.  Neglecting to add any tests is obvious in review.
Forgetting one of two not so much.

>                                                  (And, due to the above,
> only realizes this burden when someone else tries to make-check his/her
> code outside of the tree...)

Anyone posting patches without running "make check" gets exactly what he
asked for: embarrassment :)