| Message ID | 20180509165530.29561-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On 2018-05-10 16:08, Eric Blake wrote: > 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). Oh, sure. >> 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. OK, but I'd like to rename the test, then. Max
diff --git a/tests/Makefile.include b/tests/Makefile.include index 3b9a5e31a2..d01df12b09 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -500,7 +500,10 @@ qapi-schema += flat-union-invalid-branch-key.json qapi-schema += flat-union-invalid-discriminator.json qapi-schema += flat-union-no-base.json qapi-schema += flat-union-optional-discriminator.json +qapi-schema += flat-union-optional-discriminator-no-default.json +qapi-schema += flat-union-optional-discriminator-invalid-default.json qapi-schema += flat-union-string-discriminator.json +qapi-schema += flat-union-superfluous-default-variant.json qapi-schema += funny-char.json qapi-schema += ident-with-escape.json qapi-schema += include-before-err.json diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json new file mode 100644 index 0000000000..015a47ba52 --- /dev/null +++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json @@ -0,0 +1,12 @@ +# default-variant must refer to an actual value of the discriminator's +# enum +{ 'enum': 'Enum', 'data': [ 'one', 'two' ] } +{ 'struct': 'Base', + 'data': { '*switch': 'Enum' } } +{ 'struct': 'Branch', 'data': { 'name': 'str' } } +{ 'union': 'MyUnion', + 'base': 'Base', + 'discriminator': 'switch', + 'default-variant': 'three', + 'data': { 'one': 'Branch', + 'two': 'Branch' } } diff --git a/tests/qapi-schema/flat-union-optional-discriminator-no-default.json b/tests/qapi-schema/flat-union-optional-discriminator-no-default.json new file mode 100644 index 0000000000..0d612f5a62 --- /dev/null +++ b/tests/qapi-schema/flat-union-optional-discriminator-no-default.json @@ -0,0 +1,11 @@ +# Using an optional discriminator requires specifying a default +# variant +{ 'enum': 'Enum', 'data': [ 'one', 'two' ] } +{ 'struct': 'Base', + 'data': { '*switch': 'Enum' } } +{ 'struct': 'Branch', 'data': { 'name': 'str' } } +{ 'union': 'MyUnion', + 'base': 'Base', + 'discriminator': 'switch', + 'data': { 'one': 'Branch', + 'two': 'Branch' } } diff --git a/tests/qapi-schema/flat-union-optional-discriminator.json b/tests/qapi-schema/flat-union-optional-discriminator.json index 08a8f7ef8b..68e87083a1 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.json +++ b/tests/qapi-schema/flat-union-optional-discriminator.json @@ -1,10 +1,10 @@ -# we require the discriminator to be non-optional { 'enum': 'Enum', 'data': [ 'one', 'two' ] } { 'struct': 'Base', 'data': { '*switch': 'Enum' } } { 'struct': 'Branch', 'data': { 'name': 'str' } } { 'union': 'MyUnion', 'base': 'Base', - 'discriminator': '*switch', + 'discriminator': 'switch', + 'default-variant': 'one', 'data': { 'one': 'Branch', 'two': 'Branch' } } diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.json b/tests/qapi-schema/flat-union-superfluous-default-variant.json new file mode 100644 index 0000000000..8558165868 --- /dev/null +++ b/tests/qapi-schema/flat-union-superfluous-default-variant.json @@ -0,0 +1,11 @@ +# default-variant only makes sense with an optional discriminator +{ 'enum': 'Enum', 'data': [ 'one', 'two' ] } +{ 'struct': 'Base', + 'data': { 'switch': 'Enum' } } +{ 'struct': 'Branch', 'data': { 'name': 'str' } } +{ 'union': 'MyUnion', + 'base': 'Base', + 'discriminator': 'switch', + 'default-variant': 'one', + 'data': { 'one': 'Branch', + 'two': 'Branch' } } diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err new file mode 100644 index 0000000000..b6bd3423d6 --- /dev/null +++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-optional-discriminator-invalid-default.json:7: Default variant 'three' of flat union 'MyUnion' is not part of 'Enum' diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.exit b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.out b/tests/qapi-schema/flat-union-optional-discriminator-invalid-default.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/flat-union-optional-discriminator-no-default.err b/tests/qapi-schema/flat-union-optional-discriminator-no-default.err new file mode 100644 index 0000000000..1342efd9e8 --- /dev/null +++ b/tests/qapi-schema/flat-union-optional-discriminator-no-default.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-optional-discriminator-no-default.json:7: Default variant must be specified for optional discriminator 'switch' diff --git a/tests/qapi-schema/flat-union-optional-discriminator-no-default.exit b/tests/qapi-schema/flat-union-optional-discriminator-no-default.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/flat-union-optional-discriminator-no-default.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/flat-union-optional-discriminator-no-default.out b/tests/qapi-schema/flat-union-optional-discriminator-no-default.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/flat-union-optional-discriminator.err b/tests/qapi-schema/flat-union-optional-discriminator.err index aaabedb3bd..e69de29bb2 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.err +++ 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' diff --git a/tests/qapi-schema/flat-union-optional-discriminator.exit b/tests/qapi-schema/flat-union-optional-discriminator.exit index d00491fd7e..573541ac97 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.exit +++ b/tests/qapi-schema/flat-union-optional-discriminator.exit @@ -1 +1 @@ -1 +0 diff --git a/tests/qapi-schema/flat-union-optional-discriminator.out b/tests/qapi-schema/flat-union-optional-discriminator.out index e69de29bb2..d3a72f63c2 100644 --- a/tests/qapi-schema/flat-union-optional-discriminator.out +++ b/tests/qapi-schema/flat-union-optional-discriminator.out @@ -0,0 +1,15 @@ +object q_empty +enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool'] + prefix QTYPE +module flat-union-optional-discriminator.json +enum Enum ['one', 'two'] +object Base + member switch: Enum optional=True +object Branch + member name: str optional=False +object MyUnion + base Base + tag switch + default variant: one + case one: Branch + case two: Branch diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.err b/tests/qapi-schema/flat-union-superfluous-default-variant.err new file mode 100644 index 0000000000..5230bbf645 --- /dev/null +++ b/tests/qapi-schema/flat-union-superfluous-default-variant.err @@ -0,0 +1 @@ +tests/qapi-schema/flat-union-superfluous-default-variant.json:6: Must not specify a default variant for non-optional discriminator 'switch' diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.exit b/tests/qapi-schema/flat-union-superfluous-default-variant.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/flat-union-superfluous-default-variant.exit @@ -0,0 +1 @@ +1
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. Besides this one, a number of tests adding various error cases are added. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/Makefile.include | 3 +++ ...flat-union-optional-discriminator-invalid-default.json | 12 ++++++++++++ .../flat-union-optional-discriminator-no-default.json | 11 +++++++++++ tests/qapi-schema/flat-union-optional-discriminator.json | 4 ++-- .../flat-union-superfluous-default-variant.json | 11 +++++++++++ .../flat-union-optional-discriminator-invalid-default.err | 1 + ...flat-union-optional-discriminator-invalid-default.exit | 1 + .../flat-union-optional-discriminator-invalid-default.out | 0 .../flat-union-optional-discriminator-no-default.err | 1 + .../flat-union-optional-discriminator-no-default.exit | 1 + .../flat-union-optional-discriminator-no-default.out | 0 tests/qapi-schema/flat-union-optional-discriminator.err | 1 - tests/qapi-schema/flat-union-optional-discriminator.exit | 2 +- tests/qapi-schema/flat-union-optional-discriminator.out | 15 +++++++++++++++ .../flat-union-superfluous-default-variant.err | 1 + .../flat-union-superfluous-default-variant.exit | 1 + .../flat-union-superfluous-default-variant.out | 0 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 diff --git a/tests/qapi-schema/flat-union-superfluous-default-variant.out b/tests/qapi-schema/flat-union-superfluous-default-variant.out new file mode 100644 index 0000000000..e69de29bb2