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

[Eric Blake]

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