Re: [Qemu-devel] [PATCH RFC v2 24/47] tests/qapi-schema: Convert test harness to QAPISchemaVisitor

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> The old code prints the result of parsing (list of expression
>> dictionaries), and partial results of semantic analysis (list of enum
>> dictionaries, list of struct dictionaries).
>> 
>> The new code prints a trace of a schema visit, i.e. what the back-ends
>> are going to use.  Built-in and array types are omitted, because
>> they're boring.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/qapi-schema/alternate-good.out            |  15 +-
>>  tests/qapi-schema/comments.out                  |   4 +-
>>  tests/qapi-schema/data-member-array.out         |  13 +-
>>  tests/qapi-schema/empty.out                     |   3 -
>>  tests/qapi-schema/enum-empty.out                |   4 +-
>>  tests/qapi-schema/event-case.out                |   4 +-
>>  tests/qapi-schema/flat-union-reverse-define.out |  21 +--
>>  tests/qapi-schema/ident-with-escape.out         |   7 +-
>>  tests/qapi-schema/include-relpath.out           |   4 +-
>>  tests/qapi-schema/include-repetition.out        |   4 +-
>>  tests/qapi-schema/include-simple.out            |   4 +-
>>  tests/qapi-schema/indented-expr.out             |   7 +-
>>  tests/qapi-schema/qapi-schema-test.out | 186
>> +++++++++++++++++-------
>>  tests/qapi-schema/returns-int.out               |   5 +-
>>  tests/qapi-schema/test-qapi.py                  |  37 ++++-
>>  tests/qapi-schema/type-bypass.out               |   7 +-
>>  16 files changed, 210 insertions(+), 115 deletions(-)
>
> We have a lot more negative than positive tests of the parser (good
> thing, because that meant fewer .out files to update to the new format).
>
> No change to actual qemu code, and proves that the previous three
> patches have set up enough of a framework to accurately cover our testsuite.
>
>> 
>> diff --git a/tests/qapi-schema/alternate-good.out
>> b/tests/qapi-schema/alternate-good.out
>> index 99848ee..0cbdfa1 100644
>> --- a/tests/qapi-schema/alternate-good.out
>> +++ b/tests/qapi-schema/alternate-good.out
>> @@ -1,6 +1,9 @@
>> -[OrderedDict([('struct', 'Data'), ('data', OrderedDict([('*number',
>> 'int'), ('*name', 'str')]))]),
>> - OrderedDict([('enum', 'Enum'), ('data', ['hello', 'world'])]),
>> - OrderedDict([('alternate', 'Alt'), ('data', OrderedDict([('value',
>> 'int'), ('string', 'Enum'), ('struct', 'Data')]))])]
>> -[{'enum_name': 'Enum', 'enum_values': ['hello', 'world']},
>> - {'enum_name': 'AltKind', 'enum_values': None}]
>> -[OrderedDict([('struct', 'Data'), ('data', OrderedDict([('*number',
>> 'int'), ('*name', 'str')]))])]
>> +alternate Alt
>> +    case value: int flat=False
>> +    case string: Enum flat=False
>> +    case struct: Data flat=False
>
> I'm still not convinced whether we need .flat exposed through this much
> detail, or if we should just normalize plain unions into flat unions
> with implicit structs for each branch.  Changing your design will have
> obvious ripple effects here.

Yes.  I think it's okay as long as we keep it out of external
interfaces.

>> +++ b/tests/qapi-schema/data-member-array.out
>> @@ -1,5 +1,8 @@
>> -[OrderedDict([('enum', 'abc'), ('data', ['a', 'b', 'c'])]),
>> - OrderedDict([('struct', 'def'), ('data', OrderedDict([('array',
>> ['abc'])]))]),
>> - OrderedDict([('command', 'okay'), ('data',
>> OrderedDict([('member1', ['int']), ('member2', ['def'])]))])]
>> -[{'enum_name': 'abc', 'enum_values': ['a', 'b', 'c']}]
>> -[OrderedDict([('struct', 'def'), ('data', OrderedDict([('array',
>> ['abc'])]))])]
>> +object :obj-okay-args
>> +    member member1: intList optional=False
>
> Took me a moment to realize the object is an implicit one (named
> ':obj-okay-args') and not a typo for 'object: obj-okay-args' consistent
> with members being listed 'name: type'.  But not worth changing things,
> as it is sufficiently unambiguous to serve as a valid test.

Perhaps omitting the ':' after member names would be less confusing.

>> +object UserDefFlatUnion
>> +    base UserDefUnionBase
>> +    tag enum1
>> +    case value1: UserDefA flat=True
>> +    case value2: UserDefB flat=True
>> +    case value3: UserDefB flat=True
>> +object UserDefFlatUnion2
>> +    base UserDefUnionBase
>> +    tag enum1
>> +    case value1: UserDefC flat=True
>> +    case value2: UserDefB flat=True
>> +    case value3: UserDefA flat=True
>> +object UserDefNativeListUnion
>> +    case integer: intList flat=False
>> +    case s8: int8List flat=False
>> +    case s16: int16List flat=False
>> +    case s32: int32List flat=False
>> +    case s64: int64List flat=False
>> +    case u8: uint8List flat=False
>> +    case u16: uint16List flat=False
>> +    case u32: uint32List flat=False
>> +    case u64: uint64List flat=False
>> +    case number: numberList flat=False
>> +    case boolean: boolList flat=False
>> +    case string: strList flat=False
>> +    case sizes: sizeList flat=False
>> +enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32',
>> 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string',
>> 'sizes']
>
> Hmm. You are dumping the tag name and type of flat unions, but not of
> simple unions.  I would have expected:
>
> object UserDefNativeListUnion
>     member kind: UserDefNativeListUnionKind
>     tag kind
>     case integer: intList flat=False
> ...

See below.

> The above was fallout, while below is the meat of the new visitor.
>
>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -15,11 +15,34 @@ from pprint import pprint
>>  import os
>>  import sys
>>  
>> -try:
>> -    exprs = QAPISchema(sys.argv[1]).get_exprs()
>> -except SystemExit:
>> -    raise
>> +class QAPISchemaTestVisitor(QAPISchemaVisitor):
>> +    def visit_enum_type(self, name, info, values):
>> +        print 'enum %s %s' % (name, values)
>> +    def visit_object_type(self, name, info, base, members, variants):
>> +        print 'object %s' % name
>> +        if base:
>> +            print '    base %s' % base.name
>> +        for m in members:
>> +            print '    member %s: %s optional=%s' % (m.name, m.type.name,
>> +                                                     m.optional)
>> +        self._print_variants(variants)
>
> So here is where information was missing when visiting a simple union.
> Why didn't 'kind' get injected into members?  And...

Because members are the explicit members.  A simple union's implicit tag
member is in QAPISchemaObjectTypeVariants.  See further below.

>> +    def visit_alternate_type(self, name, info, variants):
>> +        print 'alternate %s' % name
>> +        self._print_variants(variants)
>> +    def visit_command(self, name, info, args, rets, gen, success_response):
>> +        print 'command %s %s -> %s' % (name, (args and args.name),
>> +                                       (rets and rets.name))
>> +        print '   gen=%s success_response=%s' % (gen, success_response)
>> +    def visit_event(self, name, info, data):
>> +        print 'event %s %s' % (name, data and data.name)
>>  
>> -pprint(exprs)
>> -pprint(enum_types)
>> -pprint(struct_types)
>> +    @staticmethod
>> +    def _print_variants(variants):
>> +        if variants:
>> +            if variants.tag_name:
>> +                print '    tag %s' % variants.tag_name
>
> ...shouldn't a simple union report 'kind' as its tag_name?

It could.  Here's why it currently isn't.

QAPISchemaObjectType has members .base and .local_members, which come
straight from the type definition.

These get flattened into .members.

We do create a simple union's implicit member 'kind', and store it in
.variants.tag_member.  But we don't add it to .local_members, only to
.members, similar to how .base's members are only in .members.

We don't print .members here.  Instead, we print .base, .local_members
and .variants.  The part printing .variants doesn't print
.variants.tag_member.

We need to draw the line on what to print *somewhere*.  Where exactly is
of course debatable.  My working idea on what to print here is "print
everything that isn't derived from other stuff".

>> +            for v in variants.variants:
>> + print ' case %s: %s flat=%s' % (v.name, v.type.name, v.flat)
>> +
>> +schema = QAPISchema(sys.argv[1])
>> +schema.visit(QAPISchemaTestVisitor())
>> diff --git a/tests/qapi-schema/type-bypass.out
>> b/tests/qapi-schema/type-bypass.out
>
> Overall, fairly slick, but I have enough questions about the
> representation of a simple union that I'll wait for the non-RFC respin
> to see if you change any design.

Thanks!