[v7,08/18] qapi: Test use of 'number' within alternates

On 09/29/2015 04:21 PM, Eric Blake wrote:
> 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 <eblake@redhat.com>
> 
> ---

> +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 fails to reset err = NULL...

> +
> +    /* 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;

...which means that this test is not reliable.  Do you need a v8, or can
you squash this in?


     v = visitor_input_test_init(data, "42");

Eric Blake <eblake@redhat.com> writes:

> On 09/29/2015 04:21 PM, Eric Blake wrote:
>> 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 <eblake@redhat.com>
>> 
>> ---
>
>> +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 fails to reset err = NULL...
>
>> +
>> +    /* 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;
>
> ...which means that this test is not reliable.  Do you need a v8, or can
> you squash this in?
>
>
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 1b5a369..6104ac6 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -395,6 +395,8 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
>      v = visitor_input_test_init(data, "42");
>      visit_type_AltStrBool(v, &asb, NULL, &err);
>      g_assert(err);
> +    error_free(err);
> +    err = NULL;
>      qapi_free_AltStrBool(asb);
>
>      v = visitor_input_test_init(data, "42");

Squashed & pushed to qapi-next.

I also updated commit messages to document the tweaks made on commit.