[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alte
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alternates |
Date: |
Thu, 24 Sep 2015 16:36:14 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
> ---
> 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);
- Re: [Qemu-devel] [PATCH v5 01/46] qapi: Sort qapi-schema tests, (continued)
[Qemu-devel] [PATCH v5 04/46] qapi: Add tests for empty unions, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 06/46] qapi: Improve 'include' error message, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alternates, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/21