Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates

[Markus Armbruster]

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.
>
> 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 <address@hidden>
>
> ---
> 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);