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/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
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
--
2.14.3
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
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
© 2016 - 2025 Red Hat, Inc.