Eric Blake <ebl...@redhat.com> writes: > Add some testsuite exposure for use of a 'number' as part of > an alternate. The current state of the tree has a few bugs > exposed by this: our input parser depends on the ordering of > how the qapi schema declared the alternate, and the parser > does not accept integers for a 'number' in an alternate even > though it does for numbers outside of an alternate. > > Mixing 'int' and 'number' in the same alternate is unusual, > since both are supplied by json-numbers, but there does not > seem to be a technical reason to forbid it given that our > json lexer distinguishes between json-numbers that can be > represented as an int vs. those that cannot. > > Improve the existing test_visitor_in_alternate() to match the > style of the new test_visitor_in_alternate_number(), and to > ensure full coverage of all possible qtype parsing. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v6: Rename from AltOne to AltStrBool (and so forth), drop > dead 'one = NULL' assignments, avoid leaking memory of > test data, tweak failing tests to give better hints of what > desired behavior is > --- > tests/qapi-schema/qapi-schema-test.json | 8 ++ > tests/qapi-schema/qapi-schema-test.out | 24 ++++++ > tests/test-qmp-input-visitor.c | 129 > +++++++++++++++++++++++++++++++- > 3 files changed, 158 insertions(+), 3 deletions(-) > > diff --git a/tests/qapi-schema/qapi-schema-test.json > b/tests/qapi-schema/qapi-schema-test.json > index c511338..abe59fd 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -67,6 +67,14 @@ > { 'struct': 'UserDefC', > 'data': { 'string1': 'str', 'string2': 'str' } } > > +# for testing use of 'number' within alternates > +{ 'alternate': 'AltStrBool', 'data': { 's': 'str', 'b': 'bool' } } > +{ 'alternate': 'AltStrNum', 'data': { 's': 'str', 'n': 'number' } } > +{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } } > +{ 'alternate': 'AltStrInt', 'data': { 's': 'str', 'i': 'int' } } > +{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } } > +{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } } > + > # for testing native lists > { 'union': 'UserDefNativeListUnion', > 'data': { 'integer': ['int'], > diff --git a/tests/qapi-schema/qapi-schema-test.out > b/tests/qapi-schema/qapi-schema-test.out > index 28a0b3c..8f81784 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -53,6 +53,30 @@ object :obj-user_def_cmd2-arg > object :obj-user_def_cmd3-arg > member a: int optional=False > member b: int optional=True > +alternate AltIntNum > + case i: int > + case n: number > +enum AltIntNumKind ['i', 'n'] > +alternate AltNumInt > + case n: number > + case i: int > +enum AltNumIntKind ['n', 'i'] > +alternate AltNumStr > + case n: number > + case s: str > +enum AltNumStrKind ['n', 's'] > +alternate AltStrBool > + case s: str > + case b: bool > +enum AltStrBoolKind ['s', 'b'] > +alternate AltStrInt > + case s: str > + case i: int > +enum AltStrIntKind ['s', 'i'] > +alternate AltStrNum > + case s: str > + case n: number > +enum AltStrNumKind ['s', 'n'] > event EVENT_A None > event EVENT_B None > event EVENT_C :obj-EVENT_C-arg > diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c > index 61715b3..641a3ab 100644 > --- a/tests/test-qmp-input-visitor.c > +++ b/tests/test-qmp-input-visitor.c > @@ -371,12 +371,133 @@ static void > test_visitor_in_alternate(TestInputVisitorData *data, > UserDefAlternate *tmp; > > v = visitor_input_test_init(data, "42"); > - > - visit_type_UserDefAlternate(v, &tmp, NULL, &err); > - g_assert(err == NULL); > + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); > g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I); > g_assert_cmpint(tmp->i, ==, 42); > qapi_free_UserDefAlternate(tmp); > + visitor_input_teardown(data, NULL);
Ugly in this test: visitor_input_test_init() is to be paired with visitor_input_teardown(), but each test's last visitor_input_teardown() can be omitted, because we also pass visitor_input_teardown to g_test_add(). Not your patch's fault. Let's ignore it for now. > + > + v = visitor_input_test_init(data, "'string'"); > + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); > + g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S); > + g_assert_cmpstr(tmp->s, ==, "string"); > + qapi_free_UserDefAlternate(tmp); > + visitor_input_teardown(data, NULL); > + > + v = visitor_input_test_init(data, "false"); > + visit_type_UserDefAlternate(v, &tmp, NULL, &err); > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_UserDefAlternate(tmp); > + visitor_input_teardown(data, NULL); This case tests an alternate type without a bool member rejects false. > +} > + > +static void test_visitor_in_alternate_number(TestInputVisitorData *data, > + const void *unused) > +{ > + Visitor *v; > + Error *err = NULL; > + AltStrBool *asb; > + AltStrNum *asn; > + AltNumStr *ans; > + AltStrInt *asi; > + AltIntNum *ain; > + AltNumInt *ani; > + > + /* Parsing an int */ > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltStrBool(v, &asb, NULL, &err); > + g_assert(err); > + qapi_free_AltStrBool(asb); > + visitor_input_teardown(data, NULL); This test case tests an alternate type without a numeric member (number, int, int8, ...) rejects 42. Almost identical to the case above. I wouldn't have done that one, but since you did, I'm not going demand you drop it. > + > + /* FIXME: Order of alternate should not affect semantics; asn should > + * parse the same as ans */ > + v = visitor_input_test_init(data, "42"); > + visit_type_AltStrNum(v, &asn, NULL, &err); > + /* FIXME g_assert_cmpint(asn->kind, == ALT_STR_NUM_KIND_N); */ > + /* FIXME g_assert_cmpfloat(asn->n, ==, 42); */ > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_AltStrNum(asn); > + visitor_input_teardown(data, NULL); > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltNumStr(v, &ans, NULL, &error_abort); > + g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N); > + g_assert_cmpfloat(ans->n, ==, 42); > + qapi_free_AltNumStr(ans); > + visitor_input_teardown(data, NULL); [More...] Reads easier now. Thanks! > @@ -720,6 +841,8 @@ int main(int argc, char **argv) > &in_visitor_data, test_visitor_in_alternate); > input_visitor_test_add("/visitor/input/errors", > &in_visitor_data, test_visitor_in_errors); > + input_visitor_test_add("/visitor/input/alternate-number", > + &in_visitor_data, > test_visitor_in_alternate_number); > input_visitor_test_add("/visitor/input/native_list/int", > &in_visitor_data, > test_visitor_in_native_list_int);