Hi
On Sat, Dec 9, 2017 at 10:07 AM, Markus Armbruster <arm...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> ---
>> 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
