Re: [Qemu-devel] [PATCH 03/13] tests: Add QAPI optional discriminator tests

[Eric Blake <eblake@redhat.com>]

On 05/09/2018 11:55 AM, Max Reitz wrote:
> There already is an optional discriminator test, although it also noted
> the discriminator name itself as optional.  Changing that, and adding a
> default-variant field, makes that test pass instead of failing.

I'm not sure I agree with that one.  I think that you should instead add 
a positive test of a default variant into qapi-schema-test.json, 
especially since that is the one positive test where we also ensure that 
the C compiler is happy with the generated code (other positive tests 
prove that the generator produced something without error, but not that 
what it produced could be compiled).

>   17 files changed, 61 insertions(+), 4 deletions(-)
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.json
>   create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.json
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.exit
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-invalid-default.out
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.err
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.exit
>   create mode 100644 tests/qapi-schema/flat-union-optional-discriminator-no-default.out
>   create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.err
>   create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.exit
>   create mode 100644 tests/qapi-schema/flat-union-superfluous-default-variant.out

Patch 1 converted 2 error messages into 6, where 2 of them look identical:

>> -        # The value of member 'discriminator' must name a non-optional
>> -        # member of the base struct.
>> +        # The value of member 'discriminator' must name a member of
>> +        # the base struct.
>>          check_name(info, "Discriminator of flat union '%s'" % name,
>>                     discriminator)

[0] check_name() checks that 'discriminator':'*name' is rejected - this 
check is unchanged

>> -        discriminator_type = base_members.get(discriminator)
>> -        if not discriminator_type:
>> -            raise QAPISemError(info,
>> -                               "Discriminator '%s' is not a member of base "
>> -                               "struct '%s'"
>> -                               % (discriminator, base))

The old code ensured that 'discriminator':'name' finds 'name' as a 
mandatory field in the base type; which changed into:

>> +        if default_variant is None:
>> +            discriminator_type = base_members.get(discriminator)
>> +            if not discriminator_type:
>> +                if base_members.get('*' + discriminator) is None:
>> +                    raise QAPISemError(info,
>> +                                       "Discriminator '%s' is not a member of "
>> +                                       "base struct '%s'"
>> +                                       % (discriminator, base))

[1] the discriminator type must be a member field (we didn't find either 
a mandatory or an optional field)

>> +                else:
>> +                    raise QAPISemError(info,
>> +                                       "Default variant must be specified for "
>> +                                       "optional discriminator '%s'"
>> +                                       % discriminator)

[2] without default_variant, the member field must be mandatory

>> +        else:
>> +            discriminator_type = base_members.get('*' + discriminator)
>> +            if not discriminator_type:
>> +                if base_members.get(discriminator) is None:
>> +                    raise QAPISemError(info,
>> +                                       "Discriminator '%s' is not a member of "
>> +                                       "base struct '%s'"
>> +                                       % (discriminator, base))

[3] the discriminator type must be a member field (we didn't find either 
a mandatory or an optional field), identical message to [1]

>> +                else:
>> +                    raise QAPISemError(info,
>> +                                       "Must not specify a default variant for "
>> +                                       "non-optional discriminator '%s'"
>> +                                       % discriminator)

[4] with default_variant, the member field must be optional

>> +
>>          enum_define = enum_types.get(discriminator_type)
>>          allow_metas = ['struct']
>>          # Do not allow string discriminator
>> @@ -763,6 +785,15 @@ def check_union(expr, info):
>>                                 "Discriminator '%s' must be of enumeration "
>>                                 "type" % discriminator)
>>  
>> +        if default_variant is not None:
>> +            # Must be a value of the enumeration
>> +            if default_variant not in enum_define['data']:
>> +                raise QAPISemError(info,
>> +                                   "Default variant '%s' of flat union '%s' is "
>> +                                   "not part of '%s'"
>> +                                   % (default_variant, name,
>> +                                      discriminator_type))
>> +

[5] the default discriminator value must belong to the discriminator's type

Of those 6 messages, you now have coverage of:

> 
> +++ b/tests/Makefile.include
> @@ -500,7 +500,10 @@ qapi-schema += flat-union-invalid-branch-key.json
>  qapi-schema += flat-union-invalid-discriminator.json

[1] no change to that test

>  qapi-schema += flat-union-no-base.json
>  qapi-schema += flat-union-optional-discriminator.json

the old test for [0] - you turned it into success. See more below...

> +qapi-schema += flat-union-optional-discriminator-no-default.json

new test for [2]

> +qapi-schema += flat-union-optional-discriminator-invalid-default.json

new test for [5]

>  qapi-schema += flat-union-string-discriminator.json
> +qapi-schema += flat-union-superfluous-default-variant.json

new test for [4]

I don't know if it is worth trying to explicitly test for [3], given 
that it is identical to [1], but the loss of the test for [0] bothers me.

> +++ b/tests/qapi-schema/flat-union-optional-discriminator.err
> @@ -1 +0,0 @@
> -tests/qapi-schema/flat-union-optional-discriminator.json:6: Discriminator of flat union 'MyUnion' does not allow optional name '*switch'

This used to be covered by [0] - but you rewrote the test by changing 
'discriminator':'*switch' to 'discriminator':'switch'.

I'd argue that you WANT to keep 'discriminator':'*switch' in this test, 
to keep the testing of error [0] (even when the discriminator type is 
optional, we spell it without leading '*' in the 'union' description), 
and then only qapi-schema-test.json gets a positive test, which also 
gives us the additional coverage that the C compiler likes our generated 
code.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org