Re: [Qemu-devel] [PATCH v3 24/50] qapi: add some struct member tests

[Marc-André Lureau]

Hi

On Sat, Dec 9, 2017 at 10:07 AM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  tests/Makefile.include                    |  2 ++
>>  tests/qapi-schema/struct-if-invalid.err   |  1 +
>>  tests/qapi-schema/struct-if-invalid.exit  |  1 +
>>  tests/qapi-schema/struct-if-invalid.json  |  3 +++
>>  tests/qapi-schema/struct-if-invalid.out   |  0
>>  tests/qapi-schema/struct-member-type.err  |  0
>>  tests/qapi-schema/struct-member-type.exit |  1 +
>>  tests/qapi-schema/struct-member-type.json |  2 ++
>>  tests/qapi-schema/struct-member-type.out  | 12 ++++++++++++
>>  9 files changed, 22 insertions(+)
>>  create mode 100644 tests/qapi-schema/struct-if-invalid.err
>>  create mode 100644 tests/qapi-schema/struct-if-invalid.exit
>>  create mode 100644 tests/qapi-schema/struct-if-invalid.json
>>  create mode 100644 tests/qapi-schema/struct-if-invalid.out
>>  create mode 100644 tests/qapi-schema/struct-member-type.err
>>  create mode 100644 tests/qapi-schema/struct-member-type.exit
>>  create mode 100644 tests/qapi-schema/struct-member-type.json
>>  create mode 100644 tests/qapi-schema/struct-member-type.out
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 0aa532f029..44a3d8895e 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -520,7 +520,9 @@ qapi-schema += returns-whitelist.json
>>  qapi-schema += struct-base-clash-deep.json
>>  qapi-schema += struct-base-clash.json
>>  qapi-schema += struct-data-invalid.json
>> +qapi-schema += struct-if-invalid.json
>>  qapi-schema += struct-member-invalid.json
>> +qapi-schema += struct-member-type.json
>>  qapi-schema += trailing-comma-list.json
>>  qapi-schema += trailing-comma-object.json
>>  qapi-schema += type-bypass-bad-gen.json
>> diff --git a/tests/qapi-schema/struct-if-invalid.err 
>> b/tests/qapi-schema/struct-if-invalid.err
>> new file mode 100644
>> index 0000000000..42245262a9
>> --- /dev/null
>> +++ b/tests/qapi-schema/struct-if-invalid.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/struct-if-invalid.json:2: Member 'bar' of 'data' for 
>> struct 'TestIfStruct' should be a type name
>> diff --git a/tests/qapi-schema/struct-if-invalid.exit 
>> b/tests/qapi-schema/struct-if-invalid.exit
>> new file mode 100644
>> index 0000000000..d00491fd7e
>> --- /dev/null
>> +++ b/tests/qapi-schema/struct-if-invalid.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/struct-if-invalid.json 
>> b/tests/qapi-schema/struct-if-invalid.json
>> new file mode 100644
>> index 0000000000..466cdb61e1
>> --- /dev/null
>> +++ b/tests/qapi-schema/struct-if-invalid.json
>> @@ -0,0 +1,3 @@
>> +# check missing 'type'
>> +{ 'struct': 'TestIfStruct', 'data':
>> +  { 'foo': 'int', 'bar': { 'if': 'defined(TEST_IF_STRUCT_BAR)' } } }
>
> Hmm.  This tests the previous patch's change to check_type().  It
> demonstrates that the "should be a type name" error message can be
> suboptimal: it gets triggered when
>
>     not isinstance(value, str)
>     and not (isinstance(value, dict) and 'type' in value)
>
> Fine when the value is neither str not dict.  Suboptimal when it's dict
> and 'type' is missing.  A more obvious example of suboptimality would be
>
>    'bar': { 'tvpe': 'int' }
>
> The previous patch's
>
>         if isinstance(value, dict) and 'type' in value:
>             check_type(info, source, value['type'], allow_array,
>                        allow_dict, allow_optional, allow_metas)
>             if 'if' in value:
>                 check_if(value, info)
>             check_unknown_keys(info, value, {'type', 'if'})
>             return
>         else:
>             raise QAPISemError(info, "%s should be a type name" % source)
>
> should be improved to something like
>
>         if not isinstance(value, dict):
>             raise QAPISemError(
>                 info, "%s should be a type name or dictionary" % source)
>         if 'type' not in value:
>             raise QAPISemError(
>                 info, "%s must have a member 'type'" % source)
>         check_type(info, source, value['type'], allow_array,
>                    allow_dict, allow_optional, allow_metas)
>         if 'if' in value:
>             check_if(value, info)
>         check_unknown_keys(info, value, {'type', 'if'})
>         return
>
> producing
>
>     struct-if-invalid.json:2: Member 'bar' of 'data' for struct 
> 'TestIfStruct' must have a member 'type'
>

Fixed differently in v4

> The fact that I missed this in review of the code, but noticed it in the
> tests is why I want tests added together with the code they test.
>

Changed in v4

> Nitpick: the file name struct-if-invalid makes me expect an invalid if.
> Not the case.  It's a missing type.  Let's rename accordingly.

Done

>
>> diff --git a/tests/qapi-schema/struct-if-invalid.out 
>> b/tests/qapi-schema/struct-if-invalid.out
>> new file mode 100644
>> index 0000000000..e69de29bb2
>> diff --git a/tests/qapi-schema/struct-member-type.err 
>> b/tests/qapi-schema/struct-member-type.err
>> new file mode 100644
>> index 0000000000..e69de29bb2
>> diff --git a/tests/qapi-schema/struct-member-type.exit 
>> b/tests/qapi-schema/struct-member-type.exit
>> new file mode 100644
>> index 0000000000..573541ac97
>> --- /dev/null
>> +++ b/tests/qapi-schema/struct-member-type.exit
>> @@ -0,0 +1 @@
>> +0
>> diff --git a/tests/qapi-schema/struct-member-type.json 
>> b/tests/qapi-schema/struct-member-type.json
>> new file mode 100644
>> index 0000000000..8b33027817
>> --- /dev/null
>> +++ b/tests/qapi-schema/struct-member-type.json
>> @@ -0,0 +1,2 @@
>> +# check member 'a' with 'type' key only
>> +{ 'struct': 'foo', 'data': { 'a': { 'type': 'str' } } }
>> diff --git a/tests/qapi-schema/struct-member-type.out 
>> b/tests/qapi-schema/struct-member-type.out
>> new file mode 100644
>> index 0000000000..04b969d2e3
>> --- /dev/null
>> +++ b/tests/qapi-schema/struct-member-type.out
>> @@ -0,0 +1,12 @@
>> +enum QType
>> +    prefix QTYPE
>> +    member none:
>> +    member qnull:
>> +    member qnum:
>> +    member qstring:
>> +    member qdict:
>> +    member qlist:
>> +    member qbool:
>> +object foo
>> +    member a: str optional=False
>> +object q_empty
>
> This is a positive test, isn't it?  Positive tests go into
> qapi-schema-test.json.
>

Right, I wonder why we have .exit files then. Perhaps the few ones
that return 0 shouldn't exist.

thanks

-- 
Marc-André Lureau