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. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > 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 c904db4..7593ecb 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -65,6 +65,14 @@ > { 'struct': 'UserDefC', > 'data': { 'string1': 'str', 'string2': 'str' } } > > +# for testing use of 'number' within alternates > +{ 'alternate': 'AltOne', 'data': { 's': 'str', 'b': 'bool' } } > +{ 'alternate': 'AltTwo', 'data': { 's': 'str', 'n': 'number' } } > +{ 'alternate': 'AltThree', 'data': { 'n': 'number', 's': 'str' } } > +{ 'alternate': 'AltFour', 'data': { 's': 'str', 'i': 'int' } } > +{ 'alternate': 'AltFive', 'data': { 'i': 'int', 'n': 'number' } } > +{ 'alternate': 'AltSix', '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..de29a45 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 AltFive > + case i: int > + case n: number > +enum AltFiveKind ['i', 'n'] > +alternate AltFour > + case s: str > + case i: int > +enum AltFourKind ['s', 'i'] > +alternate AltOne > + case s: str > + case b: bool > +enum AltOneKind ['s', 'b'] > +alternate AltSix > + case n: number > + case i: int > +enum AltSixKind ['n', 'i'] > +alternate AltThree > + case n: number > + case s: str > +enum AltThreeKind ['n', 's'] > +alternate AltTwo > + case s: str > + case n: number > +enum AltTwoKind ['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..cd41847 100644 > --- a/tests/test-qmp-input-visitor.c > +++ b/tests/test-qmp-input-visitor.c > @@ -368,15 +368,136 @@ static void > test_visitor_in_alternate(TestInputVisitorData *data, > { > Visitor *v; > Error *err = NULL; > - UserDefAlternate *tmp; > + UserDefAlternate *tmp = NULL;
Any particular reason for adding the initializer? > > 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); The pattern foo(..., &err); g_assert(err == NULL); is pretty common in tests. Can't see what it buys us over straight &error_abort. Perhaps I'll spatch it away. > g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I); > g_assert_cmpint(tmp->i, ==, 42); > qapi_free_UserDefAlternate(tmp); > + tmp = NULL; Why do you need to clear tmp? > + > + 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); > + tmp = 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); Okay, here you merely make the test of existing UserDefAlternate more thorough. Could be a separate patch, but keeping it in this one is fine with me, too. > +} > + > +static void test_visitor_in_alternate_number(TestInputVisitorData *data, > + const void *unused) > +{ > + Visitor *v; > + Error *err = NULL; > + AltOne *one = NULL; > + AltTwo *two = NULL; > + AltThree *three = NULL; > + AltFour *four = NULL; > + AltFive *five = NULL; > + AltSix *six = NULL; > + > + /* Parsing an int */ > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltOne(v, &one, NULL, &err); > + g_assert(err); > + qapi_free_AltOne(one); > + one = NULL; Similar to the new last test in test_visitor_in_alternate(). Not sure that one's needed. > + > + /* FIXME: Integers should parse as numbers */ Suggest to augment or replace this comment... > + v = visitor_input_test_init(data, "42"); > + visit_type_AltTwo(v, &two, NULL, &err); ... with /* FIXME g_assert_cmpint(two->kind, ==, ALT_TWO_KIND_N); */ /* FIXME g_assert_cmpfloat(two->n, ==, 42); */ > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_AltTwo(two); > + one = NULL; *chuckle* Why do you clear one here? More of the same below. > + > + /* FIXME: Order of alternate should not affect semantics */ Inhowfar does it affect semantics? Or asked differently: what exactly is wrong with this test now? > + v = visitor_input_test_init(data, "42"); > + visit_type_AltThree(v, &three, NULL, &error_abort); > + g_assert_cmpint(three->kind, ==, ALT_THREE_KIND_N); > + g_assert_cmpfloat(three->n, ==, 42); > + qapi_free_AltThree(three); > + one = NULL; > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltFour(v, &four, NULL, &error_abort); > + g_assert_cmpint(four->kind, ==, ALT_FOUR_KIND_I); > + g_assert_cmpint(four->i, ==, 42); > + qapi_free_AltFour(four); > + one = NULL; > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltFive(v, &five, NULL, &error_abort); > + g_assert_cmpint(five->kind, ==, ALT_FIVE_KIND_I); > + g_assert_cmpint(five->i, ==, 42); > + qapi_free_AltFive(five); > + one = NULL; > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltSix(v, &six, NULL, &error_abort); > + g_assert_cmpint(six->kind, ==, ALT_SIX_KIND_I); > + g_assert_cmpint(six->i, ==, 42); > + qapi_free_AltSix(six); > + one = NULL; > + > + /* Parsing a double */ > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltOne(v, &one, NULL, &err); > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_AltOne(one); > + one = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltTwo(v, &two, NULL, &error_abort); > + g_assert_cmpint(two->kind, ==, ALT_TWO_KIND_N); > + g_assert_cmpfloat(two->n, ==, 42.5); > + qapi_free_AltTwo(two); > + two = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltThree(v, &three, NULL, &error_abort); > + g_assert_cmpint(three->kind, ==, ALT_THREE_KIND_N); > + g_assert_cmpfloat(three->n, ==, 42.5); > + qapi_free_AltThree(three); > + three = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltFour(v, &four, NULL, &err); > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_AltFour(four); > + four = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltFive(v, &five, NULL, &error_abort); > + g_assert_cmpint(five->kind, ==, ALT_FIVE_KIND_N); > + g_assert_cmpfloat(five->n, ==, 42.5); > + qapi_free_AltFive(five); > + five = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltSix(v, &six, NULL, &error_abort); > + g_assert_cmpint(six->kind, ==, ALT_SIX_KIND_N); > + g_assert_cmpint(six->n, ==, 42.5); > + qapi_free_AltSix(six); > + six = NULL; Reading this, I had to refer back to the definition of AltOne, ..., AltSix all the time. Let's rename them to AltStrBool, AltStrNum, ..., AltNumInt. > } > > static void test_native_list_integer_helper(TestInputVisitorData *data, > @@ -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);