Re: [Qemu-devel] [PATCH v5 19/28] qapi: Add some type check tests

[Eric Blake]

On 03/26/2015 11:58 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Demonstrate that the qapi generator silently parses confusing
>> types, which may cause other errors later on. Later patches
>> will update the expected results as the generator is made stricter.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
> [...]
>> diff --git a/tests/qapi-schema/data-array-empty.err 
>> b/tests/qapi-schema/data-array-empty.err
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/data-array-empty.exit 
>> b/tests/qapi-schema/data-array-empty.exit
>> new file mode 100644
>> index 0000000..573541a
>> --- /dev/null
>> +++ b/tests/qapi-schema/data-array-empty.exit
>> @@ -0,0 +1 @@
>> +0
>> diff --git a/tests/qapi-schema/data-array-empty.json 
>> b/tests/qapi-schema/data-array-empty.json
>> new file mode 100644
>> index 0000000..edb543a
>> --- /dev/null
>> +++ b/tests/qapi-schema/data-array-empty.json
>> @@ -0,0 +1,2 @@
>> +# FIXME: we should reject an array for data if it does not contain a known 
>> type
>> +{ 'command': 'oops', 'data': { 'empty': [ ] } }
> 
> v4 tested
> 
>     { 'command': 'oops', 'data': [ ] }
> 
> Is that still covered somewhere else?

Probably not.  Sounds like a good test case to add, presumably on top if
I don't have to respin.


>> +
>> +{ 'command': 'no-way-this-will-get-whitelisted',
>> +  'returns': [ 'int' ] }
> 
> I like the pattern "add tests to demonstrate issues, then code to
> address the issues".  But this test appears too much out of the blue for
> my taste.
> 
> You could use the commit message to prepare the reader.
> 
> Or you could deviate from the pattern and add this test together with
> the actual whitelist.  That's what I'd try.

Of course, if I do need to respin, I can shuffle and split patches as
needed to make the resubmission prettier.

> 
>> diff --git a/tests/qapi-schema/returns-whitelist.out 
>> b/tests/qapi-schema/returns-whitelist.out
>> new file mode 100644
>> index 0000000..2adcd8b
>> --- /dev/null
>> +++ b/tests/qapi-schema/returns-whitelist.out
>> @@ -0,0 +1,7 @@
>> +[OrderedDict([('command', 'human-monitor-command'), ('data', 
>> OrderedDict([('command-line', 'str'), ('*cpu-index', 'int')])), ('returns', 
>> 'str')]),
>> + OrderedDict([('enum', 'TpmModel'), ('data', ['tpm-tis'])]),
>> + OrderedDict([('command', 'query-tpm-models'), ('returns', ['TpmModel'])]),
>> + OrderedDict([('command', 'guest-get-time'), ('returns', 'int')]),
>> + OrderedDict([('command', 'no-way-this-will-get-whitelisted'), ('returns', 
>> ['int'])])]
>> +[{'enum_name': 'TpmModel', 'enum_values': ['tpm-tis']}]
>> +[]
> 
> Since I found nothing that's actually wrong:
> 
> Reviewed-by: Markus Armbruster <address@hidden>
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature